Skip to content

Improve handling of missing game version #2444

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

Merged
merged 1 commit into from
May 15, 2018

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented May 13, 2018

Problem

If you:

  1. Have exactly one game instance
  2. Set at least one additional compatible version in the compatible versions dialog
  3. Remove buildID.txt and readme.txt, without removing the KSP folder or the CKAN folder
  4. Launch CKAN

... then on load, you get the compatible versions popup warning you about a version change (if in GUI), and clicking Save causes an exception:

************** Exception Text **************
System.NullReferenceException: Object reference not set to an instance of an object.
   at CKAN.KSP.SaveCompatibleVersions()
   at CKAN.CompatibleKspVersionsDialog.SaveButton_Click(Object sender, EventArgs e)
   at System.Windows.Forms.Control.OnClick(EventArgs e)
   at System.Windows.Forms.Button.OnClick(EventArgs e)
   at System.Windows.Forms.Button.OnMouseUp(MouseEventArgs mevent)
   at System.Windows.Forms.Control.WmMouseUp(Message& m, MouseButtons button, Int32 clicks)
   at System.Windows.Forms.Control.WndProc(Message& m)
   at System.Windows.Forms.ButtonBase.WndProc(Message& m)
   at System.Windows.Forms.Button.WndProc(Message& m)
   at System.Windows.Forms.NativeWindow.Callback(IntPtr hWnd, Int32 msg, IntPtr wparam, IntPtr lparam)

(One way to make this happen is apparently to uninstall in Steam, then re-install to a different Steam library folder. See #2443.)

The problem is in Core, so the other UIs are also affected, but the details vary.

Causes

KSP.Version() is allowed to be null in most places (and it happens when the buildID.txt and readme.txt files are missing), but the code to load and save compatible versions doesn't like it. (This is somewhat ironic as that code would be one of the better ways of handling this situation, since you could use it to set the correct game version manually.)

For saving, ToString() is called on the null reference.

For loading, if compatible_ksp_versions.json has a null value for the VersionOfKspWhenWritten property, it is passed to KspVersion.Parse, which throws exceptions on null. (That property can't be null currently, but it will be possible once we allow the save to proceed with a null.)

To be considered invalid, a game instance must lack a GameData folder. If it merely lacks the files that provide the game version, this is treated as fine; you can attempt to open and use such an instance even though we know it's missing at least some data that should be there.

Changes

Now the null conditional operator ("?.") is used to avoid a null reference exception at save. This allows a null value to be written to the file.

Now we use KspVersion.TryParse instead of Parse to allow VersionOfKspWhenCompatibleVersionsWereStored to be set to null at load.

Finally, a game instance without a version is now treated as invalid. This means you can't open them from the Select KSP Install window, and they won't be auto-opened at launch. If your only known instance loses its version info (say, by uninstalling in Steam), then it'll automatically open the Select KSP Install window instead.

Fixes #2443.

@HebaruSan HebaruSan added Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN Pull request Tests Issues affecting the internal tests and removed Bug Something is not working as intended labels May 13, 2018
@HebaruSan HebaruSan changed the title Fix crashes in compat logic with missing game version Improve handling of missing game version May 14, 2018
@HebaruSan HebaruSan force-pushed the fix/compat-popup-null branch from d251486 to 7ce35a5 Compare May 14, 2018 00:47
@politas
Copy link
Member

politas commented May 14, 2018

1) Failed : Tests.Core.KSP.SaveCompatibleVersions_MissingVersionData_NoCrash Expected: No Exception to be thrown

New test is failing?

@HebaruSan HebaruSan force-pushed the fix/compat-popup-null branch from 7ce35a5 to a4077e9 Compare May 14, 2018 15:17
@HebaruSan
Copy link
Member Author

Oh yeah, that test no longer applies after the latest changes. Replaced with one confirming that Valid is false.

@politas
Copy link
Member

politas commented May 15, 2018

Gotta say, if we're chasing down bugs this obscure, we must have a pretty solid app!

Sorry, something went wrong.

@politas politas merged commit a4077e9 into KSP-CKAN:master May 15, 2018
politas added a commit that referenced this pull request May 15, 2018
@HebaruSan HebaruSan deleted the fix/compat-popup-null branch May 15, 2018 13:36
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 Tests Issues affecting the internal tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants