There should be a way within the sim to set the default temperature units

[jbphet]

Currently the sim defaults to Kelvin, and we should probably have a way within the sim to set the default units to something else, and that setting should still be respected after a reset. Issue #72 will be related.

[jbphet]

At this point, we're not sure if this will be in an "Options..." dialog or a "Preferences" dialog. It may hinge upon whether we have extra sounds. I'll defer this for now, and once things are further along we can add the appropriate item to the appropriate dialog.

[jbphet]

This was discussed in the 12/14/2022 meeting, and we decided that yes, it would be good to have this. It is not blocking for the publication of the prototype for #220. Undeferring and assigning to myself.

[jbphet]

This has now been implemented. @arouinfar - please review the behavior on master and see if you approve. Please assign back to me when done, since I'd like to leave it open and have QA check it on the next release.

I have two specific questions about this feature:

• I made the units change in the sim when the selected default is changed from the Preferences menu. It felt weird to me when it didn't to this, but it could lead to a slightly odd scenario where the user intentionally changes to one units value in a screen, then changes the default via the preferences menu to something different, and their previous intentional non-default selection changes. This seems like an unlikely use case, and not particularly surprising even if it does occur, so I opted for changing the units selection everywhere when the default changes. Are you good with this?
• Do you think we should make the query parameter for the default units tolerant of lower case values? For example, should it accept "k" as well as "K"?

The tab in the "Preferences" dialog where this can be set looks like this:

[arouinfar]

please review the behavior on master and see if you approve.

Behavior on master looks good, but the casing of some of the tandem names looks off. Can you please use camel casing for the name of the control defaultTemperatureUnitsSelector as well as the individual radio buttons, e.g. celsiusRadioButton?

I made the units change in the sim when the selected default is changed from the Preferences menu. It felt weird to me when it didn't to this

This feels really natural to me. If someone is taking the time to change the default temperature units in the Preferences dialog, it's not a huge leap to assume they prefer to work in that unit.

Do you think we should make the query parameter for the default units tolerant of lower case values? For example, should it accept "k" as well as "K"?

Currently, the accepted values are uppercase which naturally corresponds to how the units are displayed in the sim. This seemingly violates the norms of query parameters (all camel casing). I think it would be a nice touch to accept both k and K for Kelvin, but I probably would only advertise the lowercase version since that follows the pattern used elsewhere across sims.

[arouinfar]

While reviewing dynamic layout, I noticed the Preferences dialog can get really wide. It seems like the Default Temperature Units control may need a narrower maxWidth.

The content in the other tabs is width-limited, so I think the Simulation tab should match. Here's the Visual tab for comparison.

[pixelzoom]

maxWidth was not set for "Default Temperature Units". That has been fixed in the above commit. @arouinfar please review in master, close if OK.

[arouinfar]

Thanks @pixelzoom the maxWidth looks good in master. There are some additional comments for @jbphet in #187 (comment), so I'll reassign back to him.

[pixelzoom]

... There are some additional comments for @jbphet in #187 (comment), so I'll reassign back to him.

From #187 (comment):

Behavior on master looks good, but the casing of some of the tandem names looks off. Can you please use camel casing for the name of the control defaultTemperatureUnitsSelector as well as the individual radio buttons, e.g. celsiusRadioButton?

Done in the above commit. Here's the Studio tree:

I think it would be a nice touch to accept both k and K for Kelvin, but I probably would only advertise the lowercase version since that follows the pattern used elsewhere across sims.

The defaultTemperatureUnits query parameter currently accepts value 'K', 'C', 'F'. There is no requirement/convention that query parameter values need to begin with a lowercase letter. Uppercase feels correct/natural to me here -- it's what I would try without consulting documentation. I don't think it needs to support both uppercase and lowercase, and I don't think that supporting lowercase only would be an improvement.

Back to @arouinfar to review. Let me know if I missed anything else in this issue.

[arouinfar]

There is no requirement/convention that query parameter values need to begin with a lowercase letter.

I didn't realize this, thanks for letting me know @pixelzoom. In that case, let's continue with just the uppercase values since they are the more natural choice.

Everything looks good, closing.