Skip to content

Navigation Menu

Sign in
Sign up

Improved Notifications: enhanced user interface - #149

Open
brunooss wants to merge 2 commits into
pebble-dev:master from
brunooss:brunooss-notification-enhancement
Open

Improved Notifications: enhanced user interface #149
brunooss wants to merge 2 commits into
pebble-dev:master from
brunooss:brunooss-notification-enhancement

Conversation

@brunooss

@brunooss brunooss commented Mar 1, 2021

Copy link
Copy Markdown

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:

Captura de tela 2021年02月28日 190933

  • Currently, notifications are shown just in the white color.
  • The 'source' attribute is now defining just the icon that will be shown, and the name of the app is defined by the attribute 'sender'
  • Now there's a status bar showing the time during the notifications' viewing.

north20xx reacted with hooray emoji

jwise commented Mar 1, 2021

Copy link
Copy Markdown
Contributor

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 ifdef PBL_RECT stuff -- I think this still probably won't look correct on Chalk (or does it?).
brunooss reacted with thumbs up emoji

brunooss commented Mar 1, 2021

Copy link
Copy Markdown
Author

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! 😅

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The extra spaces here were actually intentional, to make it visually easier to see the parallel between these lines :)

if (curnotif_maxscroll < 0)
curnotif_maxscroll = 0;
int16_t curnotif_nudge = curnotif_maxscroll + NUDGE_HEIGHT;
int16_t curnotif_nudge = curnotif_maxscroll + NUDGE_HEIGHT;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this whitespace also intentional

}

#else /* !PBL_RECT */
#else !PBL_RECT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is that syntactically valid? I guess it builds in CI, so ...

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Whatever is doing this auto-reformatting, please configure it to not do that...


w->curnotif = (size_t) -1;
w->curnotif_nudging = 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does NUDGE_HEIGHT need to change in order to make scrolling look correct?

jwise commented Mar 8, 2021

Copy link
Copy Markdown
Contributor

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jwise jwise jwise left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /