KOSync: Clarify settings, plus refactor & fixes to make "auto-sync" more reliable#10605
Conversation
|
Note to potential testers: please follow #10603, as the official sync server will be down for a few days ;). |
Also, tie "Sync every # pages" with auto sync, as it requires it.
As it's going to be naggy as hell once I'm done with it Also, until sync behavior from autosync, as it has nothing to do with it,; it's the sync behavior, *period*, manual syncs included.
Note that with a periodic sync often enough, it might be enough to overcome the network activity check, and leave WiFi enabled at all times...
(I, err, had completely forgotten that I'd even started to implement it at all...).
857485f to
fccdc11
Compare
And update the periodic sync menu entry on set
require the reader to be ready Also, stop leaking menu instances via the _menu_to_update hack And track page turns instance per instance, too.
(e.g., delay the push if we're skimming)
NiLuJe
left a comment
There was a problem hiding this comment.
Reviewed 5 of 7 files at r1, 3 of 5 files at r2, 3 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @NiLuJe)
The whole lifecycle stuff actually precludes any sort of async shenanigans without a lot more consideration, in fact.
(I mean, GC magic should handle some of the "zombie" stuff properly, but I'd be extremely, extremely wary of it, and it would look and feel super confusing, code-wise. (And set a precedent I strongly object to.)). TL;DR: This is probably the best it's ever going to be, and it's quite likely broken as hell with turbo enabled anyway ^^. |
i.e., chekc that we actually have *some* settings from that plugin, instead of force-feeding the defaults no matter what. (That'll happen on plugin load anyway ;p).
(I went low mostly to ease testing)
NiLuJe
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status:
|
|
||
| function KOSync:getSyncPeriod() | ||
| if not self.settings.auto_sync then | ||
| return _("Unavailable") |
There was a problem hiding this comment.
Probably better to use "Not available" (which is already translated), unless there's a specific reason to use unavailable.
|
Current behaviour: I am in a book, have "Automatically keep documents in sync" checked, wifi off. |
|
That's most likely gesture-specific (possibly we're missing an UI frame for the popup to actually show up), because this works via the Menu ;). I'll look into it, thanks! |
|
Err, wait, never mind, this should not work with "prompt" as the before wifi action, which getting a "Do you want to turn on wi-fi?" implies it's what it is set to? Unless you switched to prompt from inside a document, auto sync should have been disabled on document load in that case, so, I'm confused. (There may be Kindle-specific shenanigans involved, as I'm not entirely sure you actually have the choice of before wifi action on there...). |
The same when leaving the reader via the upper menu "file browser" icon.
No, we do not have it on Kindle. |
|
So after 30 s timeout the reader is closed, and in the file browser:
|
|
Oh, yeah, I was living in a happy dreamland where everything not Android was
I'll see if I can fudge before_wifi support on those platforms, because prompt will never ever work for this use-case (we'd need UI frames to tick, while also delaying event propagation to stop one in its track in its own handler. That's just plain not possible). |
…ore reliable (koreader#10605) Fix: koreader#10539, and for context koreader#6489, koreader#6733, koreader#6534 Reorganize and reword most of the settings to make it clear what actually ties into auto sync, and what doesn't. (Specifically, what happens when a pull attempts to sync forward or backward has nothing to do with auto sync, it applies in all cases; while the periodic sync *does* require auto sync). The main point of contention, though, is that auto sync will now *always* attempt to setup network connectivity (i.e., on resume/suspend/close). Periodic sync will *not* though (the intent being that, if you use periodic sync, you're relying on the activity check to actually keep wifi on at all times)). Since this may lead to a large amount of nagging about wifi toggles on devices w/ NetworkManager support, it is now *disabled* by default on those devices. (And given that it wouldn't have worked because of the lack of connectivity, that doesn't really make any practical difference ;p). Additionally, given the fact that there's no way to make this behavior viable if the "before wifi" action is left at its default of "prompt", this feature now *requires* that to be set to "turn_on" (on devices where it can, of course); attempting to toggle it on will warn about that if necessary. This change is retroactive (OTM). Includes an assortment of fixes and cleanups, including migrating to the new LuaSettings API, which is why there's no longer a smattering of superfluous flushes.
Fix: #10539, and for context #6489, #6733, #6534
Reorganize and reword most of the settings to make it clear what actually ties into auto sync, and what doesn't. (Specifically, what happens when a pull attempts to sync forward or backward has nothing to do with auto sync, it applies in all cases; while the periodic sync does require auto sync).
The main point of contention, though, is that auto sync will now always attempt to setup network connectivity (i.e., on resume/suspend/close). Periodic sync will not though (the intent being that, if you use periodic sync, you're relying on the activity check to actually keep wifi on at all times)).
Since this may lead to a large amount of nagging about wifi toggles on devices w/ NetworkManager support, it is now disabled by default on those devices. (And given that it wouldn't have worked because of the lack of connectivity, that doesn't really make any practical difference ;p).
Additionally, given the fact that there's no way to make this behavior viable if the "before wifi" action is left at its default of "prompt", this feature now requires that to be set to "turn_on" (on devices where it can, of course); attempting to toggle it on will warn about that if necessary.
This change is retroactive (OTM).
Includes an assortment of fixes and cleanups, including migrating to the new LuaSettings API, which is why there's no longer a smattering of superfluous flushes.
RFC, because I don't actually use the plugin.
This change is