search

bitfireAT / icsx5

ICSx⁵ is an Android app to subscribe to remote or local iCalendar files (like time tables of your school/university or event files of your sports team).
https://icsx5.bitfire.at
GNU General Public License v3.0
179 stars 8 forks source link

Do not require internet connection for worker if only local subscriptions are used #389

Open ArnyminerZ opened 2 months ago

ArnyminerZ commented 2 months ago

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 BaseSyncWorker through a new argument called filter.

Migration is only performed by the NetworkSyncWorker, so ONLY_MIGRATE is always false for LocalSyncWorker. Note that since there are now two workers, they are scheduled, cancelled and their status is obtained through the companion functions of BaseSyncWorker.

Checklist

ArnyminerZ commented 2 months ago

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.

sunkup commented 2 months ago

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 :+1:

ArnyminerZ commented 2 months ago

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.

ArnyminerZ commented 2 months ago

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.

sunkup commented 2 months ago

For the sync, yup, periodic workers should absolutely be working IMO.

Add it to this PR then :)

Even better if we could somehow schedule it to observe changes on the local filesystem, though I don't know if it's possible.

Nice idea, but that should definetly go in a separate PR then.