Supplying tandems to RichText links

jonathanolson commented 3 years ago

In https://github.com/phetsims/scenery/issues/1078, I added Tandem.OPT_OUT for links in RichText listeners, however it's likely we'll want to provide tandems for those input listeners.

How should <a href="..."> links in RichText be provided with tandems, or should they? <a href="..." tandem="foo"> would take the RichText's tandem and createTandem( 'foo' ) it?

Thoughts?

samreid commented 3 years ago

<a href="..." tandemName="foo"> seems most scalable to me, if it is not too difficult.

jonathanolson commented 3 years ago

Also... is the absence of that a good way to opt-out of a tandem? It seems like we'd want the default to be "you have to supply a tandem"?

samreid commented 3 years ago

How about <a href="..." tandemName="null"> and an assertion check that the tandemName must end with *InputListener if not 'null'? Or maybe it should be *FireListener, I'm not sure.

zepumph commented 3 years ago

I think that the structure should be link.fireListener, so tandemName should either assert that it ends in Link, or Link should get tacked on (I think an assertion is most idiomatic).

Then we can have a consistent api for the fireListener.

Another idea. We could just use numbers, like myText.link1, and have a private group tandem. That may be easier, and nice to not expose tandemName in translated strings etc.

I also want to add a concern for dynamic elements. I don't know the inner workings of RichText.rebuildRichText, but in my mind, the worst case scenario is that every time the text of the richText changes (text that doesn't apply to the link), then the link would need to get recreated too. Is RichText smarter than that? If not, we may need to work on another phet-io feature in which disposing and recreating the same phetioID in the same frame is allowed (@samreid I can't find that issue, can you?).

Likely things are smarter than my above paragraph, but we should confirm, Take this example. . .


myText = new RichText( 'Hello <a href="website">Friend</a>' ); // Instrumented FireListener created here

myText.text = 'Hello <a href="website">Friendly Friendly Giant</a>'; // Does it know that that a tag is the same? Or Will it tear it down and recreate?

samreid commented 3 years ago

It's my understanding that PhetioGroup doesn't currently re-use indices. I'm not sure whether RichText rebuilds links on change, but it would be reasonable to expect that it does. Regarding destroying and re-creating in the same frame, I think that is forbidden by the validator because it checks eagerly. I couldn't find an issue about making that check "at the end of the frame" instead of eagerly.

It may be easier for us to get it working with PhetioGroup, but if we consider the readability and understandability of the data stream, we should prefer an approach with named (not numbered) links.

For instance, if one version of a sim has 'Hello <a href="website">Friendly Friendly Giant</a>' and the next version has 'Hello <a href="greeting">Hello!</a><a href="website">Friendly Friendly Giant</a>', then it would be confusing that link1 has a different meaning in each.

That may be easier, and nice to not expose tandemName in translated strings etc.

If we don't currently have a way to assert that "code" parts of translation strings remain undisturbed, we should probably add that.

zepumph commented 3 years ago

I think we need to reach out to @jonathanolson to figure out if links are recreated when changing text, even if the same link persists. If the same RichTextLink instances persist, is this an implementation detail that we should rely on?

If we go with supporting tandemName and things get recreated, then each will need a PhetioCapsule to wrap them, or change our validation rules to support regeneration in the same frame. I would hate to use a PhetioCapsule, and would probably prefer just going with PhetioGroup.

For instance, if one version of a sim has . . .

I'm sure that there are other instances in the data stream where changes between versions can make for challenging situations to grok. If one really cares about knowing when a particular link was interacted with, then a unique ID for just that version does not seem like the end of the world to me. That said, the tradeoff is less usability for easier implementation, which in general is not the best philosophy.

jonathanolson commented 3 years ago

I believe RichTextLinks are recreated on any change.

Also... as a general issue, how does phet-io handle pooled Nodes or PhetioObjects in general? Presumably a RichTextLink would take multiple tandems or hrefs over its lifetime, but my understanding is that this isn't currently possible with phet-io.

samreid commented 3 years ago

Each of the proposals seems problematic. What if we (for now) restrict ourselves to the case where instrumented RichTexts that have no more than 1 link and are never rebuilt? The fireListener can be myRichText.fireListener. Simulation cases with RichTexts that have >1 link can be OPT_OUT for now until we have design team discussions. Also, maybe I am confused about something--I only see one creation of fireListener in RichText.initialize.

UPDATE: Or should we just add PhetioGroup for now?

zepumph commented 3 years ago

That is not in RichText.initialize, but instead in RichTextLeaf.initialize, which is one of potentially many sub components that are created when a the text is "richified". There are 4 classes in RichText.

zepumph commented 3 years ago

I'd be curious to see if https://github.com/phetsims/phet-io/issues/1729 is easier than we think. That said, I'm happy to go with @samreid's naive solution for now. I agree that multiple links in the same string aren't very common in phet-io currently.

jonathanolson commented 3 years ago

I'm not sure I understand how this will work...

That is not in RichText.initialize, but instead in RichTextLeaf.initialize

Say RichText A creates RichTextLeaf X with a tandem. Then RichText A changes text and rebuilds (releasing RichTextLeaf X to the pool), and RichTextB pulls it OUT of the pool, trying to give it a new tandem.

Can objects be given a different tandem? I don't see how the "naive solution" works given our current constraints. Can tandems and other properties be reassigned on the same PhetioObject?

zepumph commented 3 years ago

Can objects be given a different tandem

It doesn't really work like this. the Tandem tree is basically a singleton data structure, so if a Tandem has already been created for a phetioID, calling createTandem() to try to make the same phetioID will just grab the same instance, instead of creating two Tandem instances with the same phetioIDs.

Thus I THINK (hesitantly) that something like https://github.com/phetsims/phet-io/issues/1729 may work for the case above with poolable, because you would be recreating the exact same phetioID. But it is untested and would need be thought through more.

jonathanolson commented 3 years ago

because you would be recreating the exact same phetioID

I don't see how that's the case. If RichText A has a tandem firstScreen.model.something, and RichText B has tandem secondScreen.view.foo....

zepumph commented 3 years ago

Oh sorry, you are talking about something slightly different from me. I think you are talking about general support for Poolable and PhetioObject. This is untested also, but in theory we can just have dispose do a better job of freeing PhetioObject fields, such that when you initialize it again for the next instance, it can take whatever values it needs to gracefully.

The validation part is all in phetioEngine, where you register PhetioObjects, and presumably you can just unregister ObjectA for TandemX, and then we can work out how to clean it up, and then we can reinitialize ObjectA with TandemY and add that back to phetioEngine, and the engine will be none the wiser.

Again this isn't supported currently, but we should come up with some cases where this is important so that we can prototype. (In a new issue likely)

samreid commented 3 years ago

Here are some options:

DEFER: Maybe what would help this issue the most is if we could tie it to a deliverable. If we can decide that, for the moment, we don't need a clickstream on the links in the about dialog, then things can remain as OPT_OUT. Then when we have a PhET-iO sim that requires a clickstream for RichText links, we can address it then with context and support from designers.
HANDLE SIMPLE: What if we (for now) restrict ourselves to the case where instrumented RichTexts that have no more than 1 link and are never rebuilt? The fireListener can be myRichText.fireListener. Simulation cases with RichTexts that have >1 link can be OPT_OUT for now until we have design team discussions. Also, maybe I am confused about something--I only see one creation of fireListener in RichText.initialize.
PHETIO_GROUP: Use PhetioGroup to increment indices of listeners each time they are recreated.

Am I missing other workable options? Which do we prefer at the moment?

jonathanolson commented 3 years ago

Deferring seems natural until we hit a case where this would be used. Perhaps we'll have support for changing tandems on objects by then?

samreid commented 3 years ago

That sounds reasonable to me, unassigning.

zepumph commented 3 years ago

I like "HANDLE SIMPLE". It is easy and I don't see any usages of RichText Links besides UnderDevelopmentPlane and the about dialog link texts. To me this is the best solution, and when we have a PhET-iO case that needs more, we can reevaluate (which is what @jonathanolson seems to be calling for). I feel like it is unlikely that we will encounter multiple links in a PhET-iO context any time soon, and with this patch below, we will get events like:

{
          "index": 309,
          "time": 1611973991569,
          "type": "USER",
          "phetioID": "gravityAndOrbits.general.view.navigationBar.phetButton.phetMenu.aboutDialogCapsule.aboutDialog.termsPrivacyAndLicensingLinkText.fireListener.firedEmitter",
          "name": "emitted",
          "componentType": "EmitterIO<NullableIO<SceneryEventIO>>",
          "data": {
            "event": {
              "type": "up",
              "domEventType": "pointerup",
              "point": {
                "x": 348,
                "y": 606
              }
            }
          }

```diff Index: js/nodes/RichText.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/nodes/RichText.js b/js/nodes/RichText.js --- a/js/nodes/RichText.js (revision 9a0fd3e4db6a4ce0fe0111f8b5b40cdeb6955dcc) +++ b/js/nodes/RichText.js (date 1611973856023) @@ -60,10 +60,10 @@ import StringProperty from '../../../axon/js/StringProperty.js'; import TinyForwardingProperty from '../../../axon/js/TinyForwardingProperty.js'; import Matrix3 from '../../../dot/js/Matrix3.js'; -import Poolable from '../../../phet-core/js/Poolable.js'; import extendDefined from '../../../phet-core/js/extendDefined.js'; import merge from '../../../phet-core/js/merge.js'; import openPopup from '../../../phet-core/js/openPopup.js'; +import Poolable from '../../../phet-core/js/Poolable.js'; import Tandem from '../../../tandem/js/Tandem.js'; import IOType from '../../../tandem/js/types/IOType.js'; import FireListener from '../listeners/FireListener.js'; @@ -229,6 +229,10 @@ phetioType: RichText.RichTextIO }, options ); + // @private - For creating sub tandems in rebuildRichText() which occur before PhetioObject.initializePhetioObject( + // is called on this RichText. This cannot be named `tandem` or else instrumentation in Node doesn't occur correctly. + this.tempRichTextTandem = options.tandem; + // @private {Node} - Normal layout container of lines (separate, so we can clear it easily) this.lineContainer = new Node( {} ); this.addChild( this.lineContainer ); @@ -409,7 +413,7 @@ } } - const linkRootNode = RichTextLink.createFromPool( linkElement.innerContent, href ); + const linkRootNode = RichTextLink.createFromPool( linkElement.innerContent, href, this.tempTandem ); this.lineContainer.addChild( linkRootNode ); // Detach the node from its location, adjust its transform, and reattach under the link. This should keep each @@ -450,6 +454,7 @@ */ dispose() { this.freeChildrenToPool(); + this.tempRichTextTandem = null; super.dispose(); @@ -1832,8 +1837,9 @@ * * @param {string} innerContent * @param {function|string} href + * @param {Tandem} tandem - keep this as an arg so that we don't instrument this Node (it is just an intermediary) */ - constructor( innerContent, href ) { + constructor( innerContent, href, tandem ) { super( { cursor: 'pointer', tagName: 'a' @@ -1845,7 +1851,7 @@ // @private {function|null} this.accessibleInputListener = null; - this.initialize( innerContent, href ); + this.initialize( innerContent, href, tandem ); } /** @@ -1854,9 +1860,10 @@ * * @param {string} innerContent * @param {function|string} href + * @param {Tandem} tandem - keep this as an arg so that we don't instrument this Node (it is just an intermediary) * @returns {RichTextLink} - Self reference */ - initialize( innerContent, href ) { + initialize( innerContent, href, tandem ) { // pdom - open the link in the new tab when activated with a keyboard. // also see https://github.com/phetsims/joist/issues/430 this.innerContent = innerContent; @@ -1865,7 +1872,7 @@ if ( typeof href === 'function' ) { this.fireListener = new FireListener( { fire: href, - tandem: Tandem.OPT_OUT + tandem: tandem.createTandem( 'fireListener' ) } ); this.addInputListener( this.fireListener ); this.setAccessibleAttribute( 'href', '#' ); // Required so that the click listener will get called. @@ -1891,7 +1898,7 @@ self._linkEventsHandled && event.handle(); openPopup( href ); // open in a new window/tab }, - tandem: Tandem.OPT_OUT + tandem: tandem.createTandem( 'fireListener' ) } ); this.addInputListener( this.fireListener ); this.setAccessibleAttribute( 'href', href ); @@ -1914,6 +1921,7 @@ } this.removeInputListener( this.fireListener ); + this.fireListener.dispose(); this.fireListener = null; if ( this.accessibleInputListener ) { this.removeInputListener( this.accessibleInputListener ); ```

Note the weirdness about having access to a tandem before mutate is called (and even while mutate occurs but it hasn't yet become an initialized PhetioObject yet like in setText). That is a bit of a bummer, but if that could be a bit better, then I think this is the way to go.

What do you think SR and JO?

jonathanolson commented 3 years ago

First of all, the fireListener.dispose() seems like it should be added (as it's overwritten anyway every time).

That... seems like it works, although it will fail out if you provide a tandem and 2+ links (what happens if you provide no tandem and 2+ links?)

I'd like to hear @samreid's thoughts.

samreid commented 3 years ago

The patch has a mismatched variable name, tempTandem should be renamed to tempRichTextTandem. After I tried that, I added 2 URLs to one RichText in AboutDialog, and got this error:

Assertion failed: phetioID already registered: gravityAndOrbits.general.view.navigationBar.phetButton.phetMenu.aboutDialogCapsule.aboutDialog.termsPrivacyAndLicensingLinkText.fireListener.pressAction

The proposal from @zepumph therefore seems like a great solution for now, any RichText with 2+ links will loudly error out.

First of all, the fireListener.dispose() seems like it should be added (as it's overwritten anyway every time).

The patch does have a fireListener.dispose() so I'm confused about this comment. Back to @zepumph

jonathanolson commented 3 years ago

any RichText with 2+ links will loudly error out

Any RichText with 2+ links AND tandems hopefully?

The patch does have a fireListener.dispose() so I'm confused about this comment.

Agreed, that should have existed before the patch, and was an omission on my part!

samreid commented 3 years ago

Any RichText with 2+ links AND tandems hopefully?

Yes, that's my understanding.

zepumph commented 3 years ago

I'm happy to implement and test on my next iO day.

jonathanolson commented 1 year ago

Added the missing fireListener dispose in the commit above.

zepumph commented 1 year ago

Ok great. Thanks! Closing

phetsims / scenery

Supplying tandems to RichText links #1132