Fix pause sync states #2209

DominiqueFuchs · 2020-07-17T13:21:22Z

Changes:

Fix of a (apparently) copy/paste bug where the same signal (pause) was used for resume and pause slots.
Proper initial setting of _syncIsPaused state variable on Systray creation method (e.g. when accounts are populated and set up, if existing) to ensure correct qml gui dropdown entry text

er-vin · 2020-07-20T12:33:58Z

src/gui/systray.cpp

@@ -105,6 +105,11 @@ void Systray::create()
    }
    hideWindow();
    emit activated(QSystemTrayIcon::ActivationReason::Unknown);
+    foreach (Folder *f, FolderMan::instance()->map()) {


Please use a for range loop, foreach is a deprecated "Qtism" (and we can expect that macro to disappear post-5.15).
Another option is to use std::any_of.
In both cases it's likely a good idea to hold the result of the ->map() call in a const auto variable.

You're right, thanks! Also makes a point for the 80-100 foreach uses in the rest of the code base, glimpsing through that lets assume that some deep copies could be saved -> #2219

er-vin · 2020-07-21T08:04:44Z

src/gui/systray.cpp

@@ -105,6 +105,11 @@ void Systray::create()
    }
    hideWindow();
    emit activated(QSystemTrayIcon::ActivationReason::Unknown);
+    for (const auto *folderMap : FolderMan::instance()->map()) {


Sorry I guess I was being unclear, this is the right hand side of the : which should be wrapped into qAsConst() or stored in a const auto variable. Currently this might detach.

Didn't think of detaching b/c we're receiving a QMap here but I get your point now. But: Why not making the map method return const then as const OCC::Folder::Map map() instead of "casting" it? As it's whole purpose is to essentially generate an iterator for an independent class datablock.

Because then for any non const use you'd force a copy for nothing NRVO wouldn't kick in. Returning const data objects is the exception not the norm in most cases it's not what you want.

Rule of thumbs: non const -> const "good", const -> non const "bad". Reality is finer than that but it's a good heuristic. :-)

And one more thing: yes, you'd hope you don't need the qAsConst or the intermediate const variable (at least personally I find that annoying nowadays). Unfortunately Qt containers are CoW, there was a good reason for that, it's less of a good reason now that move semantic appeared but they can't just be changed to be non-CoW now. So we end up dealing with that legacy...

Yeah thats obvious, my point was that the map() really should be const imho. If you attempt to use map() to modify it, this is not how the current code was built to be used afaik.

But nvmd, for this particular snippet it doesn‘t make difference, rather style, I‘ll go add a qAsConst later..

Yeah thats obvious, my point was that the map() really should be const imho. If you attempt to use map() to modify it, this is not how the current code was built to be used afaik.

I think you're having a slight confusion here: it's perfectly fine to get the map() and modify it (and so shouldn't be prevented). It's a data type returned by value. I'd be worried if it was a pointer or a reference being returned. There's no risk of breaking the encapsulation here.

I'd even argue it's bad API design to try to make it harder to modify the object returned by map(). It'd be perfectly legitimate for instance to get the object returned by map() and then want to use std::remove_if on it (or whatever) to keep only the entries for which the key start with a given prefix (I'm making up the example but you get the idea).

OK turns out qAsConst was implicitly deleted for these kinds of rvalues (in this case QMap). Pulled it out into an extra const auto now.

Oh right, qAsConst shouldn't be used on rvalues indeed, my bad.

er-vin · 2020-07-21T10:09:17Z

/rebase

…stead of ::resumeSync Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>

… done Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>

er-vin · 2020-07-21T10:26:53Z

src/gui/systray.cpp

+
+    const auto folderMap = FolderMan::instance()->map();
+    for (const auto *folder : folderMap) {
+        if(!folder->syncPaused()) {


Somehow it wasn't here before or I spot it only now: please add a space after if

er-vin · 2020-07-21T10:27:32Z

src/gui/systray.cpp

+    const auto folderMap = FolderMan::instance()->map();
+    for (const auto *folder : folderMap) {
+        if(!folder->syncPaused()) {
+            _syncIsPaused = false;


Similarly, my brain kicks in only now... sigh.

You can break the loop after that, no need to iterate further on the map.

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>

nextcloud-desktop-bot · 2020-07-21T10:57:42Z

AppImage file: Nextcloud-PR-2209-a07db657d5cf1073b99122aa1345e05283ce0abd-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 requested review from er-vin, misch7 and camilasan July 17, 2020 13:21

DominiqueFuchs mentioned this pull request Jul 17, 2020

Systray account state indicators: What states to show here? [pause / etc] #2210

Open

er-vin requested changes Jul 20, 2020

View reviewed changes

er-vin added this to the 2.7 🌟 UI improvements milestone Jul 20, 2020

DominiqueFuchs requested a review from er-vin July 20, 2020 17:56

er-vin requested changes Jul 21, 2020

View reviewed changes

DominiqueFuchs added 4 commits July 21, 2020 10:10

Fixed copy/paste bug, resulting in a wrong signal ::pauseSync used in…

5ac3099

…stead of ::resumeSync Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>

Properly set syncPause state on Systray creation when account init is…

82c70f0

… done Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>

Replaced Qt foreach by C++11 ranged for loop

dbdf40e

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>

Get a const auto for the FolderMap to iterate over

cd3a728

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>

github-actions bot force-pushed the fix-pause-sync-states branch from 59b7733 to cd3a728 Compare July 21, 2020 10:10

er-vin requested changes Jul 21, 2020

View reviewed changes

sigh, too

a07db65

Signed-off-by: Dominique Fuchs <32204802+DominiqueFuchs@users.noreply.github.com>

er-vin approved these changes Jul 21, 2020

View reviewed changes

er-vin merged commit f8fb264 into master Jul 21, 2020

er-vin deleted the fix-pause-sync-states branch July 21, 2020 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pause sync states #2209

Fix pause sync states #2209

DominiqueFuchs commented Jul 17, 2020

er-vin Jul 20, 2020

DominiqueFuchs Jul 20, 2020

er-vin Jul 21, 2020

DominiqueFuchs Jul 21, 2020

er-vin Jul 21, 2020

er-vin Jul 21, 2020 •

edited

Loading

DominiqueFuchs Jul 21, 2020

er-vin Jul 21, 2020

DominiqueFuchs Jul 21, 2020

er-vin Jul 21, 2020

er-vin commented Jul 21, 2020

er-vin Jul 21, 2020

er-vin Jul 21, 2020

nextcloud-desktop-bot commented Jul 21, 2020

Fix pause sync states #2209

Fix pause sync states #2209

Conversation

DominiqueFuchs commented Jul 17, 2020

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 Jul 21, 2020 • 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

er-vin commented Jul 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nextcloud-desktop-bot commented Jul 21, 2020

er-vin Jul 21, 2020 •

edited

Loading