-
Notifications
You must be signed in to change notification settings - Fork 26
Hash server url #564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hash server url #564
Conversation
| const QString buttonText = tr("Open Download Page"); | ||
| notification.setButtons({buttonText}); | ||
| auto *sysNotification = ocApp()->systemNotificationManager()->notify(std::move(notification)); | ||
| connect(sysNotification, &SystemNotification::buttonClicked, this, [buttonText, url](const QString &button) { |
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.
how often do I get the notification? 🤔 once per software restart?
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.
yes
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.
Is the effort reasonably to show it only once per new available version? There might be users who can't install an update without an admin, so they can't do anything about the notification.
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.
The system notification is usually only displayed for a couple of seconds, so it's not too intrusive.
If we wanted to implement a "skip this update" functionality, we would need to persist the decision. Also, we would need to add a second action to the notification, which is currently not implemented on Mac.
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.
sounds all reasonable :-) thank you!
| updateJob->start(); | ||
| } else { | ||
| qCWarning(lcUpdateNotifier) << u"Update check failed with HTTP code" << job->httpStatusCode(); | ||
| qCWarning(lcUpdateNotifier) << u"Update check config.json with HTTP code" << job->httpStatusCode(); |
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.
are the warnings user facing? because then they'd be very technical 🙈
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.
only logs
kulmann
left a comment
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.
LGTM 👍
No description provided.