search

phetsims / molecule-shapes

"Molecule Shapes" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/molecule-shapes
GNU General Public License v3.0
5 stars 6 forks source link

Is the 'Show Lone Pairs' checkbox working correctly? #201

Closed Nancy-Salpepi closed 2 years ago

Nancy-Salpepi commented 3 years ago

Test device MacBook Air (m1 chip)

Operating System 12.0.1

Browser Chrome

Problem description https://github.com/phetsims/qa/issues/745

On the Model Screen, if I uncheck the Show Lone Pairs checkbox and then add another bond, it automatically checks that box again. Is this correct behavior? I noticed that if you add a lone pair, uncheck the box and then add another bond, the box remains unchecked.

Steps to reproduce

  1. On the Model Screen, uncheck 'Show Lone Pairs'
  2. Add a bond --the box becomes checked
  3. Add a lone pair and then uncheck 'Show Lone Pairs'
  4. Add a bond--the box remains unchecked

Visuals showlonepairs

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Molecule Shapes‬ URL: https://phet-dev.colorado.edu/html/molecule-shapes/1.4.0-rc.1/phet/molecule-shapes_all_phet.html Version: 1.4.0-rc.1 2021-11-24 02:29:02 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/96.0.4664.55 Safari/537.36 Language: en-US Window: 1419x690 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: 31 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 80) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
arouinfar commented 3 years ago

Good catch @Nancy-Salpepi.

In the latest published version (1.2.10) the Show Lone Pairs checkbox is disabled when there aren't any lone pairs attached to the atom in the play area. The buggy-looking behavior we're seeing now is a result of a change request I made in #194.

The outer lone pairs are now hooked up to the Show Lone Pairs checkbox, but that lead to an odd situation:

The "Show Lone Pairs" checkbox is only enabled for molecules that have lone pairs on the central atom. This can lead to odd situations where there are lone pairs in the molecule, but the checkbox is disabled (such as CO2 or BF3).

I requested a change to always leave the Show Lone Pairs checkbox enabled, regardless of whether or not the central atom has a lone pair. This works out really well for the Real Molecules screen, but the behavior is really odd on the Model screen.

@jonathanolson is it possible to treat the checkbox differently on the two screens? Basically revert to the 1.2 behavior for the Model screen, but leave things as-is on the Real Molecules screen.

jonathanolson commented 2 years ago

I'm really not sure what the desired behavior here is, or what "1.2" behavior should be? I have code comments in ModelMoleculesModel that says:

// when the molecule is made empty, make sure to show lone pairs again (will allow us to drag out new ones)

This seems independent of whether the checkbox is enabled, and it seems independent of https://github.com/phetsims/molecule-shapes/issues/194 (which was mainly for the outer lone pairs, which only seem tangentially related to this issue).

So, what should the behavior of the checkbox be, especially related to:

  1. When should the checkbox be enabled?
  2. When should the checkbox value be changed when not from user input?
  3. Should the "add lone pair" part be disabled when the checkbox is not checked?
arouinfar commented 2 years ago

@jonathanolson I've played with this a bit more, and I think what's happening is that every time the molecule is modified, the sim is checking whether or not the molecule has a lone pair. If it doesn't have a lone pair, it will turn on the checkbox if it was unchecked.

When should the checkbox be enabled?

I think it makes sense for the checkbox to always be enabled. In situations where the molecule contains no lone pairs, the checkbox would appear to do nothing, but I think that's okay. This behavior would be a consistent experience with Real Molecules screen.

When should the checkbox value be changed when not from user input?

The checkbox value should only be changed by user input. Changing its value automatically is poor UX.

Should the "add lone pair" part be disabled when the checkbox is not checked?

The current behavior is to disable the "add lone pair" element when the checkbox is not checked, and I would keep this behavior. I think this provides a better UX than always allowing lone pairs to be added since adding lone pairs when they are invisible could be confusing.

jonathanolson commented 2 years ago

I believe I've implemented this, can you verify? Now the Property shouldn't change without the user trying to change it (checkbox or reset-all or phet-io).

arouinfar commented 2 years ago

Thanks @jonathanolson. Looks good in https://github.com/phetsims/qa/issues/768.