Skip to content

Conversation

@Jakz
Copy link
Collaborator

@Jakz Jakz commented Jul 9, 2018

This PR adds some customization to the DIC process, it adds 4 options which can be changed in specific window.

  • Fixed Unknown system which wasn't present anymore in combobox
  • Created new ViewModels file which should contain all the view models needed by the UI
  • Added Quiet mode option to disable DIC sounds
  • Added Paranoid mode option which should enable all pedantic flags
  • Added Disable Media Type Detection to disable detection at startup (which is not always needed and can slow down startup by a lot
  • Added configurable /c2 flag reread amount value

This PR is also a proof of concept about how binding between data and views should be done in WPF (so that we should follow this approach in general).

I left 2 TODOs, both relative to KnownSystemAndMediaTypeToParameters since paranoid flags and their tests should be handled by someone with a better knowledge of flag meanings (I choose you @mnadareski!)

@mnadareski mnadareski self-requested a review July 9, 2018 18:10
Copy link
Collaborator

@mnadareski mnadareski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple things, and make sure to add tests for the paranoid flags specifically 👍


List<KnownSystemComboBoxItem> comboBoxItems = new List<KnownSystemComboBoxItem>();

comboBoxItems.Add(new KnownSystemComboBoxItem(KnownSystem.NONE));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check for an "Unknown" entry under the first group as well, it seems like it might have been shifted to there originally, hence I thought it was "missing".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's not a real system so it shouldn't be returned from the list but added manually. This is fixed now.


if (paranoid)
{
parameters.Add(DICFlags.ScanFileProtect);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd claim that ScanFileProtect is something that should be done regardless.

/// <param name="type">MediaType value to check</param>
/// <returns>List of strings representing the parameters</returns>
public static List<string> KnownSystemAndMediaTypeToParameters(KnownSystem? sys, MediaType? type)
public static List<string> KnownSystemAndMediaTypeToParameters(KnownSystem? sys, MediaType? type, bool paranoid, uint RereadAmountC2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious nit: Why uint for the reread amount? Also, might want to have a case where -1 means remove the count entirely, since a blank count defaults to something like 4000 rereads

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I don't like to use special values to represent special cases, it's too much ancient C way of doing things. An amount of retries can't be negative, since it has no sense being negative so I used an uint but no problem in any case.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that's fine. Just wondering 👍

{
case MediaType.CD:
parameters.Add(DICFlags.C2Opcode); parameters.Add("20");
parameters.Add(DICFlags.C2Opcode); parameters.Add(RereadAmountC2 > 0 ? Convert.ToString(RereadAmountC2) : "20");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can have a "paranoid" case for all CDs as well, I'd like to add DICFlags.SubchannelReadLevel, "2", which is the slower but more accurate subchannel read-ahead.

return null;
}

//TODO: adjust parameters according to paranoid mode
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a paranoid one for DVD: DICFlags.CMI. That's the "Copyright Management Information" flag, and it is highly paranoid for DVDs. This counts for HD-DVD as well, since that's lumped under the DVD read flag.

@mnadareski mnadareski merged commit d51adcc into SabreTools:master Jul 9, 2018
@Jakz Jakz deleted the extra_options branch July 9, 2018 23:37
mnadareski added a commit that referenced this pull request Jul 10, 2018
* Update commented code

* Add Parameters class

This class is currently unused, but represents a set of parameters that can be passed to and from any given method. It has some copied/modified code from Validators.cs, for the time being due to the current overlap.

* Add better documentation

* Add and use DICFlag enumeration

* Port more things to Parameter-specific version

* Add commented code for later

* Added new options (#90)

* Split type combobox into system combobox and disc type combobox

* corrected indentation for xaml file

* fixed merge with head

* fixed format

* fixed issues for PR, added KnownSystem.CUSTOM

* removed Updater.cs which ended by error in commit

* fixed GetOuptutName() for new drive/system combobox

* added 4 new options: quiet mode, paranoid mode, disable media type detect and c2 reread amount

* added default C2 reread tries to config

* fixed issues for PR

* removed commented leftover

* Update commented code

* Add Parameters class

This class is currently unused, but represents a set of parameters that can be passed to and from any given method. It has some copied/modified code from Validators.cs, for the time being due to the current overlap.

* Add better documentation

* Add and use DICFlag enumeration

* Port more things to Parameter-specific version

* Add commented code for later

* Use Parameters object

* Fix dumping process

* Cleanup Validators
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants