Separated of System and Disc Type #56
Conversation
mnadareski
left a comment
mnadareski
left a comment
There was a problem hiding this comment.
See inline comments below.
| if (currentSystem != null && currentSystem.Item2 != KnownSystem.NONE) | ||
| { | ||
|
|
||
| List<Tuple<string, DiscType?>> allowedDiscTypesForSystem = Utilities.Validation.GetValidDiscTypes(currentSystem.Item2) |
There was a problem hiding this comment.
Why not just modify what comes out of GetValidDiscTypes? It seems that would make the code cleaner.
| private List<Tuple<char, string, bool>> _drives { get; set; } | ||
| private List<int> _driveSpeeds { get { return new List<int> { 1, 2, 3, 4, 6, 8, 12, 16, 20, 24, 32, 40, 44, 48, 52, 56, 72 }; } } | ||
| private List<Tuple<string, KnownSystem?, DiscType?>> _systems { get; set; } | ||
| private List<Tuple<string, KnownSystem?>> _systems { get; set; } |
There was a problem hiding this comment.
For the time being, can you also have a persistent _discTypes list here as well? That helps keep track of what is being used globally.
There was a problem hiding this comment.
Sure, I'll build a List<DiscType?> of all currently used types for the systems, which sounds like what we're looking for since it's just to track it.
MainWindow.xaml.cs
Outdated
| { | ||
| Utilities.Validation.DetermineFlags(customParameters, out type, out system, out string letter, out string path); | ||
| string letter, path; | ||
| Utilities.Validation.DetermineFlags(customParameters, out type, out system, out letter, out path); |
There was a problem hiding this comment.
I know you had problems with the older version of VS and this, but can you put this back to inline variable declaration? It's much cleaner that way.
MainWindow.xaml.cs
Outdated
| FileName = dicPath, | ||
| Arguments = parameters, | ||
| }, | ||
| } |
There was a problem hiding this comment.
This is a pedantic nit: I leave the dangling commas so that future changes touch less code. It's something I've had to do for work so it's built in. If you don't mind returning that, I'd appreciate it.
There was a problem hiding this comment.
Sure I will but I don't guarantee to remember to put them where needed since it's not in my code taste.
| case DiscType.GameCubeGameDisc: | ||
| case DiscType.GDROM: | ||
| lbl_Status.Content = string.Format("{0} discs are partially supported by DIC", Converters.DiscTypeToString(selectedDiscType)); | ||
| btn_StartStop.IsEnabled = (_drives.Count > 0 ? true : false); |
There was a problem hiding this comment.
Wouldn't the DiscTypeToString calls return the same as discTypeTuple.Item1? That would reduce the number of method calls here.
There was a problem hiding this comment.
Yes, that's a side effect of the refactor of the top part for the new management, just haven't modified it, but this discTypeTuple.Item1 is so unreadable, damn unnamed tuples.
MainWindow.xaml.cs
Outdated
| string readspeed = Regex.Match(output.Substring(index), @"ReadSpeedMaximum: [0-9]+KB/sec \(([0-9]*)x\)").Groups[1].Value; | ||
| if (!Int32.TryParse(readspeed, out int speed)) | ||
| int speed; | ||
| if (!Int32.TryParse(readspeed, out speed)) |
There was a problem hiding this comment.
Ditto here with the inlining
MainWindow.xaml.cs
Outdated
| { | ||
| var driveTuple = cmb_DriveLetter.SelectedItem as Tuple<char, string, bool>; | ||
| var discTuple = cmb_DiscType.SelectedItem as Tuple<string, KnownSystem?, DiscType?>; | ||
| var discTuple = cmb_SystemType.SelectedItem as Tuple<string, KnownSystem?, DiscType?>; |
There was a problem hiding this comment.
This one needs to be Tuple<string, KnownSystem?>
There was a problem hiding this comment.
Actually we don't need to fetch anything again here since we already have selectedSystem and selectedDiscType from above, I'll enhance accordingly.
There was a problem hiding this comment.
This would still be incorrect as cmb_SystemType.SelectedItem is a Tuple<string, KnownSystem?> and this would return null always.
There was a problem hiding this comment.
yes, I was actually talking about something similar in EnsureDiscInformation() method, here we need to fetch them.
Utilities/DumpInformation.cs
Outdated
| mappings[Template.SaturnHeaderField] = GetSaturnHeader(GetFirstTrack(outputDirectory, outputFilename)).ToString(); | ||
| if (GetSaturnBuildInfo(mappings[Template.SaturnHeaderField], out string serial, out string version, out string buildDate)) | ||
| string serial, version, buildDate; | ||
| if (GetSaturnBuildInfo(mappings[Template.SaturnHeaderField], out serial, out version, out buildDate)) |
There was a problem hiding this comment.
Ditto here with the inlining
Utilities/DumpInformation.cs
Outdated
| case KnownSystem.MicrosoftXBOX360XDG3: | ||
| if (GetXBOXAuxInfo(combinedBase + "_disc.txt", out string dmihash, out string pfihash, out string sshash, out string ss)) | ||
| string dmihash, pfihash, sshash, ss; | ||
| if (GetXBOXAuxInfo(combinedBase + "_disc.txt", out dmihash, out pfihash, out sshash, out ss)) |
There was a problem hiding this comment.
Ditto here with the inlining
Utilities/Validation.cs
Outdated
| { | ||
| if (!Int32.TryParse(parameter, out int temp)) | ||
| int temp; | ||
| if (!Int32.TryParse(parameter, out temp)) |
There was a problem hiding this comment.
Ditto here with the inlining
|
Ok, I should have pushed all relevant issues and tweaks. |
| .OrderBy(t => t.Item1) | ||
| .ToList(); | ||
|
|
||
| } |
There was a problem hiding this comment.
Nit: Remove any extra space in the method. Not functional, just a style thing.
mnadareski
left a comment
mnadareski
left a comment
There was a problem hiding this comment.
Looks good, can't wait to test this out!
2 participants
As proposed and discussed before this PR separates choosing a system from choosing a disk type in the UI so that it is managed by two different combo boxes.
Fixes #51