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

fix(timer): restart can prevent executing onEnd function #667

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Frowmza
Copy link

@Frowmza Frowmza commented Nov 9, 2024

If we want to restart a timer, previous state would disable the variable that triggers the OnEnd function by default.

Didn't notice we can disable/enable it thru :forceEnd
@jag3dagster
Copy link
Member

Seems like a having a definitive one way or the other isn't ideal. An optional parameter that lets users toggle the trigger of the onEnd function. With the default being false to prevent a breaking change.

@Frowmza
Copy link
Author

Frowmza commented Jan 16, 2025

agree, should be good!

Frowmza added a commit to Frowmza/overextended.github.io that referenced this pull request Jan 16, 2025
Comment on lines +99 to +101
if type(onEnd) == 'function' then
self.private.onEnd = onEnd
end
Copy link
Member

Choose a reason for hiding this comment

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

I don't think dynamically adding an onEnd function during a reset makes a lot of sense. What would this use case actually be?

@thelindat what are your thoughts on allowing an onEnd function to be added during a reset? Personally it feels like a really confusing user experience / flow.

function timer:restart(async)
self:forceEnd(false)
function timer:restart(async, onEnd)
self:forceEnd(not not onEnd)
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this double not flip is to handle the function case, but I think there is a better way to handle this if we do end up wanting the onEnd to accept a function.

Copy link
Author

Choose a reason for hiding this comment

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

yeah just to pass it as boolean and not function. But let see what linden got!

@thelindat
Copy link
Member

I have a different idea to resolve this problem.

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.

3 participants