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

Allow canceling a timer #1478

Merged
merged 3 commits into from
Aug 25, 2023
Merged

Allow canceling a timer #1478

merged 3 commits into from
Aug 25, 2023

Conversation

falkoschindler
Copy link
Contributor

As requested in #1477, this PR adds the possibility to cancel a timer.
This stops the infinite while loop. Afterwards the timer cannot be activated again.

@falkoschindler falkoschindler added the enhancement New feature or request label Aug 23, 2023
@falkoschindler falkoschindler added this to the 1.3.12 milestone Aug 23, 2023
@bergmansj
Copy link

Looks good. Thanks for picking this up so quickly 👍

@rodja
Copy link
Member

rodja commented Aug 23, 2023

@falkoschindler I would like to use .remove() instead of .cancel() because it better carries the information of not being able to resume it.

@falkoschindler
Copy link
Contributor Author

@rodja First I chose .stop(), but then went for .cancel() to indicate the destructive nature of this method. I'm not sure about .remove(), because it suggests a similarity to an element, which the timer isn't. But I'll think about it once more.

@rodja
Copy link
Member

rodja commented Aug 23, 2023

Ok. .remove() is to close to elements. I thought about .abort() but that is very similar to .cancel(). So we can go with it, I think.

@natankeddem
Copy link
Contributor

natankeddem commented Aug 24, 2023

I like.destroy() if you guys are taking suggestions. It seems fairly popular with some JS libraries.

@rodja
Copy link
Member

rodja commented Aug 24, 2023

Cool @natankeddem. I'm also in favour of .destroy() because it hints at the internal ._cleanup() method which releases the .slot and .callback members. The object is practically unusable after calling.

@falkoschindler
Copy link
Contributor Author

I was about to push a commit renaming cancel() to destroy().

But the demo feels weird:
The title "Activate, deactivate and cancel a timer" becomes "Activate, deactivate and destroy a timer"
And the "Cancel" button becomes a "Destroy" button.

Other example: In a code-review, would you really say "Oh, we need to destroy the timer when the user clicks this button"? I think I would say "cancel" even though I would be writing "destroy". That's not a good sign.

@bergmansj
Copy link

In async frameworks tasks that are interrupted before finishing and in general cannot be resumed, the term 'cancelled' is used.

Copy link
Member

@rodja rodja left a comment

Choose a reason for hiding this comment

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

I'm now convinced .cancel() is the right name for it.

@rodja rodja merged commit 88124b2 into main Aug 25, 2023
6 checks passed
@rodja rodja deleted the cancel-timer branch August 25, 2023 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants