Implement notification layer backend and qml component for tray window #2220

DominiqueFuchs · 2020-07-20T22:36:19Z

Base for #2217

Implements the backend part (QStringList as current list of notifications + signals / helper functions) + new QML component and Connection in Window.qml to show and dismiss notifications.

@jancborchardt Design here?

Code-review: Please roast. Regarding the QStringList: Wasn't sure about this. I decided for that way for now b/c

Yeah I was glimpsing towards a more general container approach too, but then I thought KISS and not try to make this a working base for the future implementation of all notifications handled in the sync view (settings) right now. However, if we start to do that at some point, changing from the QStringList to a more sophisticated connection is still possible.
I didn't implement a model/delegate system for this b/c I don't think it is the right case for that. We might have multiple notifications in the queue. but we always will only show one per time (at least I don't see a reason for showing multiple with the limited space we have here). If the most recent is dismissed, the next one will show up.

Also UX regarding point 2: Right now only the last notification (i.e.: The one added most recently) is show, it's a LIFO. Could be reversed, though?

nicolasfella

Code-review: Please roast

Done :-) If any of my comments is unclear please ask.

src/gui/systray.cpp

nicolasfella · 2020-07-20T22:45:48Z

src/gui/tray/Notification.qml

@@ -0,0 +1,85 @@
+import QtQuick 2.9


Does this need a copyright header?

yes, please add a copyright notice with the matching license header

nicolasfella · 2020-07-20T22:52:43Z

src/gui/tray/Notification.qml

+    property alias text: notificationMessage.text
+    signal dismissNotification()
+
+    width: parent.width - Style.trayWindowBorderWidth*2


I usually consider it bad style when QML components make assumptions about their size/placement. It makes it harder to reuse them. Therefore width/height/anchors should only be set from the outside. Given that this one is only used in one place it's probably not a big deal though

Could you please clarify this? As I see it, I'm relying here on an external style property already (trayWindowBorderWidth) and only care about making it full-width of the tray window minus it's decoration border (whatever size it may be now or in the future). Guess I'm just don't getting it.

I am with @nicolasfella here.
This sizing property should be set in the parent but not in the child. That makes it much easier to reuse and to maintain in the future. So move it to places where you use the Notification component.

src/gui/tray/Notification.qml

src/gui/systray.h

src/gui/tray/Notification.qml

jancborchardt · 2020-07-21T04:04:17Z

Good stuff @DominiqueFuchs! Design-wise I'd say it's a bit better if the notification is part of the scrollable container of the activity list, but the first entry.

This is because all focus in that new tray is on the top – controls are there and the newest entries are there as well. So a notification on the bottom is likely to be missed.

er-vin · 2020-07-21T11:27:49Z

Looks too big and intrusive so late in the 2.7 cycle, we're more slowly freezing to reach rc1. I'll move the milestone for 2.8.

er-vin · 2020-09-16T16:08:03Z

/rebase

er-vin · 2020-09-16T16:08:28Z

@DominiqueFuchs are you still planning to work on this? Now would be the right time to be on the 3.1 train.

Signed-off-by: Dominique Fuchs <[email protected]>

DominiqueFuchs · 2020-09-16T18:40:43Z

@DominiqueFuchs are you still planning to work on this? Now would be the right time to be on the 3.1 train.

I'd say you choose 🙃 I'll still limited time for this project within the next few weeks - without any external guidance I would have worked on bugs in the existing timeframes, but if this is something you are prioritizing I'll put it to the top.

Signed-off-by: Dominique Fuchs <[email protected]>

DominiqueFuchs · 2020-09-17T07:13:34Z

I revisited and changed code according to comments so far (thanks @nicolasfella), aside from the one where I asked for clarification.

Another thing is: I just noticed how misleading the "notification" naming is regarding the code. Actually, "notifications" are a different thing (real notifications based on OS mechanisms, the one we push out for things like update, sync errors, etc.). Thus, we have a "NotificationHandler" that has nothing to do with this in-tray notification just showing special information in the bottom line until dismissed. I think we should rename this "Notification.qml" and corresponding properties to not lead new contributors to think this would be correlated and keep the overall syntax clear. Sugestions?

er-vin · 2020-09-17T07:58:35Z

@DominiqueFuchs are you still planning to work on this? Now would be the right time to be on the 3.1 train.

I'd say you choose 🙃 I'll still limited time for this project within the next few weeks - without any external guidance I would have worked on bugs in the existing timeframes, but if this is something you are prioritizing I'll put it to the top.

Don't want to put pressure on you, pushing it back by one release is fine. I'll do that.

er-vin · 2020-09-17T08:00:30Z

Another thing is: I just noticed how misleading the "notification" naming is regarding the code. Actually, "notifications" are a different thing (real notifications based on OS mechanisms, the one we push out for things like update, sync errors, etc.). Thus, we have a "NotificationHandler" that has nothing to do with this in-tray notification just showing special information in the bottom line until dismissed. I think we should rename this "Notification.qml" and corresponding properties to not lead new contributors to think this would be correlated and keep the overall syntax clear. Sugestions?

This is a very good point... Random ideas on top of my head: ServerNotification? ServerMessage? Announcement? (that one could be confusing as well...) StickyActivity?

