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

Uninstrument Text and RichText #339

Closed
arouinfar opened this issue Jul 21, 2023 · 12 comments
Closed

Uninstrument Text and RichText #339

arouinfar opened this issue Jul 21, 2023 · 12 comments

Comments

@arouinfar
Copy link

For #323

Issues identified in https://github.com/phetsims/studio/issues/303 lead to a new way to autoselect strings in Studio. As a result, the recommendation in https://github.com/phetsims/phet-io/issues/1941 is that most instances of text do not need to be instrumented.

@pixelzoom subsequently uninstrumented text in other sims like phetsims/molecule-polarity#160 and phetsims/graphing-quadratics#187. Seems like we should do the same in Natural Selection. Do you agree @pixelzoom?

@pixelzoom
Copy link
Contributor

Yes agreed.

@pixelzoom
Copy link
Contributor

In the above commits, I uninstrumented Text (there are no RichText) and updated the API file. The next step will be to add migration rules. Before I do that, I'd like @arouinfar to review in Studio. Please assign back to me after review.

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 22, 2023

All of these Text elements were apparently instrumented since the most recent (1.4) release, so they were never part of a published API. Therefore no migration rules are needed, as confirmed via the Migration wrapper. Great call on uninstrumenting these @arouinfar. Feel free to close this issue if everything looks good in Studio.

@arouinfar
Copy link
Author

Thanks @pixelzoom. I searched for "text" and found a few remaining instances:

  • view.dialogs.memoryLimitDialog.text
  • view.graphs.proportionsNode.proportionsGraphNode.generationSpinner.numberDisplay.valueText

Back to you @pixelzoom.

@arouinfar arouinfar assigned pixelzoom and unassigned arouinfar Jul 25, 2023
@pixelzoom
Copy link
Contributor

@arouinfar Both of the "text" elements that you've identified above are in common code. view.dialogs.memoryLimitDialog.text is in OopsDialog. generationSpinner.numberDisplay.valueText is in NumberDisplay. If I remove those, it will change the API of all sims, and require migration rules for all sims.

I recommend that we leave this "as is" for Natural Selection. And optionally open issue(s) to uninstrument Text/RichText in common code. @arouinfar thoughts?

@pixelzoom pixelzoom assigned arouinfar and unassigned pixelzoom Jul 25, 2023
@pixelzoom
Copy link
Contributor

... And optionally open issue(s) to uninstrument Text/RichText in common code. ...

See https://github.com/phetsims/phet-io/issues/1952

@arouinfar
Copy link
Author

I recommend that we leave this "as is" for Natural Selection. And optionally open issue(s) to uninstrument Text/RichText in common code. @arouinfar thoughts?

Agreed @pixelzoom. Thanks for opening https://github.com/phetsims/phet-io/issues/1952! Closing.

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 27, 2023

Reopening. Over in https://github.com/phetsims/phet-io/issues/1952, we decided to address the common-code elements that appear in for Natural Selection 1.5:

  • NumberDisplay
  • OopsDialog

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 28, 2023

In the above commits, I uninstrumented RichText in OopsDialog. This was relatively easy because natural-selection-phet-api.json is the only API that contained this element, and it was added since the 1.4 release, so could be removed without a migration rule.

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 28, 2023

NumberDisplay quickly turned into a big effort. numberDisplay.valueText.stringProperty is missing the string pattern dependency, so there's much more work to be done in NumberDisplay than just uninstrument numberDisplay.valueText -- see phetsims/scenery-phet#812.

So for this sim, I simply uninstrumented generationSpinner.numberDisplay. The Studio tree used to look like this:

screenshot_2680

... and now it looks like this:

screenshot_2681

This seems OK to me for now. There's really no reason to make the numberDisplay invisible (its visibleProperty probably should have been readonly). And all of the info is available elsewhere. And we have not instrumented the other NumberDisplay instance in this sim, whcih would have been populationGraphNode.dataProbeNode.numberDisplay.

@arouinfar Please review. To summarize:

  • memoryLimitDialog.text was uninstrumented
  • memoryLimitDialog.memoryLimitMessageStringProperty has been added
  • generationSpinner.numberDisplay was uninstrumented

@arouinfar
Copy link
Author

Thanks @pixelzoom. The updates and tree structure look great.

I noticed that toggling generationSpinner.visibleProperty can create layout issues.
image

I don't remember seeing this before, but I might of missed it. Not sure if this is related to the changes made to generationSpinner here or if this is entirely unrelated issue.

@pixelzoom
Copy link
Contributor

pixelzoom commented Jul 31, 2023

Unrelated to this issue (I reverted the above changes to confirm) and generationSpinner.visibleProperty has always been writeable. So I created #347.

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

2 participants