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

Add support for dynamic locale #140

Closed
pixelzoom opened this issue Jul 26, 2023 · 12 comments
Closed

Add support for dynamic locale #140

pixelzoom opened this issue Jul 26, 2023 · 12 comments

Comments

@pixelzoom
Copy link
Contributor

No description provided.

pixelzoom added a commit that referenced this issue Jul 26, 2023
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jul 26, 2023

In the above commits, I converted the easy stuff to StringProperty.

Still to be done:

  • See //TODO https://github.com/phetsims/graphing-lines/issues/140 comments for remaining conversions to StringProperty

  • Test dynamic layout with ?stringTest=dynamic and address problems.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 18, 2023

@kathy-phet asked me to finish this off, in preparation for publishing with character sets for the Region and Culture feature. ..."looking at early Feb for when this would need to be ready."

@pixelzoom
Copy link
Contributor Author

In a Slack#DM with @amanda-phet, we determined that #142 is behind schedule, and it's unclear when this needs to be ready for QA. So I'm going to defer this issue to work on other higher-priority things. @amanda-phet Please assign this back to me when I should start working on it. I'll need a minimum of 2 weeks notice to get this ready for QA.

@pixelzoom pixelzoom assigned amanda-phet and unassigned pixelzoom Jan 22, 2024
pixelzoom added a commit that referenced this issue Jan 22, 2024
pixelzoom added a commit that referenced this issue Jan 22, 2024
pixelzoom added a commit that referenced this issue Jan 22, 2024
pixelzoom added a commit that referenced this issue Jan 22, 2024
pixelzoom added a commit that referenced this issue Jan 22, 2024
@pixelzoom
Copy link
Contributor Author

In the above commits, I added a guard to prevent reentry of updateLayout. See isUpdatingLayout in SlopeInterceptEquationNode and PointSlopeEquationNode:

    // to prevent stack overflow, see https://github.com/phetsims/graphing-lines/issues/140#issuecomment-1904968755
    let isUpdatingLayout = false;

This works in PointSlopeEquationNode, but not in SlopeInterceptEquationNode, where I've added this TODO:

    const dynamicStringMultilink = Multilink.lazyMultilink(
      //TODO https://github.com/phetsims/graphing-lines/issues/140 Adding xText.boundsProperty to dependencies fails with 'stack size exceeded', despite isUpdatingLayout guards
      [ yText.boundsProperty, GraphingLinesStrings.slopeUndefinedStringProperty ],

@pixelzoom
Copy link
Contributor Author

PointSlopeEquationNode was failing CT with 'stack size exceeded'. So in 793ff35, I remove the isUpdatingLayout guard and problem dependencies in both PointSlopeEquationNode and SlopeInterceptEquationNode. See the TODO comments in those files.

@pixelzoom
Copy link
Contributor Author

I was asked to resume this work, see #142 (comment).

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Feb 6, 2024

I'll need @jonathanolson's assistance debugging the "stack size exceeded" problems. We're meeting on 2/6/2024.

@jonathanolson If you'd like to take a look ahead of time, here are the 2 cases that are failing, with the failing dependencies commented out. Both of these cases will fail on startup.

SlopeInterceptEquationNode.ts:

    // If dynamic strings change, update the layout. xNode.boundsProperty and yNode.boundsProperty are RichText that
    // are observing a StringProperty. slopeUndefinedStringProperty is used in this.updateLayout.
    const dynamicStringMultilink = Multilink.lazyMultilink(
      //TODO https://github.com/phetsims/graphing-lines/issues/140 Adding xText.boundsProperty to dependencies fails with 'stack size exceeded'.
      // [ xText.boundsProperty, yText.boundsProperty, GraphingLinesStrings.slopeUndefinedStringProperty ],
      [ yText.boundsProperty, GraphingLinesStrings.slopeUndefinedStringProperty ],
      () => updateLayout( lineProperty.value )
    );

PointSlopeEquationNode.ts:

    // If dynamic strings change, update the layout. xNode.boundsProperty and yNode.boundsProperty are RichText that
    // are observing a StringProperty. slopeUndefinedStringProperty is used in this.updateLayout.
    const dynamicStringMultilink = Multilink.lazyMultilink(
      //TODO https://github.com/phetsims/graphing-lines/issues/140 Adding xText.boundsProperty and yText.boundsProperty to dependencies fails with 'stack size exceeded'.
      // [ xText.boundsProperty, yText.boundsProperty, GraphingLinesStrings.slopeUndefinedStringProperty ],
      [ GraphingLinesStrings.slopeUndefinedStringProperty ],
      () => updateLayout( lineProperty.value )
    );

@jonathanolson
Copy link
Contributor

Checked into SlopeInterceptEquationNode.

xText's x-translation value seems to be ping-ponging. There's a lot of logic, so I didn't trace down the exact pattern (if that's helpful to do, let me know). However we're very clearly listening to (and modifying) xText's bounds (because we are positioning it), so we've created a lot of infinite-loop potential. I didn't check PointSlopeEquationNode, but it seems likely something similar is happening there.

For the dynamic locale support, we're presumably caring about xText's localBounds (that change with the GLSymbols.xStringProperty). Listening directly to the localBounds kills both of the infinite loops (since updating xText's position won't trigger another loop).

Committed the localBounds change above. Let me know if that looks good (and available at the planned 10am for discussion).

@jonathanolson jonathanolson removed their assignment Feb 6, 2024
pixelzoom added a commit that referenced this issue Feb 6, 2024
@pixelzoom
Copy link
Contributor Author

I always forget about observing localBoundsProperty as an alternative, and that seems to be working great in these cases. Thanks @jonathanolson!

@pixelzoom
Copy link
Contributor Author

@amanda-phet This is ready for review. The easiest way to review is with ?stringTest=dynamic.

@amanda-phet
Copy link
Contributor

Thanks for reminding me of that query parameter. All is looking good to me.

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

3 participants