Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CKAN installs recommendations even if told otherwise on "Audit recommendations" #2604

Closed
DasSkelett opened this issue Dec 8, 2018 · 5 comments
Closed

CKAN installs recommendations even if told otherwise on "Audit recommendations" #2604

DasSkelett opened this issue Dec 8, 2018 · 5 comments
Labels
Bug Something is not working as intended GUI Issues affecting the interactive GUI Relationships Issues affecting depends, recommends, etc.

Comments

@DasSkelett
Copy link
Member

DasSkelett commented Dec 8, 2018 •
edited
Loading

Background

CKAN Version:
1.25.4

Operating System:
Linux - Kubuntu 18.04.1

Have you made any manual changes to your GameData folder (i.e., not via CKAN)?
Nope

Problem

What steps did you take in CKAN?

  1. Have any mod with recommendations installed (in my example RasterProp Monitor)
  2. Select any not installed mod in mod list (not necessary for this issue, see down below)
  3. Go to File > Audit recommendations
  4. Select any of the suggestions which itself has recommendations (in my example SCANsat, shows recursion problem) and continue
  5. Deselect the recommendation (In my example Contract Configurator) and continue
  6. See that, even if you deselected it, the recommendation (Contract Configurator) is installed, as well as a recommendation of the recommendation (technically of another recommendation) (Waypoint Manager), which you never got to choose if you want to install it or not.

What did you expect to happen?
After you deselect the recommendation, it shouldn't get installed.
Changeset is saved.

What happened instead?
It was installed, and a recommendation of this recommended mod as well.
Changeset was not saved.

It seems that CKAN skips the recommendation evaluation (recursively) if you go over File > Audit recommendations instead of the Apply Changes button.
Furthermore, the in step 2 install-marked mod is unmarked again, so the changeset is not saved.

I might look myself into this bug tomorrow. HebaruSan was faster.
@HebaruSan HebaruSan added Bug Something is not working as intended GUI Issues affecting the interactive GUI Relationships Issues affecting depends, recommends, etc. labels Dec 8, 2018
@HebaruSan
Copy link
Member

I think it's because I used DefaultOpts to start the installation here:

CKAN/GUI/MainRecommendations.cs

Line 339 in 7413c23

RelationshipResolver.DefaultOpts()

This sets with_recommends:

CKAN/Core/Relationships/RelationshipResolver.cs

Lines 190 to 202 in 7413c23

// TODO: This should just be able to return a new RelationshipResolverOptions
// and the defaults in the class definition should do the right thing.
public static RelationshipResolverOptions DefaultOpts()
{
var opts = new RelationshipResolverOptions
{
with_recommends = true,
with_suggests = false,
with_all_suggests = false
};
return opts;
}

Normal installation uses this:

CKAN/GUI/MainChangeset.cs

Lines 122 to 123 in 7413c23

RelationshipResolverOptions install_ops = RelationshipResolver.DefaultOpts();
install_ops.with_recommends = false;

It'd be nice if there was an alternative to DefaultOpts that actually made sense to use...

@HebaruSan HebaruSan mentioned this issue Dec 8, 2018
Merged
@DasSkelett
Copy link
Member Author

DasSkelett commented Dec 8, 2018 •
edited
Loading

Ha! I forgot to write something in the issue, added now:
If you have a not empty changeset when auditing and installing recommendations, it is lost.
We have basically two options:

  1. Apply the preselected changeset together with the recommendations installation
  2. Or save the changeset, "create a new one" (i.e. clear it, as it happens now) and reload the saved changeset after the process is complete.

@HebaruSan do you want to add it to your PR or should I/You make a new one?

@HebaruSan
Copy link
Member

I guess my question is, what would be the user intention behind doing that? If they intend to install the change set, I would think they would do that before messing with Audit recommendations.

Maybe we should disable the Audit recommendations menu option when there's a change set, similar to how the apply changes button is disabled when there isn't one. That way this scenario wouldn't be possible.

@DasSkelett
Copy link
Member Author

That's the 3rd option :)
It might really be a bit unrealistic. I noticed it during testing, where I went full QA.
In normal circumstances a user will probably act as you stated. So disabling the option might be the best choice.

@HebaruSan
Copy link
Member

OK, third option added to the fix PR. We can change it later if someone presents a compelling case.

Sorry, something went wrong.

This was referenced Feb 26, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Something is not working as intended GUI Issues affecting the interactive GUI Relationships Issues affecting depends, recommends, etc.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@HebaruSan @DasSkelett