Add push notifications for file changes #2814

FlexW · 2021-01-12T08:26:40Z

No description provided.

er-vin

A couple more things:

commit style wise, I'd have made this in three commits: one for the credentials changes, one for the capabilities additions and one for wiring the new capability to the websocket and folderman;
design wise, I think it'd make sense to move the websocket out of AccountState and instead add a new class PushNotification which would be constructed with an Account object, this would help wrapping almost all the push notification related code in its own class, this would also allow to have that new class in libsync and so potentially easier to test, then AccountState could decide to construct an instance of it and expose it to the outside if the capability is there.

src/gui/accountstate.cpp

src/gui/accountstate.h

src/gui/folderman.cpp

src/libsync/CMakeLists.txt

src/libsync/capabilities.cpp

src/libsync/capabilities.h

src/libsync/creds/dummycredentials.cpp

FlexW · 2021-01-12T16:23:53Z

design wise, I think it'd make sense to move the websocket out of AccountState and instead add a new class PushNotification which would be constructed with an Account object

You mean the PushNotification object should be constructed if a AccountState object is constructed not an Account object?

er-vin · 2021-01-13T08:17:18Z

design wise, I think it'd make sense to move the websocket out of AccountState and instead add a new class PushNotification which would be constructed with an Account object

You mean the PushNotification object should be constructed if a AccountState object is constructed not an Account object?

Indeed I was unclear. What I meant is: the AccountState object makes the decision to construct PushNotification or not, also PushNotification receives an Account object as parameter during construction.

src/gui/accountstate.cpp

src/gui/accountstate.h

src/libsync/pushnotifications.h

test/testpushnotifications.cpp

test/testwebsocket.cpp

allexzander · 2021-01-14T12:53:46Z

src/libsync/capabilities.cpp

@@ -176,6 +176,25 @@ bool Capabilities::chunkingNg() const
    return _capabilities["dav"].toMap()["chunking"].toByteArray() >= "1.0";
 }

+PushNotificationTypes Capabilities::pushNotificationsAvailable() const


@er-vin @felixboehm
I don't know why, but, for me, when I see this PushNotificationTypes it makes me think of PushNotificationTypes as some sort of aggregate, while in fact, it's just an enum. I've immediately started to look for push_back() or insert().

Its's some sort of aggregate I think. It can hold multiple flags. How would you name it?

@FlexW Got no idea right now :(

It's fairly common in my experience to have a singular for the enum and a plural for the corresponding flag. Now admittedly a "Types" flag is less common because in our minds types tend to be mutually exclusive (you're either of a type or of another not of two types). There are precedent though (qtbase alone has three such "Types" flags).

If we're concerned that "Types" could create confusion potential alternatives (not saying we should change, but just in case we do change then we got ideas): PushNotificationAreas, PushNotificationDomains, PushNotificationChannels (with the backing enum being respectively PushNotificationArea, PushNotificationDomain and PushNotificationChannel of course).

Then I will leave it as it is?

To be clear: I'm personally fine with it.

Now I can understand if it's unclear for someone else, which is what @allexzander expressed hence why I made proposals for alternative names.

@allexzander should @FlexW go for one of the proposals I made? Or we stay with "Types"?

src/libsync/capabilities.cpp

src/libsync/pushnotifications.cpp

allexzander · 2021-01-14T14:05:52Z

src/libsync/pushnotifications.cpp

+
+void PushNotifications::onWebSocketSslErrors(const QList<QSslError> &errors)
+{
+    // TODO: What to do with them?


Having a log here would be something, to begin with. Just to make sure, later if we have some weird issue happening on a customer's end, we could just request the logs without having to roll-out an update with logs added :)

Well, that's what qWarning does, but it'd need to be a qCWarning. ;-)

Also should be treated like an auth error, we just bail out and go back to the timer.

What exaclty does bail out and go back to the timer mean?

src/libsync/websocket.cpp

src/gui/accountstate.cpp

src/gui/accountstate.h

src/gui/folderman.cpp

test/testpushnotifications.cpp

er-vin · 2021-01-18T08:31:27Z

/rebase

src/gui/folderman.h

src/gui/accountstate.cpp

src/libsync/pushnotifications.cpp

src/libsync/pushnotifications.h

