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

Cache path setting fixes #3804

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Mar 6, 2023

Problem

If you go to the settings and try to type a valid full path with drive letters into the cache path field, it silently gets "stuck" after the drive letter behind the scenes. Upon re-opening the settings the field is blank and can't be edited:

image

The setting saved to disk is like:

  "DownloadCacheDir": "E:",

Installing mods then fails with some strange text about a System Volume Information folder:

image

Cause

TrySetupCache is supposed to determine whether a cache path is valid, but an exception was being thrown in NetModuleCache.GetSizeInfo when the path was E:, after TrySetupCache succeeded, which corrupted the display and caused updatingCache to be true forever, which suppressed all further calls to update the setting while you typed the rest of the path.

Later, when we tried to use the cache during an install, NetModuleCache.GetSizeInfo again threw its exception and broke everything.

Changes

  • Now before we check whether the path segments match up, GetDrive() directly compares the given path to the drive's RootDirectory in the hopes of avoiding NREs for paths like E:
  • Now TrySetupCache tries to check the free space associated with a cache path before changing the setting to it, to ensure it only ends up with valid values, which should make it fail for E:
  • Now if we fail to initialize the cache at startup, we revert the cache path setting to its default and try again, to make sure we don't end up with an invalid cache object, to provide a recovery path for affected users
  • Now instead of trying to change the cache setting with every keystroke, the settings window creates its own temporary cache object to validate the path, then tries to change the setting when the window is closed. If that fails, it'll display an error (again) and make you go back and fix it.
  • Now we don't create the downloading folder until you actually use a cache folder (previously if you typed a long path, we'd try creating it at each level because it was in NetModuleCache's constructor)
  • Now the English strings for the settings window say "MiB" instead of "MB" since that's technically more correct

Fixes #3801.

@navuek, there should be a test build that you can try here in a few minutes on the Checks tab under the Artifacts dropdown.

@HebaruSan HebaruSan added Bug Something is not working as intended GUI Issues affecting the interactive GUI Core (ckan.dll) Issues affecting the core part of CKAN Pull request Windows Issues specific for Windows labels Mar 6, 2023
Copy link
Member

@techman83 techman83 left a comment

Choose a reason for hiding this comment

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

That makes a lot of sense, LGTM.

@HebaruSan HebaruSan merged commit 2d387d9 into KSP-CKAN:master Mar 7, 2023
@navuek
Copy link

navuek commented Mar 7, 2023

Thank you very much for the quick assist!!

@HebaruSan HebaruSan deleted the fix/cache-path-validation branch March 7, 2023 07:18
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 GUI Issues affecting the interactive GUI Windows Issues specific for Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Cache path setting can be just a drive letter but it doesn't work
3 participants