Add call notification dialog. #4426

camilasan · 2022-04-10T18:00:13Z

Implements #4159

TO DO:

I think the UI is essentially done, the buttons close the notification (dismiss) and open the browser (answer call). It does seem we do too many request for activities which atm triggers way too many call notification popups when you open the client and when you open the systray window. On Android and server they only show the notification on a second request to the server - in our case not show it when we open the client - or when it is a push notification.
Show the avatar of the caller. There is a talk icon as a placeholder atm. (see user in subjectRichParameters - https://github.com/nextcloud/notifications/blob/master/docs/ocs-endpoint-v2.md#getting-the-notifications-of-a-user)

Current status:

call-.5.mp4

jancborchardt · 2022-04-11T14:17:51Z

Mockup also posted in the issue at #4159 (comment)

claucambra · 2022-04-21T00:16:09Z

A couple of updates about my changes:

Calls from users now show the avatars for the user, as well as a blurred background per the design team's directions
Calls now appear regardless of whether the client is open or not -- they will be immediate if push notifications are enabled, otherwise they take give or take 60 seconds
Calls are notified now only once per call during the lifetime of the application

Now matches designs in #4159

claucambra · 2022-04-21T16:22:28Z

Remaining tasks:

Fix sizing and padding of the call notification window
Fix ringtone (and timeout) on macOS/Windows
Place call notification on the right corner of the screen rather than below the tray icon
Ensure there is enough spacing between screen edges and the notification window
Reduce outline on call window on macOS

claucambra · 2022-04-21T16:23:26Z

Window padding and sizing now fixed -- the call notification window width is constant but adapts to the vertical space taken up by its contents, ensuring equal padding all around:

jancborchardt · 2022-04-21T17:55:28Z

Looking good @claucambra! :) Btw quick question about group call notifications:

In light desktop theme, the icon is blue / themed and the text is black / default text color (that looks good)
In dark desktop theme, the text is white, but what is the icon color? I'd expect it to be white too / same as text color. Just to make sure it's not blue/themed in dark mode. :)

claucambra · 2022-04-21T22:06:37Z

Call notification dialog now shows up in the corners, rather than in the same position as the tray.

The exact corner depends on the position of the taskbar and the position of the system tray icon. The notification will stick close to the taskbar and then decide on the exact position depending on which extreme of the task bar the system tray icon is in.

I.e.: the task bar is at the top so the notification window will show at the top. But if the system tray icon is left of centre in the taskbar it will show up on the top-left corner, while if the system tray icon is right of center it will show up on the top-right corner.

Looking good @claucambra! :) Btw quick question about group call notifications:

In light desktop theme, the icon is blue / themed and the text is black / default text color (that looks good)

In dark desktop theme, the text is white, but what is the icon color? I'd expect it to be white too / same as text color. Just to make sure it's not blue/themed in dark mode. :)

The talk icon is now white in dark mode.

The width of the tray window borders has also been fixed on macOS.

Latest screenshot:

jospoortvliet · 2022-04-22T09:59:18Z

Hi @claucambra to make screenshots I was testing a bit. It is a bit unreliable - first, I see calls twice in the menu:

And the call screen popup is very unreliable - sometimes it shows, but now I couldn't get it to show up anymore...

I use tech-preview.nextcloud.com with user leon green calling user christine and using the desktop client from the build by the bot, 11 hours ago - Nextcloud-PR-4426-833a4ffd774269893a342ac7d765777ff1e8e0a8-x86_64.AppImage

You can reproduce that setup with the tech preview login details in our cloud, if you don't have them ping me.

Edit: let me add that WHEN it shows up, the call screen looks very nice. I still had to resize it a little on my desktop, but that might be due to the font dpi settings - it seems Qt gets confused by anything not 96 DPI.

edit2: forgot to add debug archive: https://cloud.nextcloud.com/s/i5QYwXfKSJBCxRc

claucambra · 2022-04-22T10:12:15Z