er-vin · 2021-01-18T12:15:36Z

test/testpushnotifications.cpp

+        QCOMPARE(spy.count(), 2);
+        const auto socket = spy.at(0).at(0).value<QWebSocket *>();
+        const auto firstPasswordSent = spy.at(1).at(1).toString();
+        QCOMPARE(firstPasswordSent, password);


Suggestion: This comes back very often, what about factoring out the part related to waiting for the authentication out in a function? This way we could have some of the tests written from the perspective of an already setup PushNotifications object.

Yes, that would make sense

test/testpushnotifications.cpp

src/gui/folderman.cpp

test/pushnotificationstestutils.cpp

test/testaccount.cpp

test/testpushnotifications.cpp

test/testaccount.cpp

er-vin

We're really close now. Good job.

src/libsync/account.cpp

src/gui/folderman.cpp

src/libsync/pushnotifications.cpp

src/libsync/account.cpp

er-vin

Looks like you didn't intend to push that latest commit, or?

src/libsync/pushnotifications.cpp

FlexW · 2021-01-21T17:22:11Z

The current version does now contain exactly what I have on my computer. You are welcome to try it yourself if you also get the authentication message but no other;)

FlexW · 2021-01-21T21:43:06Z

Thanks. I have now tested against cloud.nextcloud.com and everything works as expected (except for the problem with the name). One last doubt that I have is, if we loose any file changes if there are a lot of really fast file notification events but I have not encountered such an issue in my tests.

er-vin · 2021-01-25T08:21:16Z

Thanks. I have now tested against cloud.nextcloud.com and everything works as expected (except for the problem with the name). One last doubt that I have is, if we loose any file changes if there are a lot of really fast file notification events but I have not encountered such an issue in my tests.

Well, if they're fast enough that they are within the 2s delay after you trigger from the push notification they'll be caught by the sync anyway... if they occur during the sync we'd have scheduled another sync right after the ongoing one anyway, so I think it'd be fine.

er-vin · 2021-01-25T08:24:41Z

Thanks. I have now tested against cloud.nextcloud.com and everything works as expected (except for the problem with the name). One last doubt that I have is, if we loose any file changes if there are a lot of really fast file notification events but I have not encountered such an issue in my tests.

Well, if they're fast enough that they are within the 2s delay after you trigger from the push notification they'll be caught by the sync anyway... if they occur during the sync we'd have scheduled another sync right after the ongoing one anyway, so I think it'd be fine.

OK... Actually I was wrong, I thought I remember we were going through the sync scheduling mechanism but that's not the case... I'll add a comment in the code to potentially deal with this.

er-vin · 2021-01-25T08:28:33Z

src/gui/folderman.cpp

+        }
+
+        qCInfo(lcFolderMan) << "Run etag job if possible on " << folder;
+        runEtagJobIfPossible(folder);


