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

Patch 2.0.0 #10

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

Patch 2.0.0 #10

wants to merge 38 commits into from

Conversation

jay-jay-lama
Copy link

feat:

  • Added method skip
  • Added method cancel
  • Added method reset
  • Added an ability to start timer without setting publishing time
  • Added state publishers to make it class friendly
  • Added several timers states

refactor:

  • Added comparability with Swift 6.0
  • Architecture updated
  • Renamed methods
  • Documentation updated

@amirsaam
Copy link

amirsaam commented Dec 2, 2024

@jay-jay-lama
Can we have a completion handler for bindTimerStatus too?
I do also think on skip we should mark status of the timer to cancelled not finished cuz you know it is getting skipped and cancelled not actually finishing the time it should've finished in order to differentiate between when they actually happen and trigger actions based on them.

@jay-jay-lama
Copy link
Author

jay-jay-lama commented Dec 2, 2024

@amirsaam

Hello

  1. Timer status binding
    There are 2 functions to receive status change updates

completion
func onTimerStatusChange(_ action: @escaping (_ timerStatus: MTimerStatus) -> ()) -> MTimer

binding
func bindTimerStatus(timerStatus: Binding<MTimerStatus>) -> MTimer

also MTimer itself is ObservableObject so you can observe changes in timer properties
Here you can find more information https://github.com/Mijick/Timer/wiki/Timer-State-Observing

  1. Skipping
    Using the skip function will cause the timer to go to the “Final” state.
    timerProgress = 1.0
    timerStatus = .finished
    timerTime = endime
    So I can't put the timer in a canceled state.

If you want to cancel the timer - just use the cancel function. In this case, the timer properties will receive these values:
timerProgress = 0.0
timerStatus = .cancelled
timerTime = startTime

  1. .Cancelled status.
    I think we will get rid of this status, because cancel function simply returns the timer to the start position. And logically it is better to set the status .notStarted.

@amirsaam
Copy link

amirsaam commented Dec 2, 2024

@jay-jay-lama
Oh shoot, I didn't notice onTimerStatusChange in the wiki cuz there was no example for it (didn't read the doc carefully sorry)
But about the skip and cancel, I think we are thinking about different use cases that's why you think cancelled state is not usable and marking skip as cancelled is wrong.
I am using your package in a Werewolf (Mafia) game to keep track of each person's turn to talk, so I need to know if someone turn ends sooner than talking until end of the time, if time gets finished I need to send alarm but not when it gets skipped because you know timer didn't finish so I need to differentiate finish state with skip state, so cancelled is what I need (and I think in many similar use cases) to receive.

P.S: Maybe in a simple Timer app what you are saying about skip and cancel is ok but not in other use cases

@jay-jay-lama
Copy link
Author

jay-jay-lama commented Dec 2, 2024

@amirsaam Hm. I see what you are talking about.

I think that you can use a pause method. It fits this use case.

The timer properties in such case will have next values:
timerProgress = current progress
timerStatus = .paused
timerTime = current time

If timer is paused you can see that time is not expired. It also has separate status.

@amirsaam
Copy link

amirsaam commented Dec 2, 2024

@jay-jay-lama
pause is also used in my app, we can pause the timer in order if something happened during the turn and we need the time to not pass, the only way is making skip instead of making progress 1.0 and finished, to be cancelled.
I dunno if it does fit what you have in mind for the package but for many use cases you need to keep track of events and finish is not equal to skipped but cancelled might do the work.

P.S: I do understand that I can use the cancel function, but I also think an extra status as skipped may help lot of people like me using this package, cancel should not be removed, if we cannot add an extra status we should mark skip as cancelled

@jay-jay-lama
Copy link
Author

Hello, @amirsaam.
I’m sorry to bring you some unfortunate news.
After a discussion we have next situation: 



  1. The cancel status doesn't align with the timer's logic, so it will be removed. Instead, when you cancel, the timer will revert to the "notStarted" state.

  2. I also can’t add any extra statuses, as that’s not what the timer is designed to handle.



Let me clarify.



The timer has specific functions: 

All the functions allow the timer to change its state between statuses: not started, finished, paused, inProgress. 

Adding additional statuses makes the library implement additional logic, that doesn’t fit initial tasks.

I understand your needs, but this is more about specific logic.
In future there may appear another app and it will need another statuses and logic.
All It will lead to a mess.

As a solution, you could manage these actions within your app or fork the library to customize it for your needs.

@amirsaam
Copy link

amirsaam commented Dec 3, 2024

@jay-jay-lama
I do understand your point about different needs but what I've been saying had a logic that you think that doesn't fit what you are trying to achieve, but still cannot understand cancelled state removal, tbh I don't wanna make tweaks on your package for my own needs and could work it out with cancelled but w/o it, it's not possible because there is no difference between having a timer not started and a one that did and got skipped/cancelled. notStarted is not a good state to be used.
Thanks for hearing what I've said so far, I hope at least you keep cancelled state or reason me about it.

@FulcrumOne
Copy link
Contributor

Hey @amirsaam,

In that case I would recommend you to choose another package that better fit your needs.

Best regards,
Tomasz

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