Hi @claucambra to make screenshots I was testing a bit. It is a bit unreliable - first, I see calls twice in the menu:

And the call screen popup is very unreliable - sometimes it shows, but now I couldn't get it to show up anymore...

I use tech-preview.nextcloud.com with user leon green calling user christine and using the desktop client from the build by the bot, 11 hours ago - Nextcloud-PR-4426-833a4ffd774269893a342ac7d765777ff1e8e0a8-x86_64.AppImage

You can reproduce that setup with the tech preview login details in our cloud, if you don't have them ping me.

Hi @jospoortvliet,

I would say the duplicated entries in the menu are out of scope of this PR, though obviously they are a problem. It is unclear where the issue is stemming from, we will need to investigate the code for fetching/parsing notification data and/or see if this is also an issue on the server side.

The client will also only provide the popup once per call, so if this is the same call then there will be no popup (i.e. the same ongoing call, if everyone leaves the call and a new group call starts then the notification will show). Additionally, the notification window will also only show if the notification received from the server is less that 2 minutes old. This is to prevent users from being over-notified by calls.

Lastly, depending on if push notifications are configured the call notifications should be near-immediate, but if push notifications are not configured on the server it may take around 60 seconds for the call notification to show as this is how often we poll the server for new notifications. Any more than that and we risk dynamiting the server.

I don't have the details for the tech preview server, would be useful to have them :)

allexzander · 2022-04-22T10:50:31Z

src/libsync/configfile.cpp

@@ -557,7 +557,7 @@ chrono::milliseconds ConfigFile::notificationRefreshInterval(const QString &conn
    QSettings settings(configFile(), QSettings::IniFormat);
    settings.beginGroup(con);

-    auto defaultInterval = chrono::minutes(5);
+    auto defaultInterval = chrono::minutes(1);


@claucambra @mgallien I am not sure we need to change this for every user. Won't we end up creating a high load on the server in case of thousands of clients refresh notifications every minute? AFAIK, push notifications will almost always be available, so, we can assume the interval of 5 minutes is fine for the rest of the cases, but, if not, a user can change it directly in the custom config.

Yes, this is true... on the flip side, though, the only connected server I have on my client that supports push notifications is our work instance, and in those conditions this means if we leave it at 5 minutes the likelihood of the call notifications being received in a timely way by the client get really close to 0 :/

I am no expert here, but did we not set the default to 5 min only for cases there is a push server? Did we change it for all desktop clients? Can we not set it to 30 seconds without the HPB and 5 minutes with the HPB?

If you then have load issues from the clients, use the HPB.

I agree with @jospoortvliet

@jospoortvliet @mgallien At least in the user model, which handles notifications, this is not the case -- when push notifications are connected to the server, the polling is stopped altogether and notifications are fetched solely through the push notification method. If the connection is lost or the push notifications are no longer available, the polling is re-initiated

Relevant code:

desktop/src/gui/tray/usermodel.cpp

Line 152 in 14220e4

// as we are now able to use push notifications - let's stop the polling timer

allexzander · 2022-04-22T11:09:30Z

@claucambra Approved. But, please check my comment.

allexzander · 2022-04-22T11:14:44Z

I would say the duplicated entries in the menu are out of scope of this PR, though obviously they are a problem. It is unclear where the issue is stemming from, we will need to investigate the code for fetching/parsing notification data and/or see if this is also an issue on the server side.

@jospoortvliet, @claucambra
Duplicate entries will be fixed by me in another PR (they actually already are, but, the PR will need review)

jospoortvliet · 2022-04-22T19:40:41Z

@jospoortvliet, @claucambra Duplicate entries will be fixed by me in another PR (they actually already are, but, the PR will need review)

Thanks!!!

CarlSchwan · 2022-04-22T22:13:10Z

I wonder if this could be possible to disable (even if this would be a secret config flag for now) as it would otherwise collide at some point with my small personal project https://github.com/CarlSchwan/talk-desktop and getting two notifications would a bit annoying

CarlSchwan · 2022-04-23T07:19:49Z

src/gui/systray.cpp

+    if (!useNormalWindow()) {
+        window->setScreen(currentScreen());
+        const auto position = computeNotificationPosition(window->width(), window->height());
+        window->setPosition(position);


this doesn't work with wayland :( and on x doesn't follow the kde settings

(X11)

Ok, so digging into the code, the problem is this: on Linux DEs Qt's QSystemTrayIcon::geometry() is completely broken, which means we have no way of getting the position of the tray icon or, by extension, the task bar.

For the main tray window this is ok with a workaround which checks the position of the cursor and uses this as the position for the tray to open -- the idea being you'll click on the icon, thus giving the position of the icon. This breaks when trying to use it for the call notification dialog as your cursor could be anywhere on the screen at the time of the call, and the call notification position is unpredictable.

Since we have no good way of finding the taskbar position (or size) on Linux, we have to either hardcode a position for the dialog or let the window manager decide where to place it (which will usually be in the center of the screen). Hardcoding it to a corner will risk breaking the convention of the desktop in question (KDE Plasma notifies wherever the taskbar is by default, usually at the bottom-right, whereas GNOME is top-right). @jancborchardt

For Wayland I will do some more digging, though my hunch is that the positioning will just be handled by the window manager

For now I have disabled the positioning code for the notification of the system tray icon geometry is invalid

For now I have disabled the positioning code for the notification of the system tray icon geometry is invalid

Image for reference:

we have to either hardcode a position for the dialog or let the window manager decide where to place it (which will usually be in the center of the screen)

@claucambra If we have to decide between the 2, then I would say definitely hardcoding makes for a way better UX. If it’s KDE show on bottom right, and if it’s GNOME or anything else show on top right. Just use the default task bar height + some padding for vertical position, and some padding to the right side. Sure that can still lead to overlaps in super super edge cases, but we can fix those as they are reported. Way less bad than having a notification smack dab in the middle of the screen. :)

This is a bad idea in practice -- we do not know the size of the panels, the orientation of the panels, how DPI will affect positioning and padding of the notification, etc. A center position on the screen may not be ideal but, IMO, it is preferable to risking positioning that will be eating into people's panels or showing off-screen for users with a variety of display densities and DEs

I think the only viable solution here for notifications in corners on the Linux DEs is to default to a safe position like the center and then let the user customise the positioning through the application settings. Happy to hear if I have missed something here @mgallien @camilasan @CarlSchwan

Will this issue come up in a default installation of Ubuntu, or where would it happen?

I know hardcoding is not elegant, but putting this notification in the center is quite bad design. I'd say even if the notification is all the way in the top right / bottom right, eating into a panel, that's a better experience than in the very center.

Wayland is now the default on GNOME (Ubuntu and Fedora) and KDE (Fedora). So in these cases the system tray and custom notification will be displayed on the middle of the screen.

On X11 GNOME environments, the notification will now be displayed on the top-right.

For Wayland environments, as well as other DEs, we still display in center

Yeah, so as said it sounds bad that centering this would be the default experience, even if it's only Linux. Top right and bottom right would be much better, even if it overlaps something. Cause center overlaps everything always.

But cc @tobiasKaminsky on weighing the workload.

claucambra · 2022-04-23T13:35:01Z

I wonder if this could be possible to disable (even if this would be a secret config flag for now) as it would otherwise collide at some point with my small personal project https://github.com/CarlSchwan/talk-desktop and getting two notifications would a bit annoying

It should be relatively easy to implement a setting that disables this, I think some users would like this too

claucambra · 2022-04-25T15:22:50Z

I wonder if this could be possible to disable (even if this would be a secret config flag for now) as it would otherwise collide at some point with my small personal project https://github.com/CarlSchwan/talk-desktop and getting two notifications would a bit annoying

I added a checkbox in the client's settings to disable the call notifications:

src/gui/systray.cpp

src/gui/systray.h

src/gui/tray/notificationhandler.cpp

src/gui/tray/usermodel.cpp

mgallien · 2022-04-26T12:46:10Z

src/libsync/configfile.cpp

@@ -557,7 +557,7 @@ chrono::milliseconds ConfigFile::notificationRefreshInterval(const QString &conn
    QSettings settings(configFile(), QSettings::IniFormat);
    settings.beginGroup(con);

-    auto defaultInterval = chrono::minutes(5);
+    auto defaultInterval = chrono::minutes(1);


I agree with @jospoortvliet

mgallien · 2022-04-26T12:48:12Z

@claucambra please also have a look at the so called Major issues reported by SonarCloud except for the id length they would be important to fix (and one is duplicating one of my comments)

src/gui/systray.h

Signed-off-by: Claudio Cambra <claudio.cambra@gmail.com> Co-authored-by: Camila <hello@camila.codes>

nextcloud-desktop-bot · 2022-04-28T07:34:50Z

AppImage file: Nextcloud-PR-4426-3f5243aaee9994e7f310cf2689b6fd05ef89503f-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.

sonarcloud · 2022-04-28T07:40:51Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
20 Code Smells

0.6% Coverage
0.0% Duplication

camilasan force-pushed the feature/call-notification branch 4 times, most recently from 1329abd to b77d7af Compare April 12, 2022 19:09

claucambra force-pushed the feature/call-notification branch from 3576e51 to 5e99f74 Compare April 20, 2022 21:59

claucambra requested review from allexzander and mgallien April 21, 2022 00:09

claucambra self-assigned this Apr 21, 2022

claucambra marked this pull request as ready for review April 21, 2022 00:10

claucambra changed the title ~~Draft: Add call notification dialog.~~ Add call notification dialog. Apr 21, 2022

claucambra requested a review from jancborchardt April 21, 2022 00:16

claucambra force-pushed the feature/call-notification branch from 214688a to 7da8baf Compare April 21, 2022 00:35

claucambra mentioned this pull request Apr 21, 2022

Enhance call notification #4159

Closed

7 tasks

jancborchardt linked an issue Apr 21, 2022 that may be closed by this pull request

Enhance call notification #4159

Closed

7 tasks

claucambra force-pushed the feature/call-notification branch from 062a65a to 45da741 Compare April 21, 2022 12:49

claucambra force-pushed the feature/call-notification branch 4 times, most recently from f6c9d6b to 833a4ff Compare April 21, 2022 22:36

allexzander reviewed Apr 22, 2022

View reviewed changes

allexzander approved these changes Apr 22, 2022

View reviewed changes

CarlSchwan reviewed Apr 23, 2022

View reviewed changes

claucambra force-pushed the feature/call-notification branch from 833a4ff to 907c832 Compare April 23, 2022 13:31

claucambra force-pushed the feature/call-notification branch from 907c832 to 056e379 Compare April 25, 2022 15:21

mgallien reviewed Apr 26, 2022

View reviewed changes

claucambra force-pushed the feature/call-notification branch 3 times, most recently from 14220e4 to 3f8bc03 Compare April 26, 2022 23:04

mgallien reviewed Apr 27, 2022

View reviewed changes

src/gui/systray.h Outdated Show resolved Hide resolved

claucambra force-pushed the feature/call-notification branch from 3f8bc03 to e4aac83 Compare April 27, 2022 18:31

mgallien approved these changes Apr 27, 2022

View reviewed changes

mgallien force-pushed the feature/call-notification branch from e4aac83 to 666bc8d Compare April 27, 2022 21:56

Add an incoming talk call notification to the desktop client

3f5243a

Signed-off-by: Claudio Cambra <claudio.cambra@gmail.com> Co-authored-by: Camila <hello@camila.codes>

mgallien force-pushed the feature/call-notification branch from 666bc8d to 3f5243a Compare April 28, 2022 07:13

mgallien merged commit d470705 into master Apr 28, 2022

mgallien deleted the feature/call-notification branch April 28, 2022 08:18

mgallien added this to the 3.5.0 milestone Apr 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add call notification dialog. #4426

Add call notification dialog. #4426

camilasan commented Apr 10, 2022 •

edited by claucambra

Loading

jancborchardt commented Apr 11, 2022 •

edited

Loading

claucambra commented Apr 21, 2022 •

edited

Loading

claucambra commented Apr 21, 2022 •

edited

Loading

claucambra commented Apr 21, 2022

jancborchardt commented Apr 21, 2022 •

edited

Loading

claucambra commented Apr 21, 2022 •

edited

Loading

jospoortvliet commented Apr 22, 2022 •

edited

Loading

claucambra commented Apr 22, 2022 •

edited

Loading

allexzander Apr 22, 2022

claucambra Apr 22, 2022

jospoortvliet Apr 22, 2022

mgallien Apr 26, 2022

claucambra Apr 26, 2022

claucambra Apr 26, 2022

allexzander commented Apr 22, 2022

allexzander commented Apr 22, 2022

jospoortvliet commented Apr 22, 2022

CarlSchwan commented Apr 22, 2022

CarlSchwan Apr 23, 2022

claucambra Apr 23, 2022 •

edited

Loading

claucambra Apr 23, 2022 •

edited

Loading

claucambra Apr 23, 2022

claucambra Apr 23, 2022

claucambra Apr 25, 2022 •

edited

Loading

jancborchardt Apr 25, 2022

CarlSchwan Apr 25, 2022

claucambra Apr 26, 2022

jancborchardt Apr 27, 2022

claucambra commented Apr 23, 2022

claucambra commented Apr 25, 2022

mgallien Apr 26, 2022

mgallien commented Apr 26, 2022

nextcloud-desktop-bot commented Apr 28, 2022

sonarcloud bot commented Apr 28, 2022

Add call notification dialog. #4426

Add call notification dialog. #4426

Conversation

camilasan commented Apr 10, 2022 • edited by claucambra Loading

Implements #4159

TO DO:

Current status:

jancborchardt commented Apr 11, 2022 • edited Loading

claucambra commented Apr 21, 2022 • edited Loading

claucambra commented Apr 21, 2022 • edited Loading

claucambra commented Apr 21, 2022

jancborchardt commented Apr 21, 2022 • edited Loading

claucambra commented Apr 21, 2022 • edited Loading

jospoortvliet commented Apr 22, 2022 • edited Loading

claucambra commented Apr 22, 2022 • 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

allexzander commented Apr 22, 2022

allexzander commented Apr 22, 2022

jospoortvliet commented Apr 22, 2022

CarlSchwan commented Apr 22, 2022

Choose a reason for hiding this comment

claucambra Apr 23, 2022 • edited Loading

Choose a reason for hiding this comment

claucambra Apr 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claucambra Apr 25, 2022 • 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

claucambra commented Apr 23, 2022

claucambra commented Apr 25, 2022

Choose a reason for hiding this comment

mgallien commented Apr 26, 2022

nextcloud-desktop-bot commented Apr 28, 2022

sonarcloud bot commented Apr 28, 2022

camilasan commented Apr 10, 2022 •

edited by claucambra

Loading

jancborchardt commented Apr 11, 2022 •

edited

Loading

claucambra commented Apr 21, 2022 •

edited

Loading

claucambra commented Apr 21, 2022 •

edited

Loading

jancborchardt commented Apr 21, 2022 •

edited

Loading

claucambra commented Apr 21, 2022 •

edited

Loading

jospoortvliet commented Apr 22, 2022 •

edited

Loading

claucambra commented Apr 22, 2022 •

edited

Loading

claucambra Apr 23, 2022 •

edited

Loading

claucambra Apr 23, 2022 •

edited

Loading

claucambra Apr 25, 2022 •

edited

Loading