New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot select Pravets 82 #415

Closed
audetto opened this Issue May 5, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@audetto
Contributor

audetto commented May 5, 2017

A colleague of mine actually used a Pravets 82 so I wanted to show him AppleWin emulating the Pravets 82, but it cannot be selected, giving this error

"A specific clone wasn't selected from the Advanced tab"

The other clones work.

The reason is in this commit 749e151 for Common.h

This is the enum eApple2Type

			// Clones start here:
				A2TYPE_CLONE=APPLECLONE_MASK,
				A2TYPE_PRAVETS=APPLECLONE_MASK,
				A2TYPE_PRAVETS82=A2TYPE_PRAVETS,	// Apple ][ clone
				A2TYPE_PRAVETS8M,			// Apple ][ clone

You can see that the value of Pravets82 is eventually the same as A2TYPE_CLONE which is forbidden in CPropertySheetHelper::PostMsgAfterClose.

The correct code should look like (I think)

			// Clones start here:
				A2TYPE_CLONE=APPLECLONE_MASK,
				A2TYPE_PRAVETS=APPLECLONE_MASK,
				A2TYPE_PRAVETS82,		// Apple ][ clone
				A2TYPE_PRAVETS8M,		// Apple ][ clone

Unless you are worried that these values are persisted and should not be changed, in which case the Pravets82 should go at the end and get a new number.

@tomcw

This comment has been minimized.

Show comment
Hide comment
@tomcw

tomcw May 5, 2017

Contributor

Thanks for the report & triage.

The enum A2TYPE_PRAVETS82=A2TYPE_PRAVETS (=A2TYPE_CLONE) was added on Apr 12 2016 in 749e151 (as you point out above), so in release v1.26.0.3 (19 Sept 2016).

And the check for A2TYPE_CLONE in CPropertySheetHelper::PostMsgAfterClose() was added on Oct 22 2016 in 2d61c75, so first got released in v1.26.1.1 (17 Feb 2017).

Unless you are worried that these values are persisted and should not be changed, in which case the Pravets82 should go at the end and get a new number.

Yes, this sounds like a reasonable solution.

NB1. A Registry key "Apple2 Type" with value of 256 (A2TYPE_CLONE) is allowed to be read on startup in LoadConfiguration(). With AppleWin 1.26.1.1+ there's no way that this could have been generated, since the UI forbids this. So could only have been generated by AppleWin v1.26.0.3 - 1.26.1.0 (inclusive).

If we change the enum, then we must trap this 256 (A2TYPE_CLONE) at start-up, and convert it to the correct (new) value of A2TYPE_PRAVETS82.

NB2. The save-state persists the "Model" using a string, eg. "Pravets82", so it is not affected. But when (a model=Pravets82) is loaded then currently will write 256 to the Registry. If we change the enum, then it will write out the correct value.

So given these points, it still seems reasonable to just move Pravet82 to the end of the enum definition.

Contributor

tomcw commented May 5, 2017

Thanks for the report & triage.

The enum A2TYPE_PRAVETS82=A2TYPE_PRAVETS (=A2TYPE_CLONE) was added on Apr 12 2016 in 749e151 (as you point out above), so in release v1.26.0.3 (19 Sept 2016).

And the check for A2TYPE_CLONE in CPropertySheetHelper::PostMsgAfterClose() was added on Oct 22 2016 in 2d61c75, so first got released in v1.26.1.1 (17 Feb 2017).

Unless you are worried that these values are persisted and should not be changed, in which case the Pravets82 should go at the end and get a new number.

Yes, this sounds like a reasonable solution.

NB1. A Registry key "Apple2 Type" with value of 256 (A2TYPE_CLONE) is allowed to be read on startup in LoadConfiguration(). With AppleWin 1.26.1.1+ there's no way that this could have been generated, since the UI forbids this. So could only have been generated by AppleWin v1.26.0.3 - 1.26.1.0 (inclusive).

If we change the enum, then we must trap this 256 (A2TYPE_CLONE) at start-up, and convert it to the correct (new) value of A2TYPE_PRAVETS82.

