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.

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

Show target version for upgrades in change set #2888

Merged
merged 3 commits into from
Oct 31, 2019

Conversation

HebaruSan
Copy link
Member

Problem

When you upgrade a module, the change set says it is going to "upgrade [...] to" the version that's already installed. That's wrong.

screenshot

Cause

The change set is a sequence of ModChange objects, each of which holds a reference to just one CkanModule object, which is always the one that's currently installed or will be installed. For upgrades, the target version is known inside GUIMod (it's SelectedMod), but is lost in the shuffle when the change set is generated until it's finally recalculated in ModuleInstaller. Since the ModChange representing an upgrade only has a reference to the version that's installed, that's what displays on screen.

Changes

Now GUIMod.GetRequestedChanges is replaced with GUIMod.GetModChanges which directly returns a sequence of ModChange objects. This allows GUIMod to pass additional info to these objects that would not have fit into a KeyValuePair<CkanModule, GUIModChangeType>, specifically a second CkanModule reference for the target version for upgrades sourced from SelectedMod, which is then used to populate the change set display.

This change simplifies ComputeUserChangeSet, which no longer has to generate its own ModChange objects from KeyValuePair<CkanModule, GUIModChangeType> objects.

Fixes #2873.

Request for assistance

I need help improving the new structure of ModChange. I do not like the name OtherMod at all, but I am drawing a blank as to what to use instead. Or maybe some other overall approach would be better? Feedback solicited!

@HebaruSan HebaruSan added Bug Something is not working as intended Easy This is easy to fix GUI Issues affecting the interactive GUI Pull request Discussion needed labels Oct 10, 2019
@DasSkelett
Copy link
Member

So as it is right now in your PR, the OtherMod property is only used for the Update ChangeType.
In this case a "downgrade" to the type ModVersion (and renaming to OtherVersion or sth) would suffice, and be more lighweight.

But either way, if we introduce different properties used for a single change type, we should go all the way with your suggestion: separate classes inheriting from ModChange.
I think that keeps the code cleaner and better structured.

@HebaruSan
Copy link
Member Author

I'm skeptical about 'lightweight', since we're just holding references to objects that already exist in the registry; a reference to a ModVersion would take up the same memory as a reference to a CkanModule. But it is true that we only need the ModVersion to achieve the current functionality.

I'll try refactoring ModChange into a hierarchy as discussed...

@HebaruSan HebaruSan added the In progress We're still working on this label Oct 16, 2019
@HebaruSan
Copy link
Member Author

OK, now we have a ModUpgrade class with a private targetMod field, and much of the complexity of UpdateChangesDialog is migrated into a virtual ModChange.Description property. I'm not 100% in love with this structure, but it does what I needed it to do without being overly messy.

Also a bunch of compilation warnings are fixed.

@HebaruSan HebaruSan removed the In progress We're still working on this label Oct 26, 2019
@DasSkelett
Copy link
Member

The GUI hangs on startup at the end of the modlist initialization.
Running in the IDE shows that this exception is thrown:

System.InvalidOperationException: RunSynchronously may not be called on a task that has already completed.
  at System.Threading.Tasks.Task.InternalRunSynchronously (System.Threading.Tasks.TaskScheduler scheduler, System.Boolean waitForCompletion) [0x0003c] in <285579f54af44a2ca048dad6be20e190>:0
  at System.Threading.Tasks.Task.RunSynchronously () [0x00006] in <285579f54af44a2ca048dad6be20e190>:0
  at CKAN.Main._UpdateModsList (System.Collections.Generic.IEnumerable`1[T] mc, System.Collections.Generic.Dictionary`2[TKey,TValue] old_modules) [0x002eb] in /home/kilian/git/CKAN/GUI/MainModList.cs:239
  at CKAN.Main+<>c__DisplayClass270_0.<UpdateModsList>b__0 () [0x00000] in /home/kilian/git/CKAN/GUI/MainModList.cs:166
  at System.Threading.Tasks.Task.InnerInvoke () [0x0000f] in <285579f54af44a2ca048dad6be20e190>:0
  at System.Threading.Tasks.Task.Execute () [0x00000] in <285579f54af44a2ca048dad6be20e190>:0
  at CKAN.Main.UpdateModsList (System.Collections.Generic.IEnumerable`1[T] mc, System.Collections.Generic.Dictionary`2[TKey,TValue] old_modules) [0x00071] in /home/kilian/git/CKAN/GUI/MainModList.cs:165
  at System.Runtime.CompilerServices.AsyncMethodBuilderCore+<>c.<ThrowAsync>b__7_0 (System.Object state) [0x00000] in <285579f54af44a2ca048dad6be20e190>:0

@HebaruSan
Copy link
Member Author

... great.

GUI/MainChangeset.cs Outdated Show resolved Hide resolved
Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this merged before another one opens a issue for this bug!

@HebaruSan HebaruSan merged commit 12d76a3 into KSP-CKAN:master Oct 31, 2019
@HebaruSan HebaruSan deleted the fix/upgrade-version branch October 31, 2019 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Discussion needed Easy This is easy to fix GUI Issues affecting the interactive GUI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] New mod version isn't showed on the update screen
2 participants