So indeed we could have an issue there, if the folder is syncing already or blocked by another one we might loose the event. This is both good and bad. Good because it avoids syncing too many times for nothing if you receive a burst of such notifications, bad because you might miss a change (I'd say chances are slim but will increase with the amount of files in the folder).

I think we might want to call scheduleFolder or scheduleFolderNext here instead.

I would go for scheduleFolder() because it does not add the folder to the queue if it's already added.

Sounds fine.

test/testpushnotifications.cpp

er-vin · 2021-01-25T17:00:10Z

/rebase

Resolves #2802 Signed-off-by: Felix Weilbach <felix.weilbach@nextcloud.com>

nextcloud-desktop-bot · 2021-01-25T17:06:48Z

AppImage file: Nextcloud-PR-2814-78f00acaa2683cf983738c3194503ad40b9df689-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.

er-vin · 2021-01-25T17:17:50Z

/backport to stable-3.1

FlexW force-pushed the files_push_notifications branch from b2cf7ee to 8e23481 Compare January 12, 2021 08:29

FlexW requested a review from er-vin January 12, 2021 08:35

FlexW force-pushed the files_push_notifications branch 3 times, most recently from faac21c to 171efad Compare January 12, 2021 13:40

er-vin requested changes Jan 12, 2021

View reviewed changes

er-vin requested changes Jan 13, 2021

View reviewed changes

allexzander reviewed Jan 14, 2021

View reviewed changes

src/libsync/capabilities.cpp Outdated Show resolved Hide resolved

allexzander reviewed Jan 14, 2021

View reviewed changes

src/libsync/pushnotifications.cpp Outdated Show resolved Hide resolved

allexzander reviewed Jan 14, 2021

View reviewed changes

src/libsync/pushnotifications.cpp Outdated Show resolved Hide resolved

allexzander reviewed Jan 14, 2021

View reviewed changes

src/libsync/websocket.cpp Outdated Show resolved Hide resolved

er-vin reviewed Jan 14, 2021

View reviewed changes

github-actions bot force-pushed the files_push_notifications branch from 19135ba to 692026c Compare January 18, 2021 08:32

er-vin reviewed Jan 18, 2021

View reviewed changes

er-vin reviewed Jan 20, 2021

View reviewed changes

er-vin reviewed Jan 21, 2021

View reviewed changes

src/libsync/pushnotifications.cpp Outdated Show resolved Hide resolved

src/libsync/pushnotifications.cpp Outdated Show resolved Hide resolved

er-vin reviewed Jan 25, 2021

View reviewed changes

FlexW force-pushed the files_push_notifications branch 2 times, most recently from 5bdb07e to 581a4a7 Compare January 25, 2021 14:40

er-vin requested changes Jan 25, 2021

View reviewed changes

test/testpushnotifications.cpp Outdated Show resolved Hide resolved

test/testpushnotifications.cpp Outdated Show resolved Hide resolved

FlexW force-pushed the files_push_notifications branch 2 times, most recently from 6d8e9d9 to 1282657 Compare January 25, 2021 16:54

er-vin approved these changes Jan 25, 2021

View reviewed changes

Add push notifications for file changes

78f00ac

Resolves #2802 Signed-off-by: Felix Weilbach <felix.weilbach@nextcloud.com>

github-actions bot force-pushed the files_push_notifications branch from 1282657 to 78f00ac Compare January 25, 2021 17:01

er-vin merged commit 8c66b9f into master Jan 25, 2021

er-vin deleted the files_push_notifications branch January 25, 2021 17:17

backportbot-nextcloud bot added the backport-request label Jan 25, 2021

backportbot-nextcloud bot mentioned this pull request Jan 25, 2021

[stable-3.1] Add push notifications for file changes #2866

Merged

backportbot-nextcloud bot removed the backport-request label Jan 25, 2021

camilasan mentioned this pull request Feb 9, 2021

Desktop client starts checking for local changes every 30 seconds #2911

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add push notifications for file changes #2814

Add push notifications for file changes #2814

FlexW commented Jan 12, 2021

er-vin left a comment

FlexW commented Jan 12, 2021

er-vin commented Jan 13, 2021

allexzander Jan 14, 2021

FlexW Jan 14, 2021

allexzander Jan 14, 2021

er-vin Jan 14, 2021

FlexW Jan 20, 2021

er-vin Jan 20, 2021

allexzander Jan 14, 2021

er-vin Jan 14, 2021

FlexW Jan 15, 2021

er-vin commented Jan 18, 2021

er-vin Jan 18, 2021

FlexW Jan 20, 2021

er-vin left a comment

er-vin left a comment

FlexW commented Jan 21, 2021

FlexW commented Jan 21, 2021

er-vin commented Jan 25, 2021

er-vin commented Jan 25, 2021

er-vin Jan 25, 2021

FlexW Jan 25, 2021

er-vin Jan 25, 2021

er-vin commented Jan 25, 2021

nextcloud-desktop-bot commented Jan 25, 2021

er-vin commented Jan 25, 2021

Add push notifications for file changes #2814

Add push notifications for file changes #2814

Conversation

FlexW commented Jan 12, 2021

er-vin left a comment

Choose a reason for hiding this comment

FlexW commented Jan 12, 2021

er-vin commented Jan 13, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

er-vin commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

er-vin left a comment

Choose a reason for hiding this comment

er-vin left a comment

Choose a reason for hiding this comment

FlexW commented Jan 21, 2021

FlexW commented Jan 21, 2021

er-vin commented Jan 25, 2021

er-vin commented Jan 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

er-vin commented Jan 25, 2021

nextcloud-desktop-bot commented Jan 25, 2021

er-vin commented Jan 25, 2021