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

Modpack usability fixes #3243

Merged
merged 2 commits into from
Feb 6, 2021
Merged

Modpack usability fixes #3243

merged 2 commits into from
Feb 6, 2021

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Dec 20, 2020

Background

@Kerbal-Propulsion-Laboratory (ezee on Discord) presented us with the idea of a metapackage in a custom repo. It works, with quirks.

Problems

  • The Download button in mod info is enabled for metapackages
  • The right click menu download option is enabled for metapackages
  • Exporting a modpack doesn't populate the author and release date
  • The changeset tab has only two options, Clear and Apply, neither of which lets you go back to the modlist with the same changeset intact
  • The column headers on the changeset tab sometimes disappear on Mono
  • The "release status" and "replaced by" fields in mod info are always shown even when they're not set, cluttering the display
  • The version of the current Breaking Ground expansion is not detected, so its other metadata (such as the store links) isn't available
  • In Mono, the mod info panel flickers noticeably when you select a mod row
  • The versions tab of mod info is slow to refresh when clicking Scatterer and frequently says that compatible modules are incompatible
  • If you install a large module hosted on a server that doesn't send the download size:
    41176 [1] ERROR CKAN.ErrorDialog (null) - Unhandled exception:
    System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.ArgumentOutOfRangeException: '-61800' is not a valid value for 'Value'. 'Value' should be between 'Minimum' and 'Maximum'
    Parameter name: Value
      at System.Windows.Forms.ProgressBar.set_Value (System.Int32 value) [0x00027] in <47c199caaa3c4c6c9c48a7c6ebb3fd29>:0 
      at (wrapper remoting-invoke-with-check) System.Windows.Forms.ProgressBar.set_Value(int)
      at CKAN.Wait+<>c__DisplayClass13_0.<SetModuleProgress>b__0 () [0x00161] in <3cb683943e2a481fbd7ca14981128ab6>:0 
      at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&)
    at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x0006a] in <9f0df102fe6e4cfea29d2e46f585d8a5>:0 
    
  • The "Automatically sort by 'Update'-column when clicking 'Add available updates'" setting overwrites your current sort, so if you chose a complex multi-column sort, it's gone after you update

Causes

  • <KSP>/GameData/SquadExpansion/Serenity/readme.txt now has whitespace after the version string, but the standard DLC detector doesn't allow this
  • Mono's DataGridView has some quirks in how it fires its SelectionChanged event:
    • Usually it fires twice—once to deselect the previously selected row (with an empty selection) and once to select the newly selected row (with it as the selection)—which caused mod info to be collapsed and expanded rapidly every time the selection changes
    • Occasionally it fires multiple times in a row with the same selection
      • If Mod info was told to show the same mod twice in a row, it would perform all the same visual updates all over again, ending up in the same state but with extra flickering
  • The tag list in mod info uses a FlowLayoutPanel, which was updated without turning off its layout, exacerbating flickering caused by repeated rapid updates
  • The versions tab of mod info has its own logic for relationships which is slow and inaccurate when virtual modules are involved
  • The per-module progress bars from Non-parallel GitHub downloads, one progress bar per download #3054 don't clamp the calculated percentage to a minimum or maximum value, in effect assuming that the server will always send the file size, but it's a big negative value if that isn't the case
  • We treat the update-sort as if it was the same as clicking a column header

Changes

  • Now the Download button in mod info is disabled for metapackages and the label explains there's no download
  • Now the right click menu download option is disabled for metapackages
  • Now the export modpack screen has an author field, defaulting to your system username (use commas to separate multiple authors)
  • Now exporting a modpack sets the release date to the current time
  • Now the changeset tab has a Back button which returns to the modlist without clearing the changeset
  • Now the workaround from Force redraw of versions ListView on Mono #2685 is applied to the changeset tab, preserving the column headers
  • Now the "release status" and "replaced by" fields in mod info are hidden when they're not set
  • Now the standard DLC detector allows whitespace after the version string
  • Now we only fire ManageMods_OnSelectedModuleChanged if the selected module is not null, which will suppress the superfluous collapse/expand events
  • Now mod info doesn't update itself if it's already showing the info for the given mod, which will reduce flickering
  • Now we call tag list's FlowLayoutPanel.SuspendLayout before we update the tags and TableLayoutPanel.ResumeLayout after, which will also reduce flickering
  • Now the versions tab of mod info uses ModuleInstaller.CanInstall, which is now public, and which is faster and more accurate, so the compatibility shown is correct
  • Now the per-module progress bars handle missing server download size info without crashing
  • Now the update-sort feature adds the Update column to the beginning of the sorted columns list, with the previous setting retained as secondary sorts. Once updates are installed, everything in the column will be -, and the previous sort restores itself by default.

@HebaruSan HebaruSan added Bug Something is not working as intended Enhancement New features or functionality GUI Issues affecting the interactive GUI Core (ckan.dll) Issues affecting the core part of CKAN Pull request labels Dec 20, 2020
@HebaruSan HebaruSan requested a review from DasSkelett December 20, 2020 07:58
@HebaruSan HebaruSan force-pushed the fix/modpacks branch 5 times, most recently from 29bfc5c to e04fc8c Compare December 21, 2020 19:02
@DasSkelett
Copy link
Member

The per-module progress bars from #3054 don't clamp the calculated percentage to a minimum or maximum value, in effect assuming that download_size will always be set (but it's optional according to the spec), but it's a big negative value if download_size is null

Hm, I'm missing how download_size affects the progress bar. remaining and total come from the WebClient agent (through several layers of encapsulation).
I guess download_size somehow gets written to total? Where, and why doesn't it throw a DivideByZeroException if download_size and total are 0?

@HebaruSan
Copy link
Member Author

Hm, I'm missing how download_size affects the progress bar. remaining and total come from the WebClient agent (through several layers of encapsulation).
I guess download_size somehow gets written to total? Where[...]?

download_size is fed into the system here:

item.Value.download_size,

... filling the size parameter of DownloadTarget:

public DownloadTarget(Uri url, Uri fallback = null, string filename = null, long size = 0, string mimeType = "")

NetAsyncDownloaderDownloadPart then copies that to its size and bytesLeft properties:

this.size = bytesLeft = target.size;

You're right that those properties should be overwritten with values from WebClient here, though.

download.size = bytesToDownload;
download.bytesLeft = download.size - bytesDownloaded;

Now I'm confused.

and why doesn't it throw a DivideByZeroException if download_size and total are 0?

Good question; I don't remember. I'll have another look at all this...

@HebaruSan
Copy link
Member Author

Following the logical chain and a possibly relevant Stackoverflow question, I think DownloadProgressChangedEventArgs.TotalBytesToReceive must have been -1 due to the server not sending it. But now I am not able to find such a server (SpaceDock and GitHub both send it). I'll edit the description to remove download_size since that looks like an incorrect explanation.

I wish I could remember which mod I was trying to install when that exception was thrown. Maybe Kopernicus_BE?

@DasSkelett
Copy link
Member

Maybe Sarbian's Jenkins? But he doesn't have any "big" mods there.
In any case, your change should fix it regardless of why, when and how it occurred.
Just pushed a commit with the German translations for the new labels, feel free to squash.

@DasSkelett DasSkelett added the i18n Issues regarding the internationalization of CKAN and PRs that need translating label Feb 6, 2021
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.

Other changes look fine as well. More QoL enhancements 🎉

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 Core (ckan.dll) Issues affecting the core part of CKAN Enhancement New features or functionality GUI Issues affecting the interactive GUI i18n Issues regarding the internationalization of CKAN and PRs that need translating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants