Stop synchronisation when behind a captive portal #11567

erikjv · 2024-03-28T17:33:51Z

erikjv · 2024-03-28T17:34:55Z

The SpacesManager still needs to be hooked up. So maybe the Utility code needs to move to libsync.

src/gui/guiutility.cpp

src/gui/scheduling/syncscheduler.cpp

TheOneRing · 2024-04-05T09:11:16Z

src/gui/accountstate.cpp

+        if (onoff) {
+            _queueGuard.block();
+        } else {
+            // TODO: empty queue?


I'm not sure. It might depend on how long we were blocked, 10s or 20h...

I'm still not sure what to do with this

Lets clear it.

TheOneRing · 2024-04-05T09:12:03Z

src/gui/accountstate.cpp

+    if (_account->accessManager()->isBehindCaptivePortal()) {
+        _queueGuard.block();
+    } else {
+        // TODO: empty queue?


I don't think this unblock has any use?

How can we be blocked during the construction of the class?

TheOneRing · 2024-04-05T09:13:23Z

src/gui/accountstate.cpp

@@ -247,6 +263,7 @@ void AccountState::setState(State state)
        } else if (_state == Connected && Utility::internetConnectionIsMetered() && ConfigFile().pauseSyncWhenMetered()) {
            _state = PausedDueToMetered;
        }
+        // Do we need an extra state PausedDueToCaptivePortal?


For now we could reuse "Connecting"? In the end its only to display a different string? In which case we could make the "connecting" message conditional on isBehindCaptivePortal

src/gui/main.cpp

src/gui/owncloudgui.cpp

TheOneRing

For a manual testing action, it has quite a heavy impact on the client...

src/gui/owncloudgui.cpp

src/libsync/accessmanager.cpp

TheOneRing · 2024-04-05T09:21:42Z

Brainstorming: We have multiple network information things to test, could it make sense to implement a custom one that wraps the native one and allows overriding values?

TheOneRing · 2024-04-16T10:16:15Z

Please rebase, once done I can start a review.

src/gui/networkinformation.cpp

TheOneRing · 2024-04-16T14:20:31Z

src/gui/accountstate.cpp

+    connect(NetworkInformation::instance(), &NetworkInformation::isBehindCaptivePortalChanged, this, [this](bool) {
+        // A direct connect is not possible, because then the state parameter of `isBehindCaptivePortalChanged`
+        // would become the `verifyServerState` argument to `checkConnectivity`.
+        QTimer::singleShot(0, this, [this] { checkConnectivity(false); });


Does it make sense to run the check if it changed to true?

Yes, it sets the state to Connecting and triggers the signals so the UI shows paused and captive portal.

Could you add that as a comment? I see the possibility that someone might "Optimize" that call.

Fixes: #11533

TheOneRing · 2024-04-18T09:25:57Z

test/testutils/testutils.cpp

@@ -27,6 +28,8 @@ namespace OCC {
 namespace TestUtils {
    TestUtilsPrivate::AccountStateRaii createDummyAccount()
    {
+        // ensure we have an instance of NetworkInformation
+        NetworkInformation::initialize();


Should we move that to https://github.com/owncloud/client/blob/master/test/testutils/testutilsloader.cpp#L9

TheOneRing · 2024-04-18T09:30:00Z

src/gui/networkinformation.cpp

+
+NetworkInformation *NetworkInformation::_instance = nullptr;
+
+static void loadQNetworkInformationBackend()


anonymous namespace please

TheOneRing · 2024-04-18T09:31:38Z

src/gui/networkinformation.cpp

+}
+
+void NetworkInformation::initialize()
+{


Do we need the explicit initialize call or could it make sense to merge it with the instance call?

TheOneRing · 2024-04-18T09:49:12Z

src/gui/accountstate.cpp

+        if (onoff) {
+            _queueGuard.block();
+        } else {
+            // TODO: empty queue?


Lets clear it.

TheOneRing · 2024-04-18T09:49:48Z

src/gui/accountstate.cpp

+    if (_account->accessManager()->isBehindCaptivePortal()) {
+        _queueGuard.block();
+    } else {
+        // TODO: empty queue?


How can we be blocked during the construction of the class?

Like we do for when the client is behind a captive portal.

erikjv requested a review from TheOneRing March 28, 2024 17:33

erikjv self-assigned this Mar 28, 2024

erikjv marked this pull request as draft March 28, 2024 17:34

TheOneRing reviewed Apr 2, 2024

View reviewed changes

src/gui/guiutility.cpp Outdated Show resolved Hide resolved

TheOneRing reviewed Apr 2, 2024

View reviewed changes

src/gui/scheduling/syncscheduler.cpp Outdated Show resolved Hide resolved

erikjv force-pushed the work/pause-sync-behind-captive-portal branch 3 times, most recently from 2728dbe to 7dd72f4 Compare April 4, 2024 15:10

TheOneRing reviewed Apr 5, 2024

View reviewed changes

src/gui/main.cpp Outdated Show resolved Hide resolved

TheOneRing reviewed Apr 5, 2024

View reviewed changes

src/gui/owncloudgui.cpp Show resolved Hide resolved

TheOneRing requested changes Apr 5, 2024

View reviewed changes

src/gui/owncloudgui.cpp Show resolved Hide resolved

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

erikjv force-pushed the work/pause-sync-behind-captive-portal branch from 7dd72f4 to f10d5eb Compare April 11, 2024 15:44

erikjv requested a review from TheOneRing April 11, 2024 15:45

erikjv force-pushed the work/pause-sync-behind-captive-portal branch 2 times, most recently from 03c2325 to 4e85060 Compare April 15, 2024 13:56

erikjv marked this pull request as ready for review April 15, 2024 14:03

erikjv force-pushed the work/pause-sync-behind-captive-portal branch from 5b1e182 to 412851f Compare April 16, 2024 13:55

TheOneRing reviewed Apr 16, 2024

View reviewed changes

erikjv added 2 commits April 17, 2024 15:28

Introduce the OCC::NetworkInformation class

2cf4a80

Rename crashMenu to debugMenu

d8d5c31

erikjv force-pushed the work/pause-sync-behind-captive-portal branch from 412851f to 07efac2 Compare April 17, 2024 13:28

erikjv requested a review from TheOneRing April 17, 2024 14:00

erikjv force-pushed the work/pause-sync-behind-captive-portal branch from 07efac2 to 8920af8 Compare April 17, 2024 15:04

Stop synchronisation when behind a captive portal

d65d66a

Fixes: #11533

erikjv force-pushed the work/pause-sync-behind-captive-portal branch from 8920af8 to fdde86d Compare April 17, 2024 17:32

TheOneRing requested changes Apr 18, 2024

View reviewed changes

Remove PausedDueToMetered state and use Connecting state

e6a9593

Like we do for when the client is behind a captive portal.

erikjv force-pushed the work/pause-sync-behind-captive-portal branch from fdde86d to e6a9593 Compare April 18, 2024 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop synchronisation when behind a captive portal #11567

Stop synchronisation when behind a captive portal #11567

erikjv commented Mar 28, 2024

erikjv commented Mar 28, 2024

TheOneRing Apr 5, 2024

erikjv Apr 17, 2024

TheOneRing Apr 18, 2024

TheOneRing Apr 5, 2024

TheOneRing Apr 18, 2024

TheOneRing Apr 5, 2024

TheOneRing left a comment

TheOneRing commented Apr 5, 2024

TheOneRing commented Apr 16, 2024

TheOneRing Apr 16, 2024

erikjv Apr 17, 2024

TheOneRing Apr 18, 2024

TheOneRing Apr 18, 2024

TheOneRing Apr 18, 2024

TheOneRing Apr 18, 2024

TheOneRing Apr 18, 2024

TheOneRing Apr 18, 2024


		NetworkInformation *NetworkInformation::_instance = nullptr;

		static void loadQNetworkInformationBackend()

Stop synchronisation when behind a captive portal #11567

Are you sure you want to change the base?

Stop synchronisation when behind a captive portal #11567

Conversation

erikjv commented Mar 28, 2024

erikjv commented Mar 28, 2024

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

TheOneRing left a comment

Choose a reason for hiding this comment

TheOneRing commented Apr 5, 2024

TheOneRing commented Apr 16, 2024

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