settings_quick_launch: new combo back + up#707
settings_quick_launch: new combo back + up#707asyba wants to merge 1 commit intocoredevices:mainfrom
Conversation
|
@asyba Thank you so much for doing this. Having a couple more quick-launch options that can be performed without even needing to look down at the watch would be so useful. I only own a Pebble 2 Duo right now. Have you tried this with a Round Pebble? Obviously, with the rectangular Pebble model this quick-launch pinch combo makes sense, as the buttons are opposite each other. I just wonder how it feels on the round Pebbles, with the back button in the center of the watch, and whether it works as well? I had forgotten that using select-back is the combo for reset, so using that combo for this quick-launch on the Round may not be possible. (It also probably helps to be consistent across models). As far as I am aware, these two system combos are currently used by PebbleOS itself:
Are there any others to be aware of? I concede that a multi-button quick-launch combo is probably not a feature that normal users are likely to ever use, not that it matters if they don't. Pebble watches tend to appeal more to geeky types, and those types of users are likely to find these really useful. I do hope that this gets merged. |
|
Also, how does this implementation adapt for left-handed users, who wear their Pebble on their right wrist, with watch and screen flipped? In this orientation, the combo would probably want to be back+down, (rather than still back+up) so the buttons remain opposite each other. |
|
|
||
| void quick_launch_app_menu_window_push_combo(void) { | ||
| QuickLaunchAppMenuData *data = app_zalloc_check(sizeof(*data)); | ||
| data->is_tap = true; |
There was a problem hiding this comment.
Setting is_tap = true for a hold-combo is confusing. Consider adding a comment explaining why, or restructuring the filter logic to be clearer.
src/fw/shell/normal/watchface.c
Outdated
| // Reset recognizer state to prevent stale state | ||
| ClickRecognizer *recognizer = &s_click_manager.recognizers[button_id]; | ||
| recognizer->is_button_down = false; | ||
| recognizer->number_of_clicks_counted = 0; |
There was a problem hiding this comment.
Directly manipulating ClickRecognizer internal state is fragile. If button up events arrive after this reset, it could cause state corruption.
There was a problem hiding this comment.
@jplexer I was having problems where, after successfully executing the combo, the quick launches would generally be messed up. This helped me reset them and fix them.
I just try instead call click_manager_reset(&s_click_manager);, I did a quick test and it worked fine.
I also tried removing both and it works fine... maybe none reset is necessary.
Although I do notice the animations are slower without any kind of reset, I don't know if that's just my imagination from so much testing, haha.
e4a359f to
ca39c78
Compare
ca39c78 to
73b374b
Compare
|
@jplexer can you review |
src/fw/shell/normal/watchface.c
Outdated
|
|
||
| if (!quick_launch_single_click_is_enabled(button)) return; | ||
| //check if quick launch app is not timeline | ||
| if (quick_launch_single_click_get_app(click_recognizer_get_button_id(recognizer)) != APP_ID_TIMELINE) { |
There was a problem hiding this comment.
nit: button is now extracted on line 124 but the function body still calls click_recognizer_get_button_id(recognizer) three more times instead of using it.
src/fw/shell/normal/watchface.c
Outdated
| // This ensures only the combo executes, not individual hold handlers | ||
| if (s_click_manager.recognizers[BUTTON_ID_BACK].hold_timer != NULL) { | ||
| app_timer_cancel(s_click_manager.recognizers[BUTTON_ID_BACK].hold_timer); | ||
| s_click_manager.recognizers[BUTTON_ID_BACK].hold_timer = NULL; |
There was a problem hiding this comment.
Reaching directly into s_click_manager.recognizers[].hold_timer is fragile, it bypasses the recognizer's own state machine and could leave fields like is_held stale. There's no per-recognizer cancel API (only click_manager_reset() which resets all buttons), so this is the only option short of adding one. Might be worth adding a click_recognizer_reset() to click_internal.h and using it here.
|
sorry! this one got away from me |
|
@saltedlolly no I don't have round pebble or others. |
Signed-off-by: Federico Bechini <federico.bechini@gmail.com>
73b374b to
8332430
Compare
| @@ -54,6 +55,8 @@ static const char *s_row_titles[NUM_ROWS] = { | |||
| [ROW_HOLD_DOWN] = i18n_noop("Hold Down"), | |||
| /// Shown in Quick Launch Settings as the title of the hold back button quick launch option. | |||
| [ROW_HOLD_BACK] = i18n_noop("Hold Back"), | |||
| /// Shown in Quick Launch Settings as the title of the hold back+up button combination option. | |||
| [ROW_HOLD_BACK_UP] = i18n_noop("Hold Back + Up"), | |||
There was a problem hiding this comment.
fyi, we'd only merge this as a phone-side setting (meaning this menu entry would be removed and the corresponding pref would be added to the settings whitelist at src/fw/services/normal/blob_db/settings_blob_db.c)
There was a problem hiding this comment.
got it, Is there a set method for deciding this, or is it done on a case-by-case basis?
For example, will each new configuration be automatically added to mobile-only, or will some other existing old configurations also be moved to mobile-only?
There was a problem hiding this comment.
currently its new configs will be mobile-only but it can be decided on a case by case basis :)
Added a new Quick Launch button using a Hold with BACK+UP.