Enhancement of options management #63
Conversation
…mented path browse buttons
…options, implemented interactions with OptionsWindow
MainWindow.xaml
Outdated
| <ToolBarTray Grid.Row="0" Grid.Column="0" Grid.ColumnSpan="2" Height="30" HorizontalAlignment="Stretch" IsLocked="True"> | ||
| <ToolBar> | ||
| <Button x:Name="tbr_Properties" Content="Properties" Command="Properties" Click="tbr_Properties_Click" CommandManager.CanExecute="tbr_Properties_CanExecute"/> | ||
| <Button x:Name="tbr_Options" Content="Options" Click="tbr_Options_Click" CommandManager.CanExecute="tbr_Options_CanExecute"/> |
There was a problem hiding this comment.
I think that the Command="Properties" was required in some way, but I cannot confirm why.
MainWindow.xaml.cs
Outdated
| e.CanExecute = true; | ||
| } | ||
| //_optionsWindow.Owner = this; | ||
| //_optionsWindow.WindowStartupLocation = WindowStartupLocation.CenterOwner; |
There was a problem hiding this comment.
Why are these two lines commented out? They seem like pretty valid options to enable.
There was a problem hiding this comment.
I commented them because they didn't work as expected but probably the problem was somewhere else.
| string dicPath = _options.dicPath; | ||
| string sgRawPath = _options.sgRawPath; | ||
| string psxtPath = _options.psxtPath; | ||
| string subdumpPath = _options.subdumpPath; |
There was a problem hiding this comment.
No need to set these here if you just call the _options.blahPath variables below.
There was a problem hiding this comment.
Actually I did this to avoid renaming them all and keep the following code more readable. I'd avoid making string copies if it's possible in C# (eg. ref string dicPath = ref _options.dicPath) but it's a micro optimization.
There was a problem hiding this comment.
In the rest of the code you still reference the _options. ones, though. So if you want to make the least changes, I'd keep those variables more global and just have the setter there. Then you shouldn't have to worry. Or go the other way and remove the usage of those variables completely.
There was a problem hiding this comment.
Yes, that's because we're dealing with StartDumping() function which is the big boy and each path is potentially (and actually) required multiple times, this is not true in the single cases around for which aliasing the name was just redundant.
There was a problem hiding this comment.
Okay, that makes sense in this case. Looks good, just need to test it.
MainWindow.xaml.cs
Outdated
|
|
||
| // Validate that the required program exits | ||
| if (!File.Exists(dicPath)) | ||
| // Validate that the required program exits and it's not DICUI itself |
There was a problem hiding this comment.
Nit: I can't spell apparently... If you can change program exits to program exists, I'd appreciate it
|
I'll have to test to see how the new window plays out with saving and loading, but the couple small comments are all I could see. |
|
I was able to test locally. The new options box looks much cleaner than it did before. I also confirmed that the saving and loading of options was consistent. This is going to be a nit, but later on, it might be cool to have the output directory automatically update if you change the default. This isn't necessary for this change, just something fun for the future. |
2 participants
This pull request refactors the management of current path properties:
Optionsfile which also manage persistenceOptionsWindow.xamlfile is createdThis refactor is not meant to be final since
Optionscurrently relies on reflection for a fast replacement solution of actual management, but we'll fix this successively.