Implement importing appx. #3
Conversation
MCLauncher/MainWindow.xaml.cs
Outdated
| Nullable<bool> result = openFileDlg.ShowDialog(); | ||
| if (result == true) | ||
| { | ||
| string directory = openFileDlg.SafeFileName + " (imported)"; |
There was a problem hiding this comment.
Is there a reason to add (imported) to the actual directory name?
I think that this shouldn't be appended in the dirname, but instead just appended in the display layer.
MCLauncher/MainWindow.xaml.cs
Outdated
| string directory = openFileDlg.SafeFileName + " (imported)"; | ||
| if (Directory.Exists(directory)) | ||
| { | ||
| Directory.Delete(directory, true); |
There was a problem hiding this comment.
Please add a question dialog before removal.
MCLauncher/MainWindow.xaml.cs
Outdated
| @@ -52,6 +53,22 @@ public partial class MainWindow : Window, ICommonVersionCommands { | |||
| }); | |||
| } | |||
|
|
|||
| private async void Button_Click(object sender, RoutedEventArgs e) | |||
There was a problem hiding this comment.
Please rename this to something that makes more sense, eg. ImportButtonClick
There was a problem hiding this comment.
Also, code style note:
This project uses inline { and } brackets. Please reconfigure your IDE to follow this codestyle and update the changes.
MCLauncher/MainWindow.xaml.cs
Outdated
| @@ -228,8 +245,21 @@ public partial class MainWindow : Window, ICommonVersionCommands { | |||
| }); | |||
| } | |||
|
|
|||
| private void InvokeRemove(Version v) { | |||
| Directory.Delete(v.GameDirectory, true); | |||
| private async void InvokeRemove(Version v) { | |||
There was a problem hiding this comment.
By making this async you are introducing a significant issue - the button can now be clicked twice and the function will be invoked twice. TL;Dr you probably don't want that unless you can either add special handling for that case and/or can display the progress to user somehow.
As I believe that displaying progress for removal is out of scope for this PR this should be rewritten to remove the async prefix.
MCLauncher/MainWindow.xaml.cs
Outdated
| @@ -263,20 +293,28 @@ public class Versions : List<Object> { | |||
| public class Version : NotifyPropertyChangedBase { | |||
|
|
|||
| public Version() { } | |||
| public Version(string uuid, string name, bool isBeta, ICommonVersionCommands commands) { | |||
| public Version(string uuid, string name, bool isBeta, ICommonVersionCommands commands, string directory = "") { | |||
There was a problem hiding this comment.
Why "" instead of null?
I'd argue that in this case a separate constructor might be better as well.
MCLauncher/VersionList.cs
Outdated
| string[] subdirectoryEntries = Directory.GetDirectories("."); | ||
| foreach (string subdirectory in subdirectoryEntries) | ||
| { | ||
| if (subdirectory.Contains("(imported)")) |
There was a problem hiding this comment.
This should probably be replaced with a scan for directories matching the following criteria: has AppxManifest.xml and Minecraft.Windows.exe (or the older one, how was it called?) inside.
|
I've amended this with some UI fixes and adjustments. |
Implement importing appx versions directly from the application.