nextcloud-desktop-bot · 2020-09-17T08:05:17Z

AppImage file: Nextcloud-PR-2220-3a9a9db259bbd85c038360dedaae8f0f7e6ae131-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

DominiqueFuchs · 2020-09-17T12:22:07Z

This is a very good point... Random ideas on top of my head: ServerNotification? ServerMessage? Announcement? (that one could be confusing as well...) StickyActivity?

They all could be misleading somehow (isn't a activity, but also hasn't necessarily to be coming from the server, too), even though I thought about similar ones initially..

Maybe we simply rename it to "Notice.qml" or "ClientNotice.qml" (and corresponding properties getLastNotice etc.). Depending on context the meaning of both words is mostly the same, but this is more or less the point here, they could at least clearly be distinguished and don't cross in search results?

er-vin · 2020-09-17T13:32:21Z

I like the Notice naming. Good idea.

mgallien

@DominiqueFuchs thanks and sorry for the delay. Do you still plan to work on that ?
there is ongoing work from @camilasan on this and you will need to rebase your on top of her work.
Please let us know if you need advice there

mgallien · 2021-05-14T10:02:45Z

src/gui/tray/Notification.qml

+    property alias text: notificationMessage.text
+    signal dismissNotification()
+
+    width: parent.width - Style.trayWindowBorderWidth*2


I am with @nicolasfella here.
This sizing property should be set in the parent but not in the child. That makes it much easier to reuse and to maintain in the future. So move it to places where you use the Notification component.

mgallien · 2021-05-14T10:03:11Z

src/gui/tray/Notification.qml

@@ -0,0 +1,85 @@
+import QtQuick 2.9


yes, please add a copyright notice with the matching license header

DominiqueFuchs requested review from er-vin, sbeyer, misch7 and camilasan July 20, 2020 22:36

DominiqueFuchs added 2. to review beta design feature: ☁️ system tray System tray icon and menu. feature: ⚡ activity and 🔔 notification labels Jul 20, 2020

DominiqueFuchs added this to the 2.7 🌟 UI improvements milestone Jul 20, 2020

nicolasfella requested changes Jul 20, 2020

View reviewed changes

er-vin modified the milestones: 2.7 🌟 UI improvements, 2.8 👥 Sharing features Jul 21, 2020

nicolasfella mentioned this pull request Jul 22, 2020

Sync status icons not shown in Linux distributions #2217

Open

Implement notification layer backend and qml component for tray window

c2bec03

Signed-off-by: Dominique Fuchs <[email protected]>

github-actions bot force-pushed the tray-window-overlay-msg branch from 987dc6b to c2bec03 Compare September 16, 2020 16:09

DominiqueFuchs added 2 commits September 17, 2020 08:47

Comments, minor code maintenance

2fec8c7

Signed-off-by: Dominique Fuchs <[email protected]>

Make notification a Q_PROPERTY

3a9a9db

Signed-off-by: Dominique Fuchs <[email protected]>

er-vin modified the milestones: Desktop 3.1, 4.0 🥚 Sep 17, 2020

mgallien requested changes May 14, 2021

View reviewed changes

mgallien added 1. to develop and removed 2. to review labels May 14, 2021

mgallien removed this from the 4.0 🥚 milestone Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement notification layer backend and qml component for tray window #2220

Implement notification layer backend and qml component for tray window #2220

DominiqueFuchs commented Jul 20, 2020 •

edited

Loading

nicolasfella left a comment •

edited

Loading

nicolasfella Jul 20, 2020

DominiqueFuchs Sep 17, 2020

mgallien May 14, 2021

nicolasfella Jul 20, 2020

DominiqueFuchs Sep 17, 2020

mgallien May 14, 2021

jancborchardt commented Jul 21, 2020

er-vin commented Jul 21, 2020

er-vin commented Sep 16, 2020

er-vin commented Sep 16, 2020

DominiqueFuchs commented Sep 16, 2020

DominiqueFuchs commented Sep 17, 2020

er-vin commented Sep 17, 2020

er-vin commented Sep 17, 2020

nextcloud-desktop-bot commented Sep 17, 2020

DominiqueFuchs commented Sep 17, 2020

er-vin commented Sep 17, 2020

mgallien left a comment

mgallien May 14, 2021

mgallien May 14, 2021

Implement notification layer backend and qml component for tray window #2220

Are you sure you want to change the base?

Implement notification layer backend and qml component for tray window #2220

Conversation

DominiqueFuchs commented Jul 20, 2020 • edited Loading

nicolasfella left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jancborchardt commented Jul 21, 2020

er-vin commented Jul 21, 2020

er-vin commented Sep 16, 2020

er-vin commented Sep 16, 2020

DominiqueFuchs commented Sep 16, 2020

DominiqueFuchs commented Sep 17, 2020

er-vin commented Sep 17, 2020

er-vin commented Sep 17, 2020

nextcloud-desktop-bot commented Sep 17, 2020

DominiqueFuchs commented Sep 17, 2020

er-vin commented Sep 17, 2020

mgallien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominiqueFuchs commented Jul 20, 2020 •

edited

Loading

nicolasfella left a comment •

edited

Loading