phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
MIT License
8 stars 6 forks source link

Preferences button should appear in the navbar if there are any preference in addition to Overview. #868

Closed pixelzoom closed 1 year ago

pixelzoom commented 2 years ago

Discussed with @arouinfar in the context of pH Scale requirements...

If the Preferences dialog would have any tab in addition to Overview, then a Preferences button should appear in the navigation bar.

This is currently not the case. supportSounds: true is apparently not creating the Audio tab, and it should be doing so. There seemed to be an assumption that the Audio tab was not necessary in the case where it was the only additional tab, and that's not the case.

Since this is related to https://github.com/orgs/phetsims/projects/44, it blocks publication of any sims in that release that would have a Preference dialog according to the above requirements.

@jessegreenberg @zepumph if you have questions, please loop in @arouinfar.

zepumph commented 2 years ago

The current behavior was a specific design decision made by the preferences team when creating it. I was not involved in this, but it seems likely worth looping some of them in before deciding on anything. Over to lead dev @jessegreenberg.

arouinfar commented 2 years ago

There's an odd intersection when the only Preferences in a sim are sound and localization. The Localization tab appears only in the _all file. So if a sim has just sound and localization, the _en file would not have a Preferences dialog, but the _all file would. However, that Preferences dialog would contain tabs for both Audio and Localization. It seems strange within the same simulation to only include the Audio tab on the basis of the sim containing other languages, something that has nothing to do with Audio.

I think the use case for a sim only having supportsSound: true but no other Preferences is not particularly realistic. Alternative Input, Interactive Highlights, UI Sound, and Localization are all becoming standard features, so most sims will have Visual, Audio, and Localization tabs. Currently, the only published sim with sound as its only Preference is Waves Intro. When it gets republished off of master, it will very likely pick up alt input & interactive highlights support and localization. (It already has 90% of the alt input working, there were just a few custom tools that went beyond the scope of that release.)

jessegreenberg commented 2 years ago

Previously we decided not to include a Preferences dialog if sound was the only supported feature because it isn't necessary. Everything in the dialog is redundant with the "Audio" toggle button in the navigation bar and it has no other use in the UI.

So if a sim has just sound and localization, the _en file would not have a Preferences dialog, but the _all file would

That sounds right to me.

However, that Preferences dialog would contain tabs for both Audio and Localization. It seems strange within the same simulation to only include the Audio tab on the basis of the sim containing other languages, something that has nothing to do with Audio.

I don't find this strange. The dialog is included now because there are meaningful controls within it.

think the use case for a sim only having supportsSound: true but no other Preferences is not particularly realistic.

That may be, but for the few cases where sound is the only supported feature I don't see a need to always include it.

@arouinfar back to you, if you would still like to change it so that the dialog appears when sound is supported let me know.

arouinfar commented 2 years ago

Previously we decided not to include a Preferences dialog if sound was the only supported feature because it isn't necessary. Everything in the dialog is redundant with the "Audio" toggle button in the navigation bar and it has no other use in the UI.

By that logic, you could argue that the "Audio Features" toggle is pointless if it is the only control in the Audio tab. The navbar button is more convenient. The number of other tabs in the dialog doesn't change the utility of the "Audio Features" toggle, so why are we bothering to show that tab at all?

Also, I want to confirm if there is extra logic to detect if supportsExtraSound: true. If I'm not mistaken, Extra Sound is a subset of Sound, so we need to be careful to still create the dialog/tab so users can access the Extra Sounds checkbox. It seems like the current logic should be something like:

This logic seems unnecessarily complicated and it caused confusion about whether or not pH Scale should have a Preferences dialog/button when @pixelzoom added supportsSound: true to pH Scale. I won't insist we change the current pattern (so long as the Extra Sounds case is already accounted for), but developers should make sure the expectations are clearly documented to avoid situations like this in the future.

Assigning to @pixelzoom and @jessegreenberg to decide next steps.

zepumph commented 2 years ago

Why have we not gotten original designers in the conversation for this yet? Everything @arouinfar is saying may be the way we should go, but it isn't up to developers. The current behavior was the last designed spec. @emily-phet can you please confirm showing a preferences dialog with only an overview/audio tab for sound (not extra sound) is acceptable and desired as described by @arouinfar above?

emily-phet commented 2 years ago

The logic that @arouinfar describes above is consistent with my understanding of the decision process.

If supportsSound: true AND supportsExtraSound: false AND Overview is the only other tab in the dialog, then do not create a Preferences dialog/button.

I can elaborate on why that decision was made if needed, but the outcome is the same. Related, there has been a recent decision to include the interactive highlights feature along with alternative input in sims going forward. This would involve the addition of a visual tab to the preferences menu, meaning it would be rare in the future that a sim would be published without a preferences dialog - i.e., there would likely be a visual and an audio tab relevant for all future sims.

zepumph commented 2 years ago

@emily-phet, given the current implementation (and you continued recommended design), in the cases where there are only UI sound and multiple locales in the the _all html. Then You have the case where the _en version doesn't have a preferences dialog (since it would just be to toggle the general UI sound), but the _all html does have a preferences dialog with overview/audio/localization. Is this also acceptable?

emily-phet commented 2 years ago

It seems I'm not clear on the repercussions re localization tab. I have a message to you responding re meeting. Could we briefly discuss this tomorrow?

In general, I'm not opposed to sims going out with only the audio tab (only UI sounds). If that is an outcome of sims with localization features, I'm fine with that personally, but it hasn't been brought up specifically in the situation with localization. That tab has been a recent addition to the general preferences design.

zepumph commented 2 years ago

Sounds good. I'll report back here after our meeting tomorrow afternoon.

jessegreenberg commented 2 years ago

I just confirmed that if "extra sound" is supported the dialog becomes available. IMO it is well documented in code. If changes are needed feel free to reassign me!

```js /** * Returns true if this model supports any controllable preferences but MORE than basic sound. If enabling sound * is the only preference then we don't want to let the user access the Preferences dialog because there is already * a sound control in the navigation bar. The PreferencesDialog is not useful in that case. */ public shouldShowDialog(): boolean { ```
pixelzoom commented 2 years ago

I opened this issue because I noted a difference between requirements (what I was asked to do) and what was possible (the current implementation behavior). How to address that is not my call. This is up to designers - either the design requirements need to change to match the implementation, or visa versa.

Unassigning myself.

zepumph commented 2 years ago

@emily-phet and I discussed this further today, and we agree that it makes the most sense long term to add in a preferences dialog when any preferences occur. This means minor changes to when a preferences dialog is shown, and generally simpler code. I'll implement.

zepumph commented 2 years ago

@jessegreenberg, will you please review this and let me know if there is anything else here?

jessegreenberg commented 1 year ago

Fair enough, sounds good! Above commits look good, I just removed the documentation describing the previous design. Closing.