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

Chimes option #813

Merged
merged 8 commits into from
Jan 4, 2022
Merged

Chimes option #813

merged 8 commits into from
Jan 4, 2022

Conversation

SteveAmor
Copy link
Contributor

Addresses #778

Avamander
Avamander requested changes Nov 7, 2021
View reviewed changes
src/components/datetime/DateTimeController.cpp Outdated Show resolved Hide resolved
src/components/datetime/DateTimeController.cpp Outdated Show resolved Hide resolved
Avamander
Avamander requested changes Nov 7, 2021
View reviewed changes
src/components/datetime/DateTimeController.cpp Outdated Show resolved Hide resolved
src/components/datetime/DateTimeController.cpp Outdated Show resolved Hide resolved
@geekbozu
Copy link
Member

geekbozu commented Nov 7, 2021

Please update the settings version in the settings file for this set of changes.

InfiniTime/src/components/settings/Settings.h

Line 176 in 822b6ae

static constexpr uint32_t settingsVersion = 0x0002;

This will prevent settings version conflicts.

geekbozu
geekbozu approved these changes Nov 7, 2021
View reviewed changes
Copy link
Member

@geekbozu geekbozu left a comment

Choose a reason for hiding this comment

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

Looks great to me

@geekbozu geekbozu linked an issue Nov 10, 2021 that may be closed by this pull request
Vibrate on hour and half hour #778
Closed
1 task
NeroBurner
NeroBurner requested changes Nov 22, 2021
View reviewed changes
src/displayapp/screens/settings/SettingChimes.cpp Outdated
@@ -0,0 +1,88 @@
#include "SettingChimes.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#include "SettingChimes.h"
#include "displayapp/screens/settings/SettingChimes.h"

could you make the include relative to the src directory as done in the (not yet merged, but approved) PR #805

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@geekbozu
Copy link
Member

geekbozu commented Dec 1, 2021

So I've been using this for a while now, Id love something to differentiate it from a normal notification, Like more buzzes or a weird pattern or something. Might not be doable in this PR but some feedback. I'm always thinking I have a notification when it goes off :D

@SteveAmor
Copy link
Contributor Author

So I've been using this for a while now, Id love something to differentiate it from a normal notification, Like more buzzes or a weird pattern or something. Might not be doable in this PR but some feedback. I'm always thinking I have a notification when it goes off :D

I thought about a different notification method but didn't want to insert blocking code 'pulsing the motor a couple of times.

FintasticMan added a commit to FintasticMan/InfiniTime that referenced this pull request Dec 13, 2021
@FintasticMan
PR InfiniTimeOrg#813: Chimes option
ec8fb4a 
Squashed commit of the following:

commit c5282f9
Author: SteveAmor <SteveAmor@users.noreply.github.com>
Date:   Sun Nov 7 14:15:39 2021 +0000

    update settings version

commit 822b6ae
Author: SteveAmor <SteveAmor@users.noreply.github.com>
Date:   Sun Nov 7 13:12:01 2021 +0000

    format changes

commit 37de10f
Author: SteveAmor <SteveAmor@users.noreply.github.com>
Date:   Sun Nov 7 12:10:23 2021 +0000

    applied clang-format

commit 5932391
Author: SteveAmor <SteveAmor@users.noreply.github.com>
Date:   Sun Nov 7 10:50:33 2021 +0000

    Chimes option
@lman0
Copy link

lman0 commented Dec 15, 2021

is this pull can be merged by @JF002 , @geekbozu ?
do you have other part to add @SteveAmor ?

@trman
Copy link

trman commented Jan 1, 2022 •
edited
Loading

any news @SteveAmor @geekbozu @Avamander ?
i do like this pr as it something i have activated in my ex peeble and wish as well to see here....

@geekbozu
Copy link
Member

geekbozu commented Jan 1, 2022

@SteveAmor if you could fix the now existing conflicts... :)

@lman0 @trman Once we approve them its up to @JF002 to merge them. All we can do is guide him. He will get there when his schedule allows and on his priority

@SteveAmor
Copy link
Contributor Author

@geekbozu I have updated the conflicting files in commit 94561b2. I thought GitHub would realise and remove the above message that says This branch has conflicts that must be resolved but it is still there.

@Avamander
Copy link
Collaborator

@SteveAmor It's not GitHub, it's Git. There's unresolved conflicts still, make a merge commit or rebase.

@JF002 JF002 added this to the 1.8.0 milestone Jan 3, 2022
JF002
JF002 requested changes Jan 3, 2022
View reviewed changes
Copy link
Collaborator

@JF002 JF002 left a comment

Choose a reason for hiding this comment

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

It looks pretty good. Could you have a look at the type of chimeState (enum classs instead of int) ?

src/components/settings/Settings.h Outdated Show resolved Hide resolved
@JF002 JF002 merged commit b8b54f4 into InfiniTimeOrg:develop Jan 4, 2022
@SteveAmor SteveAmor mentioned this pull request Jan 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NeroBurner NeroBurner NeroBurner requested changes

@Avamander Avamander Avamander approved these changes

@geekbozu geekbozu geekbozu approved these changes

@JF002 JF002 JF002 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.8.0
Development

Successfully merging this pull request may close these issues.

Vibrate on hour and half hour
7 participants
@SteveAmor @geekbozu @lman0 @trman @Avamander @JF002 @NeroBurner