-
Notifications
You must be signed in to change notification settings - Fork 327
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
add basic button support #164
base: master
Are you sure you want to change the base?
Conversation
Oh hey interesting! It looks like we were both thinking the same thing! I also just submitted #165 😄 I think your PR has an interesting implementation, because it lives alongside core functionality without overriding anything. Ultimately, both changes can be merged without conflicts, which would give users the option to override specific buttons, or the implementation as a whole. How would you feel about also adding a fourth button function, and putting Also, how would you feel about following the existing function names a little closer, and calling them something like |
I like all of this feedback, I was wondering what to call the buttons.
It does make sense to let the user override the menu even if normally they
shouldn't 😛
…On Thu, May 12, 2022, 6:27 AM Maxwell Pray ***@***.***> wrote:
Also, how about following the button numbers in the Getting Started doc?
[image: image]
<https://user-images.githubusercontent.com/820984/168064421-d470830a-3877-47d5-8df8-725c58486c59.png>
—
Reply to this email directly, view it on GitHub
<#164 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACAM657QNVL5VS4TGUXJZFDVJTTILANCNFSM5VWMS7WA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the aforementioned suggestions as a review 👍
It does make sense to let the user override the menu even if normally they
shouldn't 😛
A relatable example for this could be SW1's "confirm" functionality. If a watch face has a menu of its own (i.e. themes or backgrounds), not being able to hit the same button to confirm the menu selection could be a UX smell. Additionally, if the user presses the "confirm" button thinking that it would confirm a menu (as the doc states), and a second menu gets brought up without "closing" the first one, they might get lost in what just happened.
@@ -114,6 +114,7 @@ void Watchy::handleButtonPress() { | |||
} else if (guiState == FW_UPDATE_STATE) { | |||
showMenu(menuIndex, false); // exit to menu if already in app | |||
} else if (guiState == WATCHFACE_STATE) { | |||
button1(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
button1(); | |
handleButton2Press(); |
This lines up with the function naming pattern that already exists, handleButtonPress
.
Also, this is SW2 on the PCB, and here is SW2 documented in the Getting Started guide: https://watchy.sqfmi.com/docs/getting-started/
@@ -126,6 +127,7 @@ void Watchy::handleButtonPress() { | |||
} | |||
showMenu(menuIndex, true); | |||
} else if (guiState == WATCHFACE_STATE) { | |||
button2(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -138,6 +140,7 @@ void Watchy::handleButtonPress() { | |||
} | |||
showMenu(menuIndex, true); | |||
} else if (guiState == WATCHFACE_STATE) { | |||
button3(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void Watchy::button1() | ||
{ | ||
} | ||
void Watchy::button2() | ||
{ | ||
} | ||
void Watchy::button3() | ||
{ | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will help keep the style consistent:
void Watchy::button1() | |
{ | |
} | |
void Watchy::button2() | |
{ | |
} | |
void Watchy::button3() | |
{ | |
} | |
void Watchy::handleButton2Press() {} | |
void Watchy::handleButton3Press() {} | |
void Watchy::handleButton4Press() {} | |
virtual void button1(); // override these methods to handle different non-menu button presses | ||
virtual void button2(); | ||
virtual void button3(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual void button1(); // override these methods to handle different non-menu button presses | |
virtual void button2(); | |
virtual void button3(); | |
virtual void handleButton2Press(); // override these methods to handle different non-menu button presses | |
virtual void handleButton3Press(); | |
virtual void handleButton4Press(); |
This is my 2 cents - maybe instead of referrng to buttons by numbers in the functions names we could refer to buttons by their default functionality? e.g. handleButtonBackPress, handleButtonMenuPress, handleButtonUp/DownPress Really like this approach though! ❤️ |
This would create confusion if the dev/user wanted to swap the handedness of the watch. It would be more appropriate to use a clear coordinate naming system, such as handleButtonUpperLeft, handleButtonUpperRight, handleButtonLowerLeft, handleButtonLowerRight. This also creates a coherency issue with vertical flips, but is tied semantically to the physical orientation of the device, without making unnecessary assumptions about use case. |
Is it possible to use this to update the watch face? I've updated the .cpp and .h file with the
Then the Watchy vibrates when I press the button. But how can I get the display to change when a button is pressed? Code like this doesn't work:
Is there something else I need to do to get the screen to update? |
@edent judging from this line of code: https://github.com/sqfmi/Watchy/blob/master/src/Watchy.cpp#L610
|
I added basic support for handling three of the four buttons when the watch is in Watchface mode.
This can be an example for others, or I'm happy to modify to make it part of the core base, as it is nice to use the buttons to change settings on some watchfaces.
Enjoy! Thanks for the fun product.