[Scummvm-tracker] [ScummVM :: Bugs] #16211: ALL: The new platform options seems to break the parsing of other options

[ScummVM :: Bugs]

#16211: ALL: The new platform options seems to break the parsing of other options
-------------------------+---------------------
Reporter:  eriktorbjorn  |       Owner:  (none)
    Type:  defect        |      Status:  new
Priority:  normal        |   Component:  Common
 Version:                |  Resolution:
Keywords:                |        Game:
-------------------------+---------------------
Description changed by eriktorbjorn:

Old description:

> This appears to be a regression from
> https://github.com/scummvm/scummvm/pull/6547
>
> As I understand it, GUI options are stored in two ways. There is the
> human readable one in scummvm.ini, and the binary one produced by
> `parseGameGUIOptions()`, which produces a string which we then usually
> calls `contains()` in to see if it has a particular option.
>
> The options are defined in gui_options.h, with values from `GUIO_NONE`
> ("\x00") through `GUIO_NOLANG` ("\x33") for global options, and the game
> options `GUIO_GAMEOPTIONS1` ("\xe0") through `GUIO_GAMEOPTIONS32`
> ("\xff").
>
> And then there is `GUIO_PLATFORM_PREFIX` ("\x40"), followed by a platform
> code from `kPlatformApple2GS_VAL` ("\x01") through
> `kPlatformNintendoSwitch_VAL` ("\x27"). So now a single byte can mean
> more than one thing?
>
> At the very least, this breaks the music device setting when adding a new
> SCI game. E.g. if I add Space Quest III, it only offers me "<default>",
> "No music", and "AdLib emulator". That's because it does this to
> determine if the game explicitly specifies MIDI options:
>
> {{{
>         const Common::String allFlags =
> MidiDriver::musicType2GUIO((uint32)-1);
>         bool hasMidiDefined = (strpbrk(_guioptions.c_str(),
> allFlags.c_str()) != nullptr);
> }}}
>
> But I could easily see it break any code that calls
> `parseGameGUIOptions()` and then uses `contains()` on it. I just haven't
> tried to verify it.

New description:

 This appears to be a regression from
 https://github.com/scummvm/scummvm/pull/6773

 As I understand it, GUI options are stored in two ways. There is the human
 readable one in scummvm.ini, and the binary one produced by
 `parseGameGUIOptions()`, which produces a string which we then usually
 calls `contains()` in to see if it has a particular option.

 The options are defined in gui_options.h, with values from `GUIO_NONE`
 ("\x00") through `GUIO_NOLANG` ("\x33") for global options, and the game
 options `GUIO_GAMEOPTIONS1` ("\xe0") through `GUIO_GAMEOPTIONS32`
 ("\xff").

 And then there is `GUIO_PLATFORM_PREFIX` ("\x40"), followed by a platform
 code from `kPlatformApple2GS_VAL` ("\x01") through
 `kPlatformNintendoSwitch_VAL` ("\x27"). So now a single byte can mean more
 than one thing?

 At the very least, this breaks the music device setting when adding a new
 SCI game. E.g. if I add Space Quest III, it only offers me "<default>",
 "No music", and "AdLib emulator". That's because it does this to determine
 if the game explicitly specifies MIDI options:

 {{{
         const Common::String allFlags =
 MidiDriver::musicType2GUIO((uint32)-1);
         bool hasMidiDefined = (strpbrk(_guioptions.c_str(),
 allFlags.c_str()) != nullptr);
 }}}

 But I could easily see it break any code that calls
 `parseGameGUIOptions()` and then uses `contains()` on it. I just haven't
 tried to verify it.

--
-- 
Ticket URL: <https://bugs.scummvm.org/ticket/16211#comment:9>
ScummVM :: Bugs <https://bugs.scummvm.org>
ScummVM