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

Timer and TimerComponent APIs are inconsistent #2386

Open
luanpotter opened this issue Mar 6, 2023 · 4 comments
Open

Timer and TimerComponent APIs are inconsistent #2386

luanpotter opened this issue Mar 6, 2023 · 4 comments

Comments

@luanpotter
Copy link
Member

Taking a look at Timer and TimerComponent, I realized they have a different nomenclature on their APIs:

On TimerComponent, the length/duration of the timer is called period and it is a named parameter. That is a good name for when repeat is true.

But on the underlying Timer implementation, it is actually called limit and it is not a named parameter. That name feels better for non-repeating timers.

-> I do not have a strong preference for the parameter name, but I think it should be consistent.
-> I think whether it is named-parameter or not should also be consistent - but I do tend to prefer the named-parameter on this matter, as it is more readable from the call site.

Thoughts? I can happily update one or the other depending on everyone's preferences.
It is a breaking change, but I think keeping them consistent is very important to keep simplicity and avoid confusion for our users (for example when changing code from one to the other).

@spydon
Copy link
Member

spydon commented Mar 6, 2023

Should we add this to the list of breaking V2 changes, or do it directly? #1938

@luanpotter
Copy link
Member Author

I considered putting it there, but it came to me we do allow some minor breaking changes.
This seems pretty mild to me, as it is just a trivial rename.
But no strong opinions either way -- I leave it up to you guys.

@ufrshubham
Copy link
Collaborator

ufrshubham commented Feb 9, 2024

I'd like to suggest one more breaking change.

The onTick in TimerComponent seems redundant because Timer also stores an onTick callback. TimerComponent unnecessarily exposes a public onTick method which is registered with the internal Timer. The default implementation of TimerComponent.onTick invokes the _onTick which is provided by the user. This seem an uncessary indirection.

On top of that, one cannot change the callback of TimerComponent dynamically, but it is possible to change it for the Timer. This can leads to a following confusing code.

final tc = TimerComponent(period: 1, onTick: myCallback1);

final b1 = tc.onTick == tc.timer.onTick; // True
final b2 = tc.onTick == myCallback1; // False
final b3 = tc.timer.onTick == myCallback1; // False

tc.timer.onTick = myCallback2; // Only way to changes the callback.

final b4 = tc.onTick == tc.timer.onTick; // False.
final b5 = tc.onTick == myCallback1; // True.

// At this point, tc essentially still keeps a reference to myCallback1 even though it is not going to be called by the internal timer.

To avoid this and keep the API simple, we can completely remove the TimerComponent.onTick and directly set the user provided callback on the internal Timer.

@spydon
Copy link
Member

spydon commented Feb 10, 2024

To avoid this and keep the API simple, we can completely remove the TimerComponent.onTick and directly set the user provided callback on the internal Timer.

Sounds 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