Skip to content
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

Should keyboard dialog include play/pause command? #194

Closed
KatieWoe opened this issue Jul 18, 2022 · 19 comments
Closed

Should keyboard dialog include play/pause command? #194

KatieWoe opened this issue Jul 18, 2022 · 19 comments
Assignees

Comments

@KatieWoe
Copy link
Contributor

Test device
MacBook Pro
Operating System
MacOS 12.4
Browser
Chrome
Problem description
For phetsims/qa#819, related to #169.
The opt/alt + k command for the play/pause button works in the sim, but is not currently included in the keyboard nav dialog.

Troubleshooting information:

!!!!! DO NOT EDIT !!!!!
Name: ‪Greenhouse Effect‬
URL: https://phet-dev.colorado.edu/html/greenhouse-effect/1.1.0-dev.10/phet/greenhouse-effect_all_phet.html
Version: 1.1.0-dev.10 2022-07-08 17:41:59 UTC
Features missing: applicationcache, applicationcache, touch
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/103.0.0.0 Safari/537.36
Language: en-US
Window: 1469x861
Pixel Ratio: 2/1
WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium)
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium)
Vendor: WebKit (WebKit WebGL)
Vertex: attribs: 16 varying: 15 uniform: 1024
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32)
Max viewport: 16384x16384
OES_texture_float: true
Dependencies JSON: {}

@arouinfar
Copy link
Contributor

Thanks @KatieWoe. This seems like an oversight to me.

I think Molecules and Light is the only sim that currently uses these hotkeys. Here's the relevant snippet from its keyboard help dialog.

image

In MAL, the TimeControlNode is described within the context of the Observation Window. Given that the TimeControlNode can have huge impact on the description of the Observation Window, this makes sense.

However, we are adding support for alternative input to many sims, so I think we should consider adding optional content to the Basic Actions section. The help content for StepForwardButton is already very general, but we should change the help content for PlayPauseButton to something like:

  • Pause or play action in sim [Alt] + [K] (we could drop "in sim" if it sounds too odd)

For Greenhouse Effect, I would include the PlayPauseButton help content under the Basic Actions.

@jessegreenberg @jbphet let me know your thoughts. If you think it's worth generalizing the PlayPauseButton and StepForwardButton help content, we can move this discussion to scenery-phet (or the appropriate common repo).

@arouinfar
Copy link
Contributor

Actually, I just realized Alt+L is used to increase the standard size of the StepForwardButton. I don't know if that will become standard practice, so we might decide to skip generalizing that part to Basic Actions.

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Jul 27, 2022

I think hotkeys are typically only going to be used by superusers and are too complicated for the "Basic Actions" section. My preference would be to add a new section for this.

@jessegreenberg
Copy link
Contributor

Discussed today - Ill make a new section with default content for the "timing controls". For this sim, Ill place it under the "slider" section.

@jbphet
Copy link
Contributor

jbphet commented Jul 27, 2022

We discussed this in the 7/27/2022 design meeting and concluded that interviews can go ahead without this functionality. @arouinfar said she would let participants know verbally if they appear to be searching for this functionality.

@jessegreenberg
Copy link
Contributor

OK, an initial commit for this has been made. Strings are not translatable yet until they have been reviewed. @arouinfar would you mind taking a look and verifying the default content looks OK? Any layout changes or anything else? A "Timing Controls" section can be found on every screen (except micro and home).

@jbphet
Copy link
Contributor

jbphet commented Aug 3, 2022

The design team reviewed this in today's meeting, and @arouinfar approved, so we should move ahead with making this string translatable and then we're done.

@jbphet jbphet assigned jessegreenberg and unassigned arouinfar Aug 3, 2022
@jessegreenberg
Copy link
Contributor

In addition to the final cleanup above I noticed a layout problem
image

I thought that all icons in a column would be left aligned.

@jessegreenberg
Copy link
Contributor

Addressed above.

jessegreenberg added a commit to phetsims/scenery-phet that referenced this issue Aug 3, 2022
@jessegreenberg
Copy link
Contributor

OK, all done here. TypeScript is cleaned up and the strings are now translatable. Closing.

@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 4, 2022

Reopening. What are "timing controls"? If this is at all related to TimeControlNode, then...

(1) The terminology here is very confusing. We typically refer to "time controls", not "timing controls". Has this been cleared with designers? Is is OK to use "timing control" in the UI and "time control" in PhET-iO tandems?
(2) We've once again named a keyboard-help section (TimingControlsKeyboardHelpSection) differently than what it's related to (TimeControlNode). One of them needs to change.

Also, the doc for TimingControlsKeyboardHelpSection is currently:

 * Help section that explains how to use a keyboard to toggle play/pause and timing controls.

It would be helpful to be specific about what "timing controls" is.

@pixelzoom pixelzoom reopened this Aug 4, 2022
@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 4, 2022

It also seems like effort would be better spent on addressing phetsims/scenery-phet#682 (Hotkey support for TimeControlNode). In Fourier phetsims/fourier-making-waves#92, we decided not to enable TimeControlNode hotkeys because they did not support all of the functionality that's exposed in that sim -- there is no hotkey for Step. Two screens in Greenhouse also have a Step button. Why are we adding hotkeys and keyboard-help content to Greenhouse when it has the exact same problem as Fourier?

@jessegreenberg
Copy link
Contributor

It will be a week or so until I can return to this issue so I removed the strings from scenery-phet-strings_en.json until we know they are solid.

@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 4, 2022

So it sounds like "Timing" is related to TimeControlNode.

If "Timing" is the indeed the new terminology, then there are many things that need to change, in all sims. And it changes the PhET-iO API of all sims.

  • Rename TimeControlNode to TimingControlsNode
  • Rename all subclasses of TimeControlNode, eg:
- class CollisionLabTimeControlNode extends TimeControlNode {
+ class CollisionLabTimingControlsNode extends TimingControlsNode {
  • Rename all variables and tandems for instances of TimeControlNode. Note that this changes the PhET-iO API of all sims that use TimingControlsNode. Eg:
-    const timeControlNode = new LayersModelTimeControlNode( model, {
-      tandem: tandem.createTandem( 'timeControlNode' )
+     const timingControlsNode = new LayersModelTimingControlsNode( model, {
+      tandem: tandem.createTandem( 'timingControlslNode' )

If "Timing" is not essential, and we can continue to use "Time", then the changes are much easier:

  • Rename TimingControlsKeyboardHelpSection to TimeControlKeyboardHelpSection
  • Change the string from "Timing Controls" to "Time Controls"
  • Change the string key from sceneryPhetStrings.keyboardHelpDialog.timingControls.timingControls to sceneryPhetStrings.keyboardHelpDialog.timeControl.timeControls

I guess we should techncally change TimeControlNode to TimeControlsNode (plural), but that might be something we could skip.

@pixelzoom
Copy link
Contributor

pixelzoom commented Aug 4, 2022

I'd also be interested in hearing from @arouinfar on this issue, since she commented in #194 (comment). Fourier and Greenhouse both have identical uses of TimeControlNode, and identical problems -- no hotkey for the Step button. Why did we decide to disable TimeControlNode hotkeys in Fourier, but enable them in Greenhouse? Is it because Greenhouse includes description?

(Btw... There's no responsible designer listed for this sim.)

EDIT: Note that hotkeys for TimeControlNode are also currently disabled in MOTHA, for the same reason as Fourier.

@jbphet
Copy link
Contributor

jbphet commented Sep 21, 2022

The Greenhouse Effect design team (AR,KP,JG,JB,MN,MM) discussed the naming of the controls, tandems, and what is presented in the keyboard help dialog in our meeting today. Here are some notes.

Above, @pixelzoom said:

(1) The terminology here is very confusing. We typically refer to "time controls", not "timing controls". Has this been cleared with designers? Is it OK to use "timing control" in the UI and "time control" in PhET-iO tandems?

@arouinfar and @kathy-phet said yes to both of the questions above.

So, the only thing that we believe needs to change is the keyboard help section in the code from TimingControlsKeyboardHelpSection to TimeControlKeyboardHelpSection so that the code is internally consistent. @jessegreenberg will take care of this.

We also briefly looked at phetsims/scenery-phet#682, and it will be worked on hotkey support as Greenhouse Effect develops.

@jbphet jbphet changed the title Should keyboard dialog include play/pause command Should keyboard dialog include play/pause command? Jan 13, 2023
@jbphet
Copy link
Contributor

jbphet commented Jan 13, 2023

The inability to translate these strings was reported during the testing for the 1.1 version of Greenhouse Effect, see #257.

@jessegreenberg
Copy link
Contributor

Making the strings translatable was done in #257. The remaining component of #194 (comment) was the file rename, Ill do that now.

@jessegreenberg
Copy link
Contributor

Done. Closing.

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

No branches or pull requests

5 participants