Show Server Notifications on the Client #4567
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It is needed to easily send delete requests which happen through the notify API.
It displays a server notification that can come with a dynamic set of buttons next to a message and a subject (=header)
As interaction is required, the notifications are displayed in a separate widget above the server activity list. Note that design and also where we display the notifications can still be discussed and changed.
Parse the replyCode from the button action calls and disable buttons accordingly.
The isValid check should be used everywhere the capabilities are used as the loading of the capabilities is happening in parallel of the startup, so it is not guaranteed to be available always.
If not, do not query for them.
Show a GUI notification once an hour if no new notifications arrive to not annoy users.
That cleans the ActivityWidget class
Created a activitydata.h header (only) for the basic data, plus a separate file for the model. Cleans up the widget source.
Add the hostname from where the notification comes, as well as the name of the application to the header.
| // restart the gui log timer now that we show a notification | ||
| _guiLogTimer.restart(); | ||
|
|
||
| // Assemble a tray notification |
There was a problem hiding this comment.
I don't think you can build strings this way. Even with translator comments This will break in all kind of funny ways e.g. with RTL languages. Also, with too many notifiers, the message cannot be parsed fast enough, so we should probably limit the amount of displayed notification sources (?) as well. You at have to account for three cases, with the n=2 one being optional:
n =1:tr("You received %n notifications from %1").arg(notifier)n = 2:tr("You received %n notifications from %1 and %2").arg(notifiers.at(0), notifiers.at(1))n > 2:tr("You received %n notifications from %1, %2 and others").arg(notifiers.at(0), notifiers.at(1)(ideally, we would specify a quantity for "others", but we cannot have more than one plural form in one tr()...
| { | ||
| QList<AccountStatePtr> accounts = AccountManager::instance()->accounts(); | ||
|
|
||
| foreach (AccountStatePtr asp, accounts) { |
Even for the case where the number is fixed. Also fix the translators comments.
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This patch grew big already, before it grows even bigger, here is the pull request.
It implements the notifications from the server as requested in #3733
Still missing:
@danimo @ogoffart @ckamm @guruz