GrandOrgue / grandorgue

GrandOrgue software
Other
179 stars 43 forks source link

Introduced a new type GOBool3 for working with three-valued booleans #1779

Closed oleg68 closed 10 months ago

oleg68 commented 10 months ago

GrandOrgue has a lot of code that works with three-valued booleans. Now they are stored as integers that causes

This PR introduces a new type GOBool3 for incapsulating this concept and reusing it's code.

I'm going to use this type (in future PR's) at least in

Now the new type is used only in GOConfigReader.cpp.

No GO behavior should be changed.

larspalo commented 10 months ago

@oleg68 Did you intend to use this for all -1, 0 and 1 key values, like GCState and IsTremulant types, or only things like m_IgnorePitch and other GO (internal) settings?

Reason I'm asking is that I feel that BOOL3_UNDEF (undefined) is not really describing the intention of the -1 (ignore, don't touch, leave alone type of action) well enough in cases like GCState and IsTremulant. For me undefined means something uncertain, not known or something open to interpretation. -1 of GCState is for instance clearly defined.

For the settings where inheritance can occur (organ/windchest/rank/pipe) if a certain level has no own definition but rely on parent(s), I do understand the undefined type of value.

oleg68 commented 10 months ago

@oleg68 Did you intend to use this for all -1, 0 and 1 key values, like GCState and IsTremulant types, or only things like m_IgnorePitch and other GO (internal) settings?

I think it would be good to use the new type everewhere when Grandorgue use the three-valued bolleans, including GCState and IsTremulant. But it won't be in the same PR.

Reason I'm asking is that I feel that BOOL3_UNDEF (undefined) is not really describing the intention of the -1 (ignore, don't touch, leave alone type of action) well enough in cases like GCState and IsTremulant. For me undefined means something uncertain, not known or something open to interpretation. -1 of GCState is for instance clearly defined.

Would be BOOL3_DEFAULT better?

For the settings where inheritance can occur (organ/windchest/rank/pipe) if a certain level has no own definition but rely on parent(s), I do understand the undefined type of value.

Yes, you are rite. For the organ settings booleans BOOL3_UNDEF means Parent default.

larspalo commented 10 months ago

Would be BOOL3_DEFAULT better?

Yes, I think it better covers more use cases than BOOL3_UNDEF. Default at least implies a selection (automatically) done without a specific choice.

For GCState -1 is "no change" and for IsTremulant -1 is "not affected". How that relates to "default" is perhaps a long shot, but it's better than "undefined", I think. In the settings, the Parent default -1 is "not set" for that instance. The common denominator seems to be "no", but for a bool value "no" is closer to false...

I'll agree to BOOL3_DEFAULT if no one comes up with a better version.

oleg68 commented 10 months ago

@larspalo I renamed to BOOL3_DEFAULT

oleg68 commented 10 months ago

@rousseldenis could you approve this PR?