-
-
Notifications
You must be signed in to change notification settings - Fork 344
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
Properly determine the game when cloning instances #3478
Properly determine the game when cloning instances #3478
Conversation
e9c2f1a
to
b90b031
Compare
This comment has been minimized.
This comment has been minimized.
Ah, that's why it's passed to |
I just got the idea that we could reassign the |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b90b031
to
57bf514
Compare
This comment has been minimized.
This comment has been minimized.
ca18516
to
1e00cd2
Compare
1e00cd2
to
2f55a1e
Compare
2f55a1e
to
a6e0bec
Compare
One more update, it's weird that the instance list shows that we know each instance's game, but then when we go to clone them we may prompt the user. Pushed a commit to make the prompting a fallback if we can't get the game from the instance (i.e., if the paths don't match). |
57141ee
to
90be65d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me now, but in case you want to make some more changes, I'll wait a couple of days before merging it.
Problem
A user reported a problem when trying to clone a KSP instance, they're getting a
Clone failed: Object reference not set to an instance of an object
error.We couldn't quite figure out what the user was doing exactly, the messages were kinda vague:
But after looking at the clone UI logic, these lines stood out as a possible cause for a NRE:
CKAN/GUI/Dialogs/CloneFakeGameDialog.cs
Lines 149 to 156 in 2564dfb
They were refactored in #3223 and didn't account for the possibility of a path being entered that didn't already belong to a known instance, which would result in a
null
sourceInstance
.Changes
The underlying issue that forced this instance lookup was that we didn't have a good way to determine the game of the instance to clone.
In
GameInstanceManager.AddInstance()
we already had a basic layout for this, but it was missing the option to prompt the user to choose between multiple games if it detected multiple potential ones.This logic is now moved into a new
DetermineGame()
method, which calls User.RaiseDelectionDialog
in the aforementioned case.Both the CloneFake dialog and the AddInstance button use the new
DetermineGame()
method.For testing you can simply copy
KerbalSpaceProgram.cs
, rename the file and class, and add it toGameInstanceManager.knownGames
to force multiple games to be detected.