notifications: Dismiss to watchface when empty #1716
Conversation
|
Build checks have not completed. Possible reasons for this are:
|
| @@ -82,7 +82,6 @@ void Notifications::Refresh() { | |||
|
|
|||
| } else if (mode == Modes::Preview && dismissingNotification) { | |||
| running = false; | |||
| currentItem = std::make_unique<NotificationItem>(alertNotificationService, motorController); | |||
There was a problem hiding this comment.
I guess this line has been deleted because it isn't necessary. Can you confirm the intent?
There was a problem hiding this comment.
That's right. No longer necessary. running is set to false. Previously, currentItem would be populated with the "No notifications to display" notification item.
There was a problem hiding this comment.
Well this is for the preview mode, so I believe it's not exactly necessary for this change. It should be explained in the commit description.
| running = currentItem->IsRunning() && running; | ||
| running = running && currentItem->IsRunning(); |
There was a problem hiding this comment.
This change isn't necessary.
There was a problem hiding this comment.
I think this is safer. Setting running to false implies we're ending the screen. It'd be a waste to create an item to assign to currentItem when we're ending anyway.
There was a problem hiding this comment.
Oh I see. I didn't realize that currentItem is left a nullptr in another function, and removing line 116 means it stays nullptr. Looks like this is more of an issue with code clarity. You should squash the fixup commits and explain these things in the commit message.
|
Also addressed the strange animation. Earlier, screen was flagged to be killed without the animation occurring. |
eliedrian
force-pushed
the
main
branch
4 times, most recently
from
3c734a0 to
b49a84d
Compare
|
Squashed. Also updated the comment instead of fully removing it. |
|
Oh, thank you so much, I've been wanting a feature like this for so long! |
commit 5a07821 Author: Eli Tan <[email protected]> Date: Thu Mar 30 15:43:05 2023 +0800 notifications: Dismiss to watchface when empty Set `running` to false to flag end of watchface when there are no more notifications left to display. notifications: Update now inaccurate comment notifications: Fix `currentItem` possibly being null Consequentially, `currentItem` can be left null when `running` is set to false. This is fine. The notifications screen is ending anyway. notifications: Delay ending screen for dismiss animation notifications: End screen when item is not valid
commit 5a07821 Author: Eli Tan <[email protected]> Date: Thu Mar 30 15:43:05 2023 +0800 notifications: Dismiss to watchface when empty Set `running` to false to flag end of watchface when there are no more notifications left to display. notifications: Update now inaccurate comment notifications: Fix `currentItem` possibly being null Consequentially, `currentItem` can be left null when `running` is set to false. This is fine. The notifications screen is ending anyway. notifications: Delay ending screen for dismiss animation notifications: End screen when item is not valid
|
Have been daily driving this for months, zero issues |
|
Are there any outstanding issues preventing this from being merged? To me this is an easy usability improvement and as far as I can see all review comments have been addressed |
Set `running` to false to flag end of watchface when there are no more notifications left to display. notifications: Update now inaccurate comment notifications: Fix `currentItem` possibly being null Consequentially, `currentItem` can be left null when `running` is set to false. This is fine. The notifications screen is ending anyway. notifications: Delay ending screen for dismiss animation notifications: End screen when item is not valid
|
Yes, anything lacking from this? |
|
One more voice in support of this PR being merged :) |
I found it slightly annoying that dismissing all notifications leaves me with a "No notification to display" message. Instead of dismissing to a relatively useless message, dismiss to watchface.