Skip to content

Notifications #197

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.

Sign up for GitHub

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

Jump to bottom
Merged
camilasan merged 22 commits into from
Mar 13, 2018
Merged

Notifications #197

camilasan merged 22 commits into from
Mar 13, 2018

Conversation

@camilasan
Copy link
Member

@camilasan camilasan commented Feb 25, 2018 •
edited
Loading

  • Display the actual response text from the notification api in the popups.
  • Do not display sync activities messages like how many files were downloaded, only errors.
  • Make the notifications section above the activity feed also clickable. e.g open the call, the file that was shared, open the calendar event etc
  • Split "Show desktop notifications" setting
  • Show notifications on startup and then show only NEW notifications: uses If-None-Match check to only retrieve notifications once there are new notifications.

Notification widget:
notificationwidget

One user notification:
one-user-notification

Multiple users notification:
notifications

Settings:
settings

Camila San added 15 commits February 12, 2018 22:16
Displays the actual notification subject in the system tray messages.
a7dd3a7 
Signed-off-by: Camila San <[email protected]>
Adds button to open notification in the browser - to update something…
b24751b 
…, join a call, etc.

Signed-off-by: Camila San <[email protected]>
Adds option to toggle display of Sync and Activity notifications.
bfc7aa1 
Signed-off-by: Camila San <[email protected]>
Changes notification text in the checkboxes and adds tooltips with ex…
ab7185a 
…planation about it.

- Changes the configuration name in ConfigFile and GeneralSettings
accordingly with the new text.
- Makes sure the user sees error and conflict messages even if the
setting is disabled.

Signed-off-by: Camila San <[email protected]>
Improves Notification Widget UI.
20cf4ed 
- Changes spacing.
- Removes QFrame.
- Changes icon size.

Signed-off-by: Camila San <[email protected]>
Doesn't open the file manager when there isn't a file associated with…
32e29fb 
… the Activity.

Signed-off-by: Camila San <[email protected]>
Sync activities notifications are never shown unless there is an erro…
1a0edc1 
…r. Server activities and notifications are off by default.

Signed-off-by: Camila San <[email protected]>
Changes notification icon bell size.
4c79059 
Signed-off-by: Camila San <[email protected]>
Removes (s) from time strings.
e33d2bf 
Signed-off-by: Camila San <[email protected]>
Removes _appName from Activity data.
4a6a8ca 
Signed-off-by: Camila San <[email protected]>
Do not display user account when there is only one user in popup mess…
71f4760 
…age.

Signed-off-by: Camila San <[email protected]>
Adds host and scheme to activity link so it can be opened in the
d71438f 
browser.

- Removes debug messages.
- Removes Activity data _appName - not used anymore.

Signed-off-by: Camila San <[email protected]>
Removes debug messages.
44f299e 
Signed-off-by: Camila San <[email protected]>
Only uses 'More information' text in button for notification link.
0f9d8e1 
Signed-off-by: Camila San <[email protected]>
Align text and changes spacing in notification widget.
09a92a5
@camilasan camilasan changed the title Changes Notifications Notifications Feb 25, 2018
@jancborchardt
Copy link
Member

jancborchardt commented Feb 26, 2018

Cool! 2 things:

  • In the Notifications in the settings, it says "created 1 hour ago". Instead should only say the time, like for the Activities: "1 hour ago"
  • There are 2 settings? "Show Server Notifications" and "Show Server Activities". What does the second one mean – does it influence the desktop notifications, or does disabling it hide the "Activity" view? Cause I thought we would stick to one setting for only enabling/disabling desktop notifications.

@camilasan
Copy link
Member Author

camilasan commented Feb 26, 2018

@jancborchardt

In the Notifications in the settings, it says "created 1 hour ago". Instead should only say the time, like for the Activities: "1 hour ago"

ok, will change it

There are 2 settings? "Show Server Notifications" and "Show Server Activities". What does the second one mean – does it influence the desktop notifications, or does disabling it hide the "Activity" view? Cause I thought we would stick to one setting for only enabling/disabling desktop notifications.

The second one means a mistake :) Removing it.

@juliusknorr
Copy link
Member

juliusknorr commented Mar 2, 2018

@camilasan Is this also ready for review? Feel free to update the label if so.

juliusknorr
juliusknorr reviewed Mar 2, 2018
View reviewed changes
src/gui/servernotificationhandler.cpp
a._id = json.value("notification_id").toInt();
a._subject = json.value("subject").toString();
a._message = json.value("message").toString();

Copy link
Member

@juliusknorr juliusknorr Mar 2, 2018

Choose a reason for hiding this comment

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

Ideally we could also use and cache the icon value and use that in the UI, but that is also something that could be done later.

juliusknorr
juliusknorr reviewed Mar 2, 2018
View reviewed changes
src/libsync/configfile.cpp Outdated
{
QSettings settings(configFile(), QSettings::IniFormat);
return settings.value(QLatin1String(optionalDesktopNoficationsC), true).toBool();
return settings.value(QLatin1String(optionalServerNotificationsC), false).toBool();
Copy link
Member

@juliusknorr juliusknorr Mar 2, 2018

Choose a reason for hiding this comment

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

I think notifications should be enabled by default. Any opinions @jancborchardt

Copy link
Member Author

@camilasan camilasan Mar 3, 2018

Choose a reason for hiding this comment

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

Done.

@camilasan
Copy link
Member Author

camilasan commented Mar 3, 2018 •
edited
Loading

@juliushaertl

Is this also ready for review? Feel free to update the label if so.

I think now you can review it. Could you try to run it too? Thanks.

@camilasan camilasan mentioned this pull request Mar 3, 2018
Merged
@juliusknorr
Copy link
Member

juliusknorr commented Mar 5, 2018

I think now you can review it. Could you try to run it too? Thanks.

I did. Works pretty nice 👍 I'll have another look for a final review.

@camilasan
Copy link
Member Author

camilasan commented Mar 7, 2018

And @juliushaertl ? :)

@camilasan
Copy link
Member Author

camilasan commented Mar 13, 2018

@rullzer could you review it too? thanks!

rullzer
rullzer approved these changes Mar 13, 2018
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Good stuff. Lets get this in!

@camilasan camilasan merged commit 08b33c6 into master Mar 13, 2018
@jancborchardt jancborchardt deleted the fix-notifications branch March 20, 2018 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliusknorr juliusknorr juliusknorr left review comments

@rullzer rullzer rullzer approved these changes

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@tcanabrava tcanabrava Awaiting requested review from tcanabrava

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@camilasan @jancborchardt @juliusknorr @rullzer