Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync: use constants instead of authorities where applicable #880

Open
rfc2822 opened this issue Jul 3, 2024 · 4 comments
Open

Sync: use constants instead of authorities where applicable #880

rfc2822 opened this issue Jul 3, 2024 · 4 comments
Assignees
Labels
refactoring Internal improvement of existing functions

Comments

@rfc2822
Copy link
Member

rfc2822 commented Jul 3, 2024

We have many locations where we use the various authorities in the meaning of the Android sync framework to specify what to synchronize.

However, we don't use the sync framework anymore for most syncs, and also the mapping of authorities to what shall be synchronized is not 1:1:

  • Address books authority (R.string.address_books_authority) – Needed because main accounts are not synchronized with the ContactsContract.AUTHORITY. This is because a main account doesn't contain contacts directly, but only address book accounts, which then contain the contacts. So if we want to sync all address books of an account, we currently use the address books authority.

  • CalendarContract.AUTHORITY – Currently used as intended without extra workarounds.

  • Task authorities for jtxBoard, tasks.org and OpenTasks – Instead, we often want to just have "tasks". For instance, there should be a tasks sync interval, and not a jtxBoard sync interval, tasks.org sync interval etc. So most occurrences of these authorities will have to be replaced by a "tasks" constant.

  • Go through all occurences of authorities and see whether the location really requires an authority in the sync framework meaning or should be replaced by a {contacts, calendar, tasks} enum or something like that.

    • Probably change AccountSettings sync interval getters/setters to not require authorities.
    • Probably change the workers to accept the enum instead of authorities.
  • Check/adapt TasksAppWatcher

Related or sub-tasks:

@rfc2822 rfc2822 added the refactoring Internal improvement of existing functions label Jul 3, 2024
@sunkup
Copy link
Member

sunkup commented Oct 15, 2024

What should we call these constants? I think it would be good to call them something else than authority, so we don't get an overlap in meaning here.

@rfc2822
Copy link
Member Author

rfc2822 commented Oct 15, 2024

Hm, maybe domain, but that's also already taken by domain names.

  • Sync data type?
  • Provider type?

@sunkup sunkup self-assigned this Oct 16, 2024
@sunkup sunkup linked a pull request Nov 19, 2024 that will close this issue
4 tasks
@sunkup
Copy link
Member

sunkup commented Nov 19, 2024

I actually think it makes sense to keep passing the authorities around. We still need them in many places and if we introduce additional constants now (even if only for the get and set sync-interval methods) we will end up translating forth and back in muliple places adding IMO unecessary complexity.

Most problematic example is that Tasks authorities (OpenTasks, TasksOrg) need to be passed from SyncAdapter through BaseSyncWorker to Syncer and SyncManager in order to acquire the correct content provider and build view item action for notifications.

Swapping the calendar and address books authorities with constants could work, but is a lot of extra code to translate between authorities and constants. Getting and setting the sync interval for those two also involves ContentResolver calls which require the actual authorities.

It might be easier once the sync adapter is replaced with content triggered syncs.

@sunkup sunkup removed their assignment Nov 20, 2024
@rfc2822
Copy link
Member Author

rfc2822 commented Nov 22, 2024

I actually think it makes sense to keep passing the authorities around. We still need them in many places and if we introduce additional constants now (even if only for the get and set sync-interval methods) we will end up translating forth and back in muliple places adding IMO unecessary complexity.

But get and set sync interval is a good example – it should be independent of the actual task authority.

Just had the case during testing that I switched from task app A to task app B, and there were still syncs for app A, which should not happen. I suspect because the sync framework directly passes the authority to the sync worker, and sync was not stopped in the sync framework properly.

Having "sync domains" instead of authorities would not solve the problem with the sync framework, but make sure that only the correct authority is synced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Internal improvement of existing functions
Projects
Status: Todo
2 participants