Improved Notifications: enhanced user interface #149
Conversation
|
The visual changes are great! Looks like your editor went through and automatically reformatted the source (?), which makes the diff very difficult to read. Can you disable that behavior in your editor? Also, it looks like you commented out the |
|
You're absolutely right, @jwise; In my recent commit, I managed to fix that topics that you mentioned. In fact, those were things that I made while debugging, and forgot to turn it back to normal! 😅 |
| @@ -276,10 +280,10 @@ static void _up_single_click_handler(ClickRecognizerRef _, void *_w) { | |||
|
|
|||
| static void _notification_window_click_config_provider(NotificationWindow *w) { | |||
| window_single_repeating_click_subscribe(BUTTON_ID_DOWN, 500, _down_single_click_handler); | |||
| window_single_repeating_click_subscribe(BUTTON_ID_UP , 500, _up_single_click_handler ); | |||
| window_single_repeating_click_subscribe(BUTTON_ID_UP, 500, _up_single_click_handler); | |||
There was a problem hiding this comment.
The extra spaces here were actually intentional, to make it visually easier to see the parallel between these lines :)
| int16_t curnotif_maxscroll = w->curnotif_height - VIEWPORT_HEIGHT; | ||
| if (curnotif_maxscroll < 0) | ||
| curnotif_maxscroll = 0; | ||
| int16_t curnotif_nudge = curnotif_maxscroll + NUDGE_HEIGHT; | ||
| int16_t curnotif_nudge = curnotif_maxscroll + NUDGE_HEIGHT; |
There was a problem hiding this comment.
this whitespace also intentional
| @@ -249,6 +237,6 @@ static void single_notification_layer_update_proc(Layer *layer, GContext *ctx) { | |||
| szrect.size.h -= outsz.h + ELEMENT_PADDING; | |||
| } | |||
|
|
|||
| #else /* !PBL_RECT */ | |||
| #else !PBL_RECT | |||
There was a problem hiding this comment.
Is that syntactically valid? I guess it builds in CI, so ...
| szrect.origin.y += APPNAME_PADDING; | ||
| GPoint(szrect.origin.x, szrect.origin.y), | ||
| GPoint(szrect.origin.x + szrect.size.w, szrect.origin.y)); | ||
| szrect.origin.y += 0; | ||
| szrect.size.h -= APPNAME_PADDING; |
There was a problem hiding this comment.
Does this szrect.size.h also need to be changed?
| szrect.origin.x = 0; | ||
| szrect.origin.y = 0; | ||
|
|
||
| szrect.origin.y = STATUS_BAR_LAYER_HEIGHT; |
There was a problem hiding this comment.
Hmm, wait, is this correct when notifications start scrolling? I think this needs to be 0, and the offset should be specified in notification_window, right? Otherwise, the secondary notification that you scroll up into will start too low on the screen?
| default: | ||
| /* we don't care */ | ||
| ; | ||
| case TimelineAttributeType_Sender: sender = (const char *)a->data; break; |
There was a problem hiding this comment.
Whatever is doing this auto-reformatting, please configure it to not do that...
| w->uuids = NULL; | ||
| w->nuuids = 0; | ||
|
|
||
|
|
||
| w->curnotif = (size_t) -1; | ||
| w->curnotif_nudging = 0; |
There was a problem hiding this comment.
Does NUDGE_HEIGHT need to change in order to make scrolling look correct?
|
I left a bunch of comments (and then saw afterwards that you manually put the formatting back -- sorry for being nitpicky on that!). I think the only thing that I'd really like to see is confirmation that this works right when scrolling through multiple notifications. Having the status bar there will be great, especially for being able to put notification counters up there as we scroll (like PebbleOS) does. Thanks so much again! And I'm also sorry that this took me so long to review again... |
This is an improvement of the RebbleOS notifications, according to a prototype from Lavender Glaab that I've seen in the Discord Group.
Here is how it looks like: