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

Autoselect target follows linked elements recursively (is that good?) #300

Closed
samreid opened this issue Jun 15, 2023 · 5 comments
Closed

Comments

@samreid
Copy link
Member

samreid commented Jun 15, 2023

While working on phetsims/center-and-variability@f2941dc in phetsims/center-and-variability#204 I saw there were 2 levels of linked elements. The cardNode => cardModel => soccerBallModel. The value is changed on the soccer ball model. I incorrectly thought autoselecting linked element on the cardNode would autoselect to the cardModel, but it ended up all the way at soccerBallModel. @zepumph is this all good?

UPDATE: In this case it's probably OK since all of the important data is over in soccerBallModel. But some other sim may have important data in the cardModel layer itself.

zepumph added a commit that referenced this issue Jun 16, 2023
@zepumph
Copy link
Member

zepumph commented Jun 16, 2023

I agree. How does this sound?

@zepumph
Copy link
Member

zepumph commented Jun 16, 2023

I am considering this a bug fix for the original implementation of sim-side autoselect over in https://github.com/phetsims/studio/issues/304

@samreid
Copy link
Member Author

samreid commented Jun 19, 2023

I reviewed the commit and confirmed it works well in center and variability. Some questions:

  • Is there a better way to name fromLinking? I couldn't think of one, but just wondering if that could help maintainability.
  • I noticed there are overrides of getPhetioMouseHitTarget that do not provide the fromLinking parameter. For instance, in ReadOnlyProperty. Don't all subclasses need to be aware of this parameter and pass it to super?

@samreid samreid assigned zepumph and unassigned samreid Jun 19, 2023
zepumph added a commit to phetsims/axon that referenced this issue Jun 20, 2023
zepumph added a commit to phetsims/scenery that referenced this issue Jun 20, 2023
@zepumph
Copy link
Member

zepumph commented Jun 20, 2023

Nice catch. To understand better, I was able to create a failing unit test that showed how we could still recurse into a Text.stringProperty if in linking selection mode. Anything else here?

I can't think of a better name than fromLinking want to think about something else?

@zepumph zepumph assigned samreid and unassigned zepumph Jun 20, 2023
@samreid
Copy link
Member Author

samreid commented Jun 20, 2023

Excellent, thanks! Looks good, closing.

@samreid samreid closed this as completed Jun 20, 2023
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