Implementation of clone and fake in GUI #2665
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HebaruSan
added
Enhancement
|
Can we make this a dropdown option on the "Add new" button in the instance selection window instead? We have a
|
Use RaiseMessage instead of RaiseProgress Call UpdateInstancesList() at the end of the clone/fake tasks to update the listt in the instance selection dialog
|
So have it now as a suboption of the KSP selection dialog, see the screenshot in the PR message. One more thing: I now pass the current |
|
I get this when I click Select Known Instance: I think it's because |
HebaruSan
reviewed
Jan 23, 2019
|
Let's set the menu's renderer in Mono so it'll look nice with dark themes: Lines 28 to 31 in c49489b |
|
I think this is a different problem, happens if I click Select Known Instance when the select instance window appears at startup (so before the main form has displayed): Presumably |
|
Yep, get it too. The call stack: This part: There Edit: well, you got it too in your comment, didn't see the edit before I typed mine. |
|
There doesn't seem to be any validation around the selection of the instance to be cloned. I can put garbage in the text box and click OK, and instead of an error it just closes the dialog and returns me to the original list of instances.
|
|
Yeah, actually I completely forgot to process direct textbox inputs. It does nothing whether the path is valid or not. |
HebaruSan
reviewed
Jan 24, 2019
|
Calling |
Fix `NullReferenceException` for broken KSP installs Fix `NullReferenceException` if you open the dialog at startup Set the dropdown menu renderer in mono Consider manual path textbox entries for clone and validate them Better handle the tasks, don't close window before they finished, but show progress bar
|
This should solve all the issues @HebaruSan reported yet. I moved I close the dialog window out of the tasks, so I need to invoke the |
|
There's still something fishy with the validation and/or threading. If I open the dialog and click OK it says to fill in the new name and path, and then if I click OK again after doing that I get a very hard crash. |
|
Hmm, I cannot recreate this crash.
|
|
Yes, that's what I remember doing. ... Confirmed with another attempt, I just repeated it. |
|
I redid the task code a bit: |
|
Looks better, now I get an explanatory error:
However then the window changes to a weird size with a bunch of the controls truncated:
Haven't looked at the code yet but maybe there's a progress bar mode that's not being exited cleanly? |
|
It was a mono window size problem again... |
|
Created DasSkelett#2 with my UI suggestions. |
|
If we're adding all this functionality, we should rename the "Select KSP Instance" dialog and menu item to "Manage KSP Instances" |
|
I note that it somehow didn't get the right destination folder, so: This may be due to /home/politas/Desktop/KSP/KSP-Steam being a symlink. |
|
I see that the error message is also wrong, IOException is not the right one to throw for invalid destination paths. I think I'll create a new Kraken for that. For your first issue: This makes me think if it even makes sense to copy hidden files and folders too when cloning KSP instances. Probably not, but I would like to hear your opinions first before I change |
|
Not sure what the deal was with that .git directory. It may have been an invalid link thanks to having moved the Steam library to a different hard disk. When I delete the useless .git* directory entries, cloning works on that instance, too. So the only issue there is the error message in the second screenshot. |
|
Oups, forgot the new Kraken. Hang on... Edit: Actually, we have a |
politas
added a commit
that referenced
this pull request
Feb 8, 2019
DasSkelett
added a commit
to DasSkelett/CKAN
that referenced
this pull request
Feb 11, 2019
PR KSP-CKAN#2665 changed the IOException to PathErrorKraken in `CopyDirectory()`. Now the new tests for `CopyDirectory()` assert the new kraken.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
My attempt to implement the clone and fake functions in the GUI.
Accessible via File > Select KSP Install >New KSP instance > Clone or fake new instance:
The window itself looks like this:
or on windows:
Won't win beauty prizes with that, but it works (hopefully).
Should be pretty self-explanatory (haha).
Has all the features of the CLI implementation: