Do not require internet connection for worker if only local subscriptions are used #389
Conversation
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
There was a problem hiding this comment.
Hmm, so this will only work if the person has only local ics files, right? Feels a bit off to me, to add this then. It would be a broken enhancement kind of.
I would rather do this properly and I think it's not that hard either if we had two sync workers. A LocalSyncWorker without network requirement and a NetworkSyncWorker, which then maybe both extend from an abstract base sync worker that contains the code in common.
What do you think?
|
Yeah, that would be more proper, but I didn't want to do such a big change. Do we want to do this? I think it will be even useful for DAVx⁵ in the future. |
|
Why did you not want to make a big change? There is no problem with big changes if they are well reasoned and we discuss them before they are implemented. I actually don't think it's that big of a change, but maybe I am bad at estimating. If it becomes too much maybe split it up in two PRs. One for refactoring, one for the feature / enhancement. Do you want to give it a try? My focus lies with DAVx5 at the moment as I wrote already, but I can review at some point 👍 |
|
Okay, I'll give it a try and see if it's simple enough, otherwise I'll try to split it. You have worked more than me on workers, so this will also be a good opportunity to develop with them a bit. |
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
There was a problem hiding this comment.
Looks promising and seems to work already, but there is a failing test and it's not really clear which worker does what.
I think we should rename SyncWorker to NetworkSyncWorker or something similar for clarity and also add some short KDoc to the worker classes, explaining what their purpose is and what for example the new "filter" property is for.
Also would be nice with an updated PR description, which gives an overview of the changes. :)
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
There was a problem hiding this comment.
I am not really a fan of the anonymous filter functions in the class constructors. Does not matter if it is more verbose, but maybe there is a more descriptive and clear way of doing it?
What about the periodic sync worker ? I am guessing people who use syncthing, probably still want to synchronize their local ics files periodically too ... Could be a new feature in a new PR, but we could add it here too. What do you think?
|
Yup, I agree with everything. I'll think about it for a bit, and see which approach I find for the filtering. Since the base sync class is open, maybe we can add an open function for the filter, instead of anonimizing it and then overriding the method on the upper class. For the sync, yup, periodic workers should absolutely be working IMO. Even better if we could somehow schedule it to observe changes on the local filesystem, though I don't know if it's possible. |
Add it to this PR then :)
Nice idea, but that should definetly go in a separate PR then. |
Purpose
See #385, in some rare cases where INTERNET permission is not always granted, if trying to synchronize subscriptions for local files, they don't run since they wait until a network connection is available.
Short description
There are now two workers, now one that only syncs local collections, and another one that synchronizes the network-dependant subscriptions.
This condition is checked with the scheme of the url. If it starts with "http" (which includes "https"), it's a network subscription, otherwise it's local.
This condition is passed to the
BaseSyncWorkerthrough a new argument calledfilter.Migration is only performed by the
NetworkSyncWorker, soONLY_MIGRATEis alwaysfalseforLocalSyncWorker. Note that since there are now two workers, they are scheduled, cancelled and their status is obtained through the companion functions ofBaseSyncWorker.Checklist