GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — notifications: Dismiss to watchface when empty by eliedrian · Pull Request #1716 · InfiniTimeOrg/InfiniTime

Conversation

@eliedrian

@eliedrian eliedrian commented Mar 30, 2023

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Tiggilyboo, findirfin, mathmaniac43, devnoname120, mark9064, vkareh, spaetz, Commenter25, rnwgnr, and marigoldfish reacted with thumbs up emoji LinuxinaBit, spaetz, NitroNils, Commenter25, marigoldfish, and marcsello reacted with heart emoji
All reactions
  • 👍 10 reactions
  • ❤️ 6 reactions
@github-actions

github-actions Bot commented Mar 30, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Build checks have not completed. Possible reasons for this are:

  1. The checks need to be approved by a maintainer
  2. The branch has conflicts
  3. The firmware build has failed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Riksu9000
Riksu9000 reviewed Mar 30, 2023
View reviewed changes

@Riksu9000 Riksu9000 left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This seems like an interesting idea. The animation isn't working as I'd expect though. I expect the sideways dismiss animation to go to a black screen and then slide up to the watch face.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/displayapp/screens/Notifications.cpp

} else if (mode == Modes::Preview && dismissingNotification) {
running = false;
currentItem = std::make_unique<NotificationItem>(alertNotificationService, motorController);

@Riksu9000 Riksu9000 Mar 30, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I guess this line has been deleted because it isn't necessary. Can you confirm the intent?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@eliedrian eliedrian Mar 30, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

That's right. No longer necessary. running is set to false. Previously, currentItem would be populated with the "No notifications to display" notification item.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Riksu9000 Riksu9000 Mar 30, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/displayapp/screens/Notifications.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/displayapp/screens/Notifications.cpp
Comment on lines -120 to +119
running = currentItem->IsRunning() && running;
running = running && currentItem->IsRunning();

@Riksu9000 Riksu9000 Mar 30, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This change isn't necessary.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@eliedrian eliedrian Mar 30, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Riksu9000 Riksu9000 Mar 30, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/displayapp/screens/Notifications.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.
@eliedrian

eliedrian commented Mar 30, 2023

Copy link
Copy Markdown
Contributor Author

Also addressed the strange animation.

Earlier, screen was flagged to be killed without the animation occurring.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@eliedrian
eliedrian force-pushed the main branch 4 times, most recently from 3c734a0 to b49a84d Compare March 31, 2023 03:52
@eliedrian

eliedrian commented Mar 31, 2023

Copy link
Copy Markdown
Contributor Author

Squashed.

Also updated the comment instead of fully removing it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@eliedrian
eliedrian force-pushed the main branch from b49a84d to 0bed5f7 Compare March 31, 2023 08:15
@eliedrian
eliedrian force-pushed the main branch from 0bed5f7 to 5a07821 Compare April 11, 2023 08:39
@LinuxinaBit

LinuxinaBit commented Apr 15, 2023

Copy link
Copy Markdown

Oh, thank you so much, I've been wanting a feature like this for so long!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Riksu9000
Riksu9000 self-requested a review April 16, 2023 11:32
mark9064 added a commit to mark9064/InfiniTime that referenced this pull request May 8, 2023
@mark9064
notifications: Dismiss to watchface when empty InfiniTimeOrg#1716
5c3f255 
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
mark9064 added a commit to mark9064/InfiniTime that referenced this pull request May 27, 2023
@mark9064
notifications: Dismiss to watchface when empty InfiniTimeOrg#1716
b63df84 
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
@eliedrian
eliedrian force-pushed the main branch from 5a07821 to 3cf2432 Compare September 4, 2023 08:06
@mark9064

mark9064 commented Nov 6, 2023

Copy link
Copy Markdown
Member

Have been daily driving this for months, zero issues
Commenter25, rnwgnr, and tituscmd reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vkareh vkareh mentioned this pull request Jan 23, 2024
DisplayApp: Go to clock on wakeup if no app loaded #1980
Merged
@eliedrian
eliedrian force-pushed the main branch from 3cf2432 to 9a9165d Compare March 21, 2024 06:41
@mark9064

mark9064 commented Apr 11, 2024

Copy link
Copy Markdown
Member

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
Commenter25, LinuxinaBit, rnwgnr, marigoldfish, and tituscmd reacted with thumbs up emoji
All reactions
  • 👍 5 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@eliedrian
eliedrian force-pushed the main branch from 9a9165d to ffdafa7 Compare June 3, 2024 06:24
@eliedrian
notifications: Dismiss to watchface when empty
ffdafa7 
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
@eliedrian

eliedrian commented Jun 3, 2024

Copy link
Copy Markdown
Contributor Author

Yes, anything lacking from this?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@eliedrian
Merge branch 'InfiniTimeOrg:main' into main
4dbec96
@marigoldfish

marigoldfish commented Aug 12, 2024

Copy link
Copy Markdown

One more voice in support of this PR being merged :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
JF002
JF002 approved these changes Aug 19, 2024
View reviewed changes
@JF002 JF002 added this to the 1.15.0 milestone Aug 19, 2024
NeroBurner
NeroBurner approved these changes Sep 18, 2024
View reviewed changes

@NeroBurner NeroBurner left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

looking good. I like the feature

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Commenter25 and tituscmd reacted with heart emoji
All reactions
  • ❤️ 2 reactions
@NeroBurner
NeroBurner merged commit a266202 into InfiniTimeOrg:main Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JF002 JF002 JF002 approved these changes
@NeroBurner NeroBurner NeroBurner approved these changes
@Riksu9000 Riksu9000 Awaiting requested review from Riksu9000

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.15.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@eliedrian @LinuxinaBit @mark9064 @marigoldfish @JF002 @NeroBurner @Riksu9000
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.