Dismiss notifications by swiping right

[NeroBurner]

Original code by @Tiggilyboo at #1150 rebased on current develop branch. I just reworked the ringbuffer and squashed to one clean commit

---

Rework notification ring buffer to have a begin-idx and a size_ internally. And add a new interface to access the notifications by index. The 0 index is always the latest notification.

The second new function is a Dismiss function to remove the notification with a specified idx from the buffer.

Fixes: #176

[FintasticMan]

Could you keep the original commits, so that then the original author is credited properly?

[NeroBurner]

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.

[NeroBurner]

fixed code formatting errors and rebased on current develop branch

[FintasticMan]

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.

[Tiggilyboo]

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

[Riksu9000]

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.

TRIM_20220611_173137.mp4

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

[NeroBurner]

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

[Tiggilyboo]

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!

[Riksu9000]

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.

[devnoname120]

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

[devnoname120]

See #1186 for smooth horizontal animations.

Edit: never mind it's not a solution.

[JF002]

Thanks for this PR, this feature will make a lot of users happy ;-)

There are a few points I would like to discuss before merging.

I have to admit that the sliding animation looks far better than what I would have expected knowing that the whole screen is refreshed at every frame. However, this is still not on par with other animations in the project (the vertical animation is smooth thanks to the hardware scrolling effect, and the left/right animations we use for the quick menu hides the slowness of the display). On one hand, I like this animation because it's similar to what we are used to on other touch devices, and on the other side, it's still a bit laggy. I would like to keep the UI of InfiniTime as smooth as possible, so I would appreciate if we at least tried to improve this one.

The changes in the struct NotificationManager::Notification. In the original code, the struct contained a field id. Its purpose was to uniquely identify a notification regardless of it position in the buffer. I added this field knowing that in the future, we will want to remove a specific notification either via BLE or the UI. As notification are received asynchronously, a new notification could be received while another one is being removed, which could result in the wrong notification being deleted. The purpose of this id field was to ensure that the correct notification is removed.
The new implementation removed that field and also mixes the concept of id (identifier) and index (position in the buffer). I think this implementation could be reviewed to re-add the concept of id : each notification has an unique ID, and, when the Notification app wants to delete a notification, it passes the id of the notification instead of the index of the notification in the buffer.

Note : when writing this comment, I noticed that NotificationManager does not seem to be correctly protected against concurrent accesses from BLE and the UI (and was not before this PR)... that's probably something to keep in mind if we encounter crashes.

Notice that GetNext(), GetPrevious() and GetLastNotification() return the Notification by value and I did it on purpose : If they returned a ref to the notification, the content of notification could be changed/deleted by a new notification received by BLE while it's currently being processed in the Notification app. I think the new methods At() should do the same. I also find the name At() a bit confusing and inconsistent. Couldn't we name this method Get() ?

Regarding the code formatting, the variables begin_idx and read_idx and index_ do not follow the coding conventions of the project.

Well, I hope I didn't discourage you with this rather long review, and feel free to discuss those comments if you do not agree with me ;-)

[NeroBurner]

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

[NeroBurner]

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

[JF002]

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.

[Avamander]

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?

[NeroBurner]

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

[JF002]

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

[JF002]

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.

[NeroBurner]

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

[Riksu9000]

This'll be a great addition to the next release. The fast animations look great.

[JF002]

It looks really good to me, thanks for making the extra effort on this PR, @NeroBurner

[devnoname120]

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

[NeroBurner]

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

It should be possible

[devnoname120]

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