NB2. The save-state persists the "Model" using a string, eg. "Pravets82", so it is not affected. But when (a model=Pravets82) is loaded then currently will write 256 to the Registry. If we change the enum, then it will write out the correct value.

So given these points, it still seems reasonable to just move Pravet82 to the end of the enum definition.

@tomcw tomcw self-assigned this May 5, 2017

@tomcw tomcw added 1.26.2.3 bug labels May 5, 2017

@tomcw tomcw added this to the 1.27 milestone May 5, 2017

@tomcw

This comment has been minimized.

Show comment
Hide comment
@tomcw

tomcw May 7, 2017

Contributor

Actually there is a gap in the eApple2Type enums after A2TYPE_PRAVETS8M where it then jumps to the //e clones, so I'll just put A2TYPE_PRAVETS82 immediately after A2TYPE_PRAVETS8M.

Possible compatibility scenarios:

  1. New AppleWin loads "Apple2 Type" of 256 from Registry:
  • Gets automatically converted to the new A2TYPE_PRAVETS82 value (258)
  • Update Registry with this new value
  • Display MessageBox showing that this Apple2 type conversion has happened
  • Optionally log this message (if -log is enabled). NB. Not hugely helpful unless you always run with logging enabled (since a re-run won't catch this in the log file, as the Apple2 type in the Registry will have been updated).
  1. Old AppleWin loads "Apple2 Type" of 258 from Registry:
  • LoadConfiguration() lets this through as a valid value (since it's less than A2TYPE_MAX... the validation isn't perfect!)
  • But in MemInitializeROM() the g_Apple2Type doesn't match, so a dialog will pop up saying:
  • Unable to open the required firmware ROM data file. File: Unknown type!
  • AppleWin will exit, but will have reset the Registry value to A2TYPE_APPLE2EENHANCED, so on the next launch of AppleWin it'll work fine, but using the default Enhanced //e.
Contributor

tomcw commented May 7, 2017

Actually there is a gap in the eApple2Type enums after A2TYPE_PRAVETS8M where it then jumps to the //e clones, so I'll just put A2TYPE_PRAVETS82 immediately after A2TYPE_PRAVETS8M.

Possible compatibility scenarios:

  1. New AppleWin loads "Apple2 Type" of 256 from Registry:
  • Gets automatically converted to the new A2TYPE_PRAVETS82 value (258)
  • Update Registry with this new value
  • Display MessageBox showing that this Apple2 type conversion has happened
  • Optionally log this message (if -log is enabled). NB. Not hugely helpful unless you always run with logging enabled (since a re-run won't catch this in the log file, as the Apple2 type in the Registry will have been updated).
  1. Old AppleWin loads "Apple2 Type" of 258 from Registry:
  • LoadConfiguration() lets this through as a valid value (since it's less than A2TYPE_MAX... the validation isn't perfect!)
  • But in MemInitializeROM() the g_Apple2Type doesn't match, so a dialog will pop up saying:
  • Unable to open the required firmware ROM data file. File: Unknown type!
  • AppleWin will exit, but will have reset the Registry value to A2TYPE_APPLE2EENHANCED, so on the next launch of AppleWin it'll work fine, but using the default Enhanced //e.

@tomcw tomcw closed this in eb21f34 May 8, 2017

@tomcw

This comment has been minimized.

Show comment
Hide comment
@tomcw

tomcw May 8, 2017

Contributor

Tested by selecting all Apple2 types (regular & clones), exiting and then restarting AppleWin.

Contributor

tomcw commented May 8, 2017

Tested by selecting all Apple2 types (regular & clones), exiting and then restarting AppleWin.

@tomcw

This comment has been minimized.

Show comment
Hide comment
@tomcw

tomcw May 25, 2017

Contributor

I've pre-released a new AppleWin 1.26.2.4 version here that fixes this.

Contributor

tomcw commented May 25, 2017

I've pre-released a new AppleWin 1.26.2.4 version here that fixes this.

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