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

[Web] Migrate to get and set API [1/2] #3255

Closed
wants to merge 19 commits into from
Closed

Conversation

m-bert
Copy link
Contributor

@m-bert m-bert commented Dec 2, 2024

Caution

Suppressed by #3263

Description

In this PR I've migrated our handlers into get/set functions instead of traditional getters and setters convention (like in Java).

Note

This PR changes only handlers and detectors, tools directory is not a part of this PR as it would get too big.

Test plan

Run web example

@m-bert m-bert changed the base branch from main to next December 2, 2024 15:27
@m-bert m-bert changed the title [Web] Migrate to get and set API [Web] Migrate to get and set API [0/?] Dec 4, 2024
@m-bert m-bert changed the title [Web] Migrate to get and set API [0/?] [Web] Migrate to get and set API [1/2] Dec 4, 2024
@m-bert m-bert marked this pull request as ready for review December 4, 2024 11:05
@m-bert m-bert requested review from latekvo and j-piasecki and removed request for latekvo December 4, 2024 11:06
Comment on lines +708 to 713
public get state() {
return this._state;
}
public set state(state: State) {
this._state = state;
}
Copy link
Contributor

@latekvo latekvo Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously the currentState was protected, and was only used internally.
Now the state is exposed as its getters are public.
Same for tracker, hasCustomActivationCriteria, and multiple other internal variables which were private/protected, and not exposed by any public getters or setters before these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, yes and no. Even though those modifiers are public, they are not available to end user. At least doing something like:

const pan = Gesture.Pan()
pan.state = State.ACTIVE

won't work as the user operates on Gesture type.

I've also thought about removing setters/getters that are not used, but I'm not sure about that.

Either way, we can change access modifiers to match previous state of this implementation, but I don't think it makes much sense to create private getters and setters. CC @j-piasecki

Comment on lines +689 to +691
public getTrackedPointersID(): number[] {
return this.pointerTracker.getTrackedPointersID();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentional? I get that getTrackedPointersID is just forwarding a function with the same name, but I'm not sure why we can't make it a getter called trackedPointersID

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this is a bit of a nuance, and only relevant if you decide to change this function into a get-getter,
but trackedPointersID sounds like there's a single ID for multiple pointers. Since we're already changing the name of the function, is it ok if we rename it to trackedPointerIDs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're already changing the name of the function, is it ok if we rename it to trackedPointerIDs?

Sure, we can

I'm not sure why we can't make it a getter called trackedPointersID

Here we have to decide whether we want to have getters to values that are calculated, not stored in a private fields. It is already done ScaleGestureDetector, but I'm still not sure if we want that.

On the other hand, i get that it doesn't look good that we have ES6 and Java-style getters 😞

What's your stance on that, @j-piasecki?

@m-bert
Copy link
Contributor Author

m-bert commented Dec 6, 2024

After further discussions I'm closing this PR in favor of #3263

@m-bert m-bert closed this Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants