GeezerVison

I think the original author is properly credited: commit author is still Simon Willshire, and I'm mentioned as co-author in the commit, which is also shown correctly in the GitHub overview

And I like clean commits which work by itself.

fixed code formatting errors and rebased on current develop branch

I think the original author is properly credited: commit author is still Simon Willshire, and I'm mentioned as co-author in the commit, which is also shown correctly in the GitHub overview

Ah ok, I didn't see @Tiggilyboo's profile picture by the commit, so I thought they weren't a co-author.

Nice work, testing it on my watch, so far everything's stable and working as expected!

If I have notifications like this:
[3/3] oldest
[2/3]
[1/3] newest
If I swipe on the second one, it will change to the oldest notification, but the animation direction makes it look like it is coming from below, which I think is the wrong direction.

The PineTime LCD drawing is very slow and not well suited to display movement without making use of hardware scrolling. There are only a few frames of animation using this method. It would be nice if the sideways animation that's used between QuickSettings and watchface could be used here too.

There's also this old notification dismissal pull which is probably worth examining if that hasn't been done already #323.

fixed the direction a new notifications is moved in. Didn't touch the swipe animation as quite like the swiping animation when dismissing the notification. Would it be a show stopper to keep the little laggy animation?

Thanks to InfiniTimeOrg/InfiniSim#37 the screen transitions can also be shown in the simulator

fixed the direction a new notifications is moved in. Didn't touch the swipe animation as quite like the swiping animation when dismissing the notification. Would it be a show stopper to keep the little laggy animation?

Thanks to InfiniTimeOrg/InfiniSim#37 the screen transitions can also be shown in the simulator

Agree, also I quite like the animation. Is the hardware scrolling only limited to the y direction? Will take a look and see for later MR.
I had a crash a little while back but it was from one of the earlier commits, unsure of causes as of yet. Will update to this commit and try to replicate.

Good work!

I wouldn't say I like the laggy animation, but perhaps it's passable. I'd personally like for it to be changed soon after merging though if we don't change it beforehand. There is only hardware based vertical scrolling. This is why the horizontal scrolling is done in stripes.

I recently also had a crash related to notifications, but can't remember what branch I was running. It happened when notifications were already open, I think as history and not a popup, I think the screen was also on and I received a new notification.

@Riksu9000 Would enabling VSync help make the animation more seamless?

Alternatively we could cheat by displaying a small thrash pop-up on top of the current notification and then use vertical scroll to go to the next notification.

See #1186 for smooth horizontal animations.

Edit: never mind it's not a solution.

The easiest solution for the transition would be to use the "swipe right stripey" animation (the one that is used from clock to quicksettings) to "disintegrate" the current notification and instantly show the new one. Then the checks if a new message is shown to swipe in from below or of the top of the screen could be removed as well.

Another a bit more complicated way would be to "disintegrate" onto a black new screen, which in turn then swipes to the new message (from below or top).

The id field seemed to me like a way to have the ring buffer together with the 1/5 notification message, which I didn't need anymore after the restructure. As there was more to the id field, I'll need to re-add it again

The At() function was structured to work like the at() function of std::vector (or other random access stl-containers). If we don't want the reference a rename to Get() feels good for me (because I would expect a function named similar to a stl-container to do a similar thing)

uh, yes sorry, I need to rename those internal variables 😅

I like code reviews. They take effort and time, but the outcome is always better code and programmers who learned something new :)

ready for re-review

• dismiss is done by Notification::Id to really dismiss the shown message instead of the one at the current viewed index
• instead of the swipe animation use the RightAnim screen transition for more smoothness

Thanks @NeroBurner. The result looks quite good, IMO : https://video.codingfield.com/w/p4kXKd3Bp9co4C6yvKvjQR

But I'm still a bit troubled by the new NotificationManager : it still exposes methods with "indexes" in the public API, which is against the encapsulation goal of the original implementation.

In the original implementation, my goal was to provide a high level API to the Notification screen:

• Get the most recent notification
• Get the oldest notification
• Get the next/previous notification relative to a given notification.
  Such an interface allows to completely hide the underlying implementation. When developing the Notification app, you don't need to know that the NotificationManager stores the notification in a buffer, a queue, a circular buffer, a hashmap or anything else. You don't even need to know what a Notification ID is. It's up to the notification manager to ensure that notifications are returned in the correct chronological order.

In my mind, the only change needed to add support for notification dismissal should be to add a method Dismiss(ID) to NotificationManager.

There might be good reasons to re-write a lot of the internals of NotificationManager, but I can't see them. Was the original implementation not adapted to this new use-case? Was it too complex or too difficult to understand?

However, I really think we should keep the encapsulation level as it was in the original implementation.

However, I really think we should keep the encapsulation level as it was in the original implementation.

Could we do that refactor in a follow-up PR?

I've got a few hours today to see how much I can rewrite the encapsulation. The reason for the rewrite was the problems with the dismiss and reordering of the notifications after a dismiss. And the dependence of the "id" to the displayed index (the notification 1/5 part on the display).

A great part of the rewrite at the Notifications.h part is due to keeping track of the current index of the displayed message. And then it is much more straightforward to access the same index (like 3/5 to 3/4), or one decremented when at the oldest message (like 5/5 to 4/4).

Could we do that refactor in a follow-up PR?

Yes, probably :) And I'm even willing to write such PR, as I have a clear view of what I would like to achieve (and probably fail at explaining it).

I've got a few hours today to see how much I can rewrite the encapsulation. The reason for the rewrite was the problems with the dismiss and reordering of the notifications after a dismiss. And the dependence of the "id" to the displayed index (the notification 1/5 part on the display).

A great part of the rewrite at the Notifications.h part is due to keeping track of the current index of the displayed message. And then it is much more straightforward to access the same index (like 3/5 to 3/4), or one decremented when at the oldest message (like 5/5 to 4/4).

Thanks for the explanation! So yes, indeed, changes in NotificationManager were indeed needed! I didn't realize that when reading the PR.

That was easier than I expected 🎉

I still needed to introduce two new functions in total:

• Dismiss(id)
• IndexOf(id): return the index of the notification with the specified id

This way Notification.h doesn't need to keep track of the currentIdx. The index can't be used for anything else, because the index based functions are all private

Edit: it's three new functions

• Get(id): get the notification with the specified id

@NeroBurner Thank you for this great feature! Could you consider to make the dismiss function also work for incoming notifications? (The screen that you see when you just received a notification and which shows is a time/progress bar until it goes back to the current app.)

@devnoname120 please create a separate issue (or maybe one is already open) and ping me there

It should be possible

@NeroBurner Someone else created an issue in the mean time: #1232