Dismiss Notifications (Swipe right) #1150

Tiggilyboo · 2022-05-19T16:19:48Z

Hello all,

I've implemented dismissal of a notification by swiping right on each. This triggers an animation which throws the animation offscreen, then renders the next available notification.

src/displayapp/screens/Notifications.cpp

Riksu9000

Usually a PR should be focused on one topic or a single feature for example. Some of the changes here should be split off to at least one more PR.

Tiggilyboo · 2022-05-19T18:08:09Z

Usually a PR should be focused on one topic or a single feature for example. Some of the changes here should be split off to at least one more PR.

No problem, I have made this PR into the notifications dismissal and now creating the other watch face related one (here: #1151)

Itai-Nelken · 2022-05-19T18:38:24Z

I found a bug:
If i send 1 notification and dismiss it, the 'no notifications' "notification" shows and the counter is 0/0 as it should be.
but if I send another notifications and dismiss it, a black screen shows with the counter at 1/2.
sending and dismissing another notification causes the counter total to be incremented.
here is a video:

pt_notif_dismiss_bug.mp4

Tiggilyboo · 2022-05-19T18:43:25Z

Nice catch, taking a look at this now. Thanks @Itai-Nelken
EDIT: Odd, it does not happen on InfiniSim, and for some reason my OTA image is not flashing (Pinecone gets to the top after 100%, screen blanks and then it goes back to pinecone. Assumedly the firmware is not happy, and reverts back to verified image).

I have pushed a further change below which fixed an issue with the array assignment when dismissing. I think this should fix your issue with blank notifications.

NeroBurner · 2022-05-19T19:04:20Z

Happened for me in InfiniSim (create a notification, swipe it away, go back to watchface, swipe down to get to notifications, see total counter is still counted up)

with your fix it doesn't happen on InfiniSim anymore

- Cleaned up animation delay value (one place to configure in header)

NeroBurner

didn't get through all of it, but ran out of time. Submitting notes up until now

src/components/ble/NotificationManager.cpp

src/displayapp/screens/Notifications.cpp

Itai-Nelken · 2022-05-19T19:53:41Z

Mostly fixed. Now if I send 5 notifications and then dismiss them all, same thing happens.

NeroBurner · 2022-05-19T19:56:13Z

found a dismissal problem. Steps to reproduce

start simulator
hit n three times
dismiss the second (2/3) notification
swipe down to see the oldest notification with wrong index 3/2

Doesn't happen if I swipe away the first 1/3 notification. Then the remaining notifications read 1/2 and 2/2

NeroBurner · 2022-05-19T21:25:49Z

Found a complication, which needs to be handled: The notifications array is used as a ring buffer

In the beginning readIndex and writeIndex are 0
after the first push readIndex is still 0, and writeIndex is 1
writeIndex will always be one more advanced than readIndex
after the fifth insert readIndex will be 4 and writeIndex has wrapped to be 0
with the next push we're at readIndex is 0, and writeIndex is 1

src/components/ble/NotificationManager.cpp

- Not very pretty, but it seems to work well: reassign id's after removing a notification in order to preserve index calculation for GetPrevious and GetNext. - Fix index jumping after dismissal, only use GetPrevious or GetLastNotification calls after dismiss.

Tiggilyboo · 2022-05-21T02:26:24Z

found a dismissal problem. Steps to reproduce

* start simulator

* hit `n` three times

* dismiss the second (2/3) notification
  ![InfiniSim_2022-05-19_195359](https://user-images.githubusercontent.com/9076163/169392869-666eafec-4538-4d33-a7f8-0039fd5035b9.png)

* swipe down to see the oldest notification with wrong index `3/2`
  ![InfiniSim_2022-05-19_195408](https://user-images.githubusercontent.com/9076163/169392947-bd035e95-600e-4017-a5a6-52e8e43e654a.png)

Doesn't happen if I swipe away the first 1/3 notification. Then the remaining notifications read 1/2 and 2/2

I think I've fixed this now in my latest commit. Sad news, it's really not the nicest work around. As you've also pointed out, it uses a ring buffer, which means the tail and head are shifting about and indexes and the indexes that are displayed for each notification depends on an ever increasing id. In addition the readIndex and writeIndex now encorperates decrement logic to follow the ring buffer shenanigans.

EDIT:
So my solution was to decrement each valid notification id that is above the dismissed notification id. Afterward, reassign the nextId incrementation based on the highest last id (aka. notifications[readIndex].id + 1).
It now seems to be behaving predictably.

- Try to use more LV_HOR_RES / LV_VER_RES where applicable in place of magic 240's - Revert page to lv_container as the styling parts were not working with LV_PAGE_PART_BG (background colour, padding, border etc.).

Itai-Nelken · 2022-05-22T18:53:51Z

Works well for me.

src/components/ble/NotificationManager.cpp

NeroBurner · 2022-05-30T21:11:24Z

Found a way to break the current code:

start simulator
hit n 7 times (create seven notifications)
swipe down to get to the notifications
swipe away two times
0/0 notifications shown

Tiggilyboo · 2022-05-31T20:13:06Z

Found a way to break the current code:

* start simulator

* hit `n` 7 times (create seven notifications)

* swipe down to get to the notifications
  ![showing 1/5 notifications before swiping away](https://user-images.githubusercontent.com/9076163/171059433-a7d24080-2f30-471e-9231-b0a3d648b8f6.png)

* swipe away two times

* 0/0 notifications shown
  ![showing 0/0 notifications](https://user-images.githubusercontent.com/9076163/171059309-b4becc97-c6a4-48ce-af23-7457d51520fd.png)

Having a go at this... This is tripping me up quite a bit!
The issue also happens for n == 6.

It seems that:

n == 7: readIndex = 1
dismiss 1: readIndex = 0
dismess 2: readIndex loops back to 4.
Then notifications[4] is an empty element, and shows it. As notifications[TotalNbNotifications - 1] = {} every dismiss. Which seems to be the problem...

In order to fix this, I need to be able to dismiss based on readIndex's position, not just from dismissed element to end of array... I think.

11111  // n == 7
 ^
11110  // dismiss x1
^
11100  // dismiss x2
    ^

Cooking something up... If anyone has any ideas, let me know!

NeroBurner · 2022-06-01T19:18:16Z

I've created a SmallRingBuffer class to hide the ringbuffer index handling inside. And outside the index from 0 to 4 can be requested. You're welcome to test it :)

~~It needs some restructure and cleanups, but I think it is working~~

edit: cleaned up: NeroBurner@ef0facf

NeroBurner · 2022-06-05T16:14:25Z

@Tiggilyboo I liked the swipe to dismiss feature so much I opened a new PR #1173 with the code from this PR, rebased it and added the ring-buffer rework. Is it ok for you to move to the new PR, or would you like to update this PR instead and champion the changes yourself? Either one is fine by me, I just want the notification dismissal feature 😁

Tiggilyboo · 2022-06-05T18:37:26Z

@Tiggilyboo I liked the swipe to dismiss feature so much I opened a new PR #1173 with the code from this PR, rebased it and added the ring-buffer rework. Is it ok for you to move to the new PR, or would you like to update this PR instead and champion the changes yourself? Either one is fine by me, I just want the notification dismissal feature 😁

Yeah for sure get it merged, afk this weekend otherwise I would. Thanks for the ring buffer fixes!

NeroBurner · 2022-06-05T18:40:52Z

You're very welcome! :)

Then I'm closing this PR in favor of my new one.

Tiggilyboo commented May 19, 2022

View reviewed changes

src/displayapp/screens/Notifications.cpp Outdated Show resolved Hide resolved

Riksu9000 suggested changes May 19, 2022

View reviewed changes

Dismiss notifications by swiping right

1a3d340

Tiggilyboo force-pushed the notifications_dismiss branch from 7d32deb to 1a3d340 Compare May 19, 2022 18:02

Tiggilyboo changed the title ~~Dismiss Notifications (Swipe right) & Analogue face changes (Stepmeter using hour indicators)~~ Dismiss Notifications (Swipe right) May 19, 2022

Tiggilyboo requested a review from Riksu9000 May 19, 2022 18:17

Fix: Out of bounds array assignment

d88fc94

Fix: Notifications after dismissed have old index

3f0dc6d

- Cleaned up animation delay value (one place to configure in header)

NeroBurner requested changes May 19, 2022

View reviewed changes

src/components/ble/NotificationManager.cpp Show resolved Hide resolved

Tiggilyboo added 2 commits May 20, 2022 16:35

Cleanup, style changes

05f30d9

Tiggilyboo added 2 commits May 21, 2022 12:08

More cleanup, revert lv_page to lv_container for styling

169fb8a

- Try to use more LV_HOR_RES / LV_VER_RES where applicable in place of magic 240's - Revert page to lv_container as the styling parts were not working with LV_PAGE_PART_BG (background colour, padding, border etc.).

Fix for negative notification indexes (unsigned: 0, 255, ..)

9fedd9b

NeroBurner reviewed May 23, 2022

View reviewed changes

src/components/ble/NotificationManager.cpp Outdated Show resolved Hide resolved

Tiggilyboo marked this pull request as ready for review May 26, 2022 18:38

Tiggilyboo requested a review from NeroBurner May 27, 2022 16:00

NeroBurner added this to the 1.10.0 milestone Jun 4, 2022

NeroBurner mentioned this pull request Jun 5, 2022

Dismiss notifications by swiping right #1173

Merged

NeroBurner closed this Jun 5, 2022

NeroBurner removed this from the 1.10.0 milestone Jun 5, 2022

Uh oh!

Dismiss Notifications (Swipe right) #1150

Dismiss Notifications (Swipe right) #1150

Uh oh!

Conversation

Tiggilyboo commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Riksu9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Tiggilyboo commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Itai-Nelken commented May 19, 2022

Uh oh!

Tiggilyboo commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NeroBurner commented May 19, 2022

Uh oh!

NeroBurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Itai-Nelken commented May 19, 2022

Uh oh!

NeroBurner commented May 19, 2022

Uh oh!

NeroBurner commented May 19, 2022

Uh oh!

Uh oh!

Tiggilyboo commented May 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Itai-Nelken commented May 22, 2022

Uh oh!

Uh oh!

NeroBurner commented May 30, 2022

Uh oh!

Tiggilyboo commented May 31, 2022

Uh oh!

NeroBurner commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NeroBurner commented Jun 5, 2022

Uh oh!

Tiggilyboo commented Jun 5, 2022

Uh oh!

NeroBurner commented Jun 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Tiggilyboo commented May 19, 2022 •

edited

Loading

Tiggilyboo commented May 19, 2022 •

edited

Loading

Tiggilyboo commented May 19, 2022 •

edited

Loading

Tiggilyboo commented May 21, 2022 •

edited

Loading

NeroBurner commented Jun 1, 2022 •

edited

Loading