Throw explanatory message when user selects Incompatible mods #2237
Conversation
|
Ok, that re-enables the mod highlighting when an inconsistency is found. Still have three error dialogs popping up, though. I don't know why that is the case. |
politas
changed the title
| } | ||
| } | ||
|
|
||
| } | ||
| user_requested_mods.Add(module); | ||
| Add(module, new SelectionReason.UserRequested()); |
There was a problem hiding this comment.
It feels wrong for the GUI to tell me that the mod I just clicked can't be installed, but then to add it to the install list anyway. Can we skip these steps on conflicts?
There was a problem hiding this comment.
The user may want to deselect the other mod instead. As long as we're telling them what the problem is, flagging the conflicting mods, and disabling the Apply button, I think this is consistent with past behaviour and makes sense. The user has selected this mod because they want it. At this stage, we don't know which of the conflicts they prefer, so better to accept the selection and let them fix it. Otherwise, they may need to scroll to the other mod, unselect it, then scroll back to here and re-select this mod, making far more effort.
| @@ -216,15 +219,13 @@ public void AddModulesToInstall(IEnumerable<CkanModule> modules) | |||
| } | |||
| else | |||
| { | |||
| throw new InconsistentKraken(string.Format("{0} conflicts with {1}, can't install both.", module, | |||
| listed_mod)); | |||
| inconsistencies.Add(string.Format("{0} conflicts with {1}, can't install both.", module, listed_mod)); | |||
There was a problem hiding this comment.
Exceptions ought to have machine-readable descriptions of the problem, not just strings for the user. Can we update InconsistentKraken to use something like List<KeyValuePair<CkanModule, CkanModule>>? Then there would be the possibility of handling the problem programmatically, prompting the user, etc.
Maybe this could be a new ConflictKraken class if InconsistentKraken is needed elsewhere.
GUI/MainModList.cs
Outdated
| continue; | ||
| } | ||
| catch (TooManyModsProvideKraken k) | ||
| { | ||
| kraken = k; | ||
| } | ||
| catch (InconsistentKraken k) | ||
| { | ||
| user.RaiseError(k.InconsistenciesPretty); |
There was a problem hiding this comment.
This kraken doesn't format its errors very well; the bullet point is missing from the first item in the list because it's added with string.Join. Can we fix that before users start seeing it?
It would also be nice to have an extra line break between the initial message and the list.
There was a problem hiding this comment.
Further comment on that popup, it sounds like programmerese with its use of "inconsistencies", and it should tell the user the most important thing more clearly: ConfigurableContainers (in this case) can't be installed. Could that replace "Error!" as the title?
There was a problem hiding this comment.
There are a couple of other string formats used by InconsistencyKraken, so I've left them and just moved all the Conflicts to a new ConflictsKraken which uses the KeyValuePairs as you suggested.
GUI/Main.cs
Outdated
| { | ||
| // Need to be recomputed due to ComputeChangeSetFromModList possibly changing it with too many provides handling. | ||
| user_change_set = mainModList.ComputeUserChangeSet(); | ||
| new_conflicts = MainModList.ComputeConflictsFromModList(registry, user_change_set, CurrentInstance.VersionCriteria()); |
There was a problem hiding this comment.
ComputeConflictsFromModList's type signature and approach are unsafe; it has a ToDictionary call on a list with potentially non-unique keys, which can throw an ArgumentException. Basically it can't represent conflicts between a given mod and multiple other mods, which this PR is trying to do (more). This would be another good spot to use List<KeyValuePair<CkanModule, CkanModule>>.
There was a problem hiding this comment.
So, I didn't actually change that code at all, and I'm not entirely sure how this task works or what calls it. This is one of the main reasons I'm unsure about this change.
politas
force-pushed
the
fix/1864-inconsistent-kraken-2
branch
from
135597a to
1bf5b7f
Compare
|
This change does seem to be slowing down the response when clicking a mod. I'm not sure why |
politas
force-pushed
the
fix/1864-inconsistent-kraken-2
branch
from
1bf5b7f to
6e53efa
Compare
politas
force-pushed
the
fix/1864-inconsistent-kraken-2
branch
from
201c258 to
8ca4f94
Compare
|
Nope. I cannot figure this damn thing out. |
This strangely throws multiple dialogs, but it does at least throw a message. Changed unit test because MainModList no longer throws an inconsistentKraken
I'm really not sure this is the right way to do this. I'm going to try another way. If anyone has any ideas of how this code ought to be working, please chime in. As per the comment in GUI/Main.cs, the fundamental flaw may be the TooManyProvides handling loop messing with it.
Closes #1864