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.

Sign up for GitHub

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

Jump to bottom

Alarm: Display the time until alarm, even if it is off #698

Closed
wants to merge 3 commits into from
Closed

Alarm: Display the time until alarm, even if it is off #698

wants to merge 3 commits into from

Conversation

maksalees
Copy link
Contributor

Previously, if the alarm was turned off, when you pressed the information button, a message was displayed that the alarm was not set. The amount of time until the alarm is now displayed as well as if it is on.
What for? Allows you to save two clicks on a button if you just need to see how much time is left before the alarm

Before

Before

After

After

@maksalees
Alarm: Display the time until alarm, even if it is off
77bc346 
Previously, if the alarm was turned off, when you pressed the information button, a message was displayed that the alarm was not set. The amount of time until the alarm is now displayed as well as if it is on.
What for? Allows you to save two clicks on a button if you just need to see how much time is left before the alarm
Riksu9000
Riksu9000 reviewed Sep 29, 2021
View reviewed changes
Copy link
Contributor

@Riksu9000 Riksu9000 left a comment

Choose a reason for hiding this comment

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

Looks like the alarmTime member might not be needed anymore and could be replaced with the new function. What do you think?

src/components/alarm/AlarmController.cpp Outdated Show resolved Hide resolved
src/components/alarm/AlarmController.h Outdated Show resolved Hide resolved
@maksalees maksalees force-pushed the display-time-until-alarm-even-if-is-off branch from 616ee1e to 25034cc Compare September 29, 2021 16:27
@maksalees
Copy link
Contributor Author

Looks like the alarmTime member might not be needed anymore and could be replaced with the new function. What do you think?

I agree, I just forgot to fix it 🙂

@mruss77
Copy link
Contributor

mruss77 commented Oct 1, 2021

Looks good to me!

@geekbozu geekbozu added enhancement Enhancement to an existing app/feature in test labels Oct 3, 2021
@Avamander Avamander removed the in test label Oct 3, 2021
@Itai-Nelken Itai-Nelken mentioned this pull request Oct 22, 2021
Closed
1 task
@Riksu9000 Riksu9000 mentioned this pull request Oct 27, 2021
Closed
1 task
@Itai-Nelken Itai-Nelken mentioned this pull request Dec 1, 2021
Draft
@Itai-Nelken
Copy link
Contributor

I have been uaing this pr for some time, works fine and makes using the alarm for measuring time until something easier (don't have to turn alarm on every time).

FintasticMan added a commit to FintasticMan/InfiniTime that referenced this pull request Dec 13, 2021
@FintasticMan
PR InfiniTimeOrg#698: Alarm: Display the time until alarm, even if it…
cbc158a 
… is off

Squashed commit of the following:

commit 874e1af
Author: Maxim Leshchenko <cnmaks90@gmail.com>
Date:   Wed Sep 29 20:22:58 2021 +0300

    Sorry 😂

commit 25034cc
Author: Maxim Leshchenko <cnmaks90@gmail.com>
Date:   Wed Sep 29 19:25:25 2021 +0300

    Fixes based on code review

commit 77bc346
Author: Maxim Leshchenko <cnmaks90@gmail.com>
Date:   Tue Sep 28 13:16:50 2021 +0300

    Alarm: Display the time until alarm, even if it is off
    Previously, if the alarm was turned off, when you pressed the information button, a message was displayed that the alarm was not set. The amount of time until the alarm is now displayed as well as if it is on.
    What for? Allows you to save two clicks on a button if you just need to see how much time is left before the alarm
@Itai-Nelken Itai-Nelken mentioned this pull request Dec 19, 2021
Closed
1 task
@maksalees
Copy link
Contributor Author

Will this PR be merged?

@FintasticMan
Copy link
Member

Well at the moment it is based on quite an old revision of the develop branch, and it also has some conflicts. If you can rebase your branch onto the latest develop, it would be easier to see if it can be merged.

@Riksu9000
Copy link
Contributor

@maksalees I'm sorry it's been a long time. Probably the reason why this hasn't been merged is that we're unsure about what's the preferred behaviour. Indeed rebasing this will allow us to take another look at this.

@maksalees maksalees mentioned this pull request Sep 30, 2022
Open
@maksalees
Copy link
Contributor Author

I created a new rebased PR #1346

@maksalees maksalees closed this Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Riksu9000 Riksu9000 Riksu9000 left review comments

Assignees
No one assigned
Labels
enhancement Enhancement to an existing app/feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@maksalees @mruss77 @Itai-Nelken @FintasticMan @Riksu9000 @geekbozu @Avamander