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

Unused term maxVoltagePercent in multilink #107

Closed
samreid opened this issue Mar 18, 2024 · 3 comments
Closed

Unused term maxVoltagePercent in multilink #107

samreid opened this issue Mar 18, 2024 · 3 comments

Comments

@samreid
Copy link
Member

samreid commented Mar 18, 2024

Discovered in code review #103, ACPowerSupply says:

    Multilink.multilink( [ this.maxVoltagePercentProperty, this.angleRangeProperty ],
      ( maxVoltagePercent, angleRange ) => {
        this._angleProperty.value = angleRange.min;
      } );

The maxVoltagePercent appears unused.

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 18, 2024

Yes indeed, the value of maxVoltagePercentProperty is unused, but it is indeed a dependency. If either maxVoltagePercentProperty or angleRangeProperty is changed, then angleProperty needs to be set to the min angle. And I'd rather have unused callback params than a partial list of callback params. (Recall that including unused params in callbacks is up to developer discretion.)

Back to @samreid, close if OK.

@pixelzoom
Copy link
Contributor

To clarify... What I've tried to do consistently (there are probably exceptions) for Multilink and DerivedProperty callbacks is:

  • Include all parameters if any dependency value is used.
  • Include no parameters if no dependency values are used, or if calling some other instance method.

For example:

// FELScreenView.ts, panelsBounds is not used.
    Multilink.multilink( [ this.visibleBoundsProperty, options.panels.boundsProperty ],
      ( visibleBounds, panelsBounds ) => {
        options.panels.right = visibleBounds.right - FELConstants.SCREEN_VIEW_X_MARGIN;
        options.panels.top = this.layoutBounds.top + FELConstants.SCREEN_VIEW_Y_MARGIN;
      } );
// FieldNode.ts
    Multilink.multilink(
      [ magnet.positionProperty, magnet.rotationProperty, magnet.strengthProperty, magnet.fieldScaleProperty ],
      () => this.update()
    );

samreid added a commit that referenced this issue Mar 27, 2024
@samreid
Copy link
Member Author

samreid commented Mar 27, 2024

Great! I documented it as you described, and the policy for the multilink and derived property sounds good. Closing.

@samreid samreid closed this as completed Mar 27, 2024
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