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

Countdown autorepeat #444

Closed
wants to merge 1 commit into from

Conversation

jpdbryant
Copy link

Add an "autorepeat" mode to the Countdown face.

On the countdown setting face, hold "alarm" to toggle autorepeat. When the countdown is set to autorepeat, it will automatically restart when it reaches zero. Useful to e.g. remind you to check on something every twenty minutes.

Changed to use the newer movement_schedule_background_task_for_face method, since we potentially need to schedule the new alarm when the Countdown face is not itself active.

Changed the use of indicators within the Countdown face. Formerly, the "bell" indicator was used to indicate an active countdown. Now, we use the "signal" indicator for this, and the "bell" indicates autorepeat mode. This seems more consistent with those icons' traditional purpose - the "bell" indicates a repeating alarm, the "signal" indicates a one-off.

Copy link
Collaborator

@matheusmoreira matheusmoreira left a comment

Choose a reason for hiding this comment

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

The following commits have already been merged separately and are already in main:

The following merge commit is also unnecessary:

Please rebase the following commit on top of the latest main and then force push your branch:

If main moves forward after you have submitted a pull request, please do not merge main into your branch. Always rebase your changes on top of the latest main instead.

# change current branch to your branch
git checkout your-branch
# rebase commits from current branch on top of main
git rebase main

@jpdbryant
Copy link
Author

Sorry for my terrible pull requests! As you can see I am new to github workflow. I will look at this again in a few days. Thanks for taking the time to untangle what's actually required.

@matheusmoreira
Copy link
Collaborator

It's not terrible at all, it's actually pretty great! I read the code and it seems fine for inclusion and testing. This is a very useful feature and I expect the changes to go straight into next as soon as the requested rebasing is implemented.

This is just to improve the quality of the commit log, and also to make the PR automatically close when next is merged into main. Don't worry about being new or inexperienced. If I need something in order to merge, I'll politely request it here and provide help if needed.

Thank you for your contribution!

@jpdbryant jpdbryant force-pushed the countdown-autorepeat branch from b98b49d to 9ef7bcf Compare September 4, 2024 17:53
@jpdbryant jpdbryant force-pushed the countdown-autorepeat branch from 9ef7bcf to 9258ec1 Compare September 4, 2024 20:56
@matheusmoreira matheusmoreira self-assigned this Sep 7, 2024
matheusmoreira added a commit that referenced this pull request Sep 7, 2024
Adds the ability to automatically restart the timer
in the countdown watch face. This allows users to
easily time repeating events.

Reviewed-by: Matheus Afonso Martins Moreira <matheus@matheusmoreira.com>
GitHub-Pull-Request: #444
@matheusmoreira matheusmoreira added enhancement New feature or request next This feature or pull request is present in the next branch watch-face Related to a new or existing watch face labels Sep 7, 2024
@matheusmoreira
Copy link
Collaborator

Hello! This is not gonna be a blocker or anything, the PR is already in next. I'd like to ask you though: is the time zone code strictly necessary? We might have found a way to simplify it by keeping all logic in the native timezone. What do you think?

@jpdbryant
Copy link
Author

I didn't touch any of the timezone code for this change, I didn't even understand it! I guess the thing to consider is - if you change the timezone while a countdown is running, it shouldn't affect the countdown.

@voloved
Copy link
Contributor

voloved commented Sep 10, 2024

@matheusmoreira
I added this commit for removing timezones: #473

@jpdbryant
Copy link
Author

I found that if I changed the watch time while a countdown was running, it was possible to trigger the end of the countdown, since it's waiting for a calendar time.

In an ideal world, the countdown would not know/care about the actual "current time" at all, and simply count until some number of "ticks since the watch started".

However I don't see a way to do that with Movement currently.

@matheusmoreira matheusmoreira added the main This feature or pull request is present in the main branch label Sep 17, 2024
@matheusmoreira
Copy link
Collaborator

This PR has been merged into main but did not automatically close because it was rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main This feature or pull request is present in the main branch next This feature or pull request is present in the next branch watch-face Related to a new or existing watch face
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants