meterNode visibleProperty for model and view causing buggy behavior

[matthew-blackman]

For the ammeter and voltmeter nodes, there are two visibleProperties: both view and model.

model visibleProperty: false puts the tool back into the toolbox. visibleProperty: true restores the tool back to its previous position in the play area. This is consistent with other sims.

@arouinfar @samreid and I agree that we should rename this isActiveProperty (follows Projectile Motion and MSAB pattern) and uninstrument the view visibleProperty for meters.

[AgustinVallejo]

Will take a look!

[matthew-blackman]

Note: Review model.meters phet io overrides after this issue is complete. We will want to feature activeProperty on the meters.

[matthew-blackman]

@arouinfar this is ready for us to review in the tree

[samreid]

I reviewed the commits and tested in studio. Everything seems great here. Sounds like @arouinfar and @matthew-blackman may need to check on the tree and possibly phet-io-overrides. Reassigning.

[arouinfar]

Something's not quite right with isActiveProperty. If both voltmeters are active in the play area, the "Voltmeter" label is hidden in the toolbox. This a change from the published version, and looks a bit odd.

Master	Latest Published Version

What's stranger is that completely removing all series ammeters leaves behind a ghost, similar to the strategy we employ for carousel. This creates an inconsistent appearance:

Back to @matthew-blackman.

[samreid]

Before deciding how to proceed for this issue, I'd like to discuss part of the implementation and design. It's nice to reuse the circuit element tool code for the ammeter, but it has the default behavior of leaving behind the ghost. If we liked having the ghost in the carousel, would we also like to have ghosts in the sensor toolbox? I'm not sure what will be easier--unghosting the series ammeter or ghosting the other element. But thought I should ask before starting. Or should we just implement whatever seems easier? Or is it OK that they are inconsistent, because one is a circuit element and the other is not?

It seems in either case we would need to fix the voltmeters text. So perhaps I'll start there.

[samreid]

It was straightforward to zero out the series ammeter ghost transparency. So I did that, and I tried to retain the text when all items are dragged out (but not when tools are made invisible from studio). Most things seem to be good, but there may be a corner case or two that is not consistent? @arouinfar or @matthew-blackman can you please help test?

[arouinfar]

It was straightforward to zero out the series ammeter ghost transparency. So I did that, and I tried to retain the text when all items are dragged out (but not when tools are made invisible from studio).

Great! While the ghosting looks nice for the carousel, it seems odd in the toolbox. Overall, things are looking good.

Most things seem to be good, but there may be a corner case or two that is not consistent?

There's some strange behavior with dynamic layout that I don't love because it exposes resizing panels to the users.

• On the Intro Screen, take both non-contact ammeters meters out of the toolbox, and the toolbox will get shorter.
• On the Lab Screen, take both non-contact ammeters out of the toolbox. The series ammeter will recenter over the "Ammeter" label and the entire panel will get narrower. The same does not happen when series ammeters are all in the play area and there is still a non-contact ammeter left in the toolbox.

Additionally, using isActiveProperty to remove the voltmeters from the toolbox looks odd. Everything ends up centered in the play area, including the probes.

It would be preferable if the probes could look like this:

[samreid]

Thanks, I was able to reproduce those 3 errant behaviors and correct them with the commits. Can you please test/verify?

[arouinfar]

Thanks @samreid, looks good!