Jean-Luc Makiola
13cb27b2ab
The vendored dmfs provider was kept on the grounds that it hands us the sync bookkeeping for free. The phase-1 sync audit measured that bookkeeping and found most of it broken, absent, or unusable: _DIRTY not set on delete, no home for a per-collection sync token, read-only collections inexpressible, ACCOUNT_TYPE write-once so enabling sync is a full migration, and cleanUpLists able to delete a user's lists after a backup restore. Sixteen findings are provider-imposed rather than platform- or protocol-imposed. Costing the alternative showed the swap is far smaller than assumed. TasksDataSource is already a 14-method, domain-shaped interface; exactly one file above the data layer references TasksContract. The work is a second implementation behind an interface built for it, not a rewrite. Against ~5 weeks to build, owning the store removes 2.5-4 weeks from the sync plan, and 8,200 of the vendored 14,555 lines are things we would never write - 23 migrations from a 2013 schema, 798 lines of full-text search the app has zero call sites for, and 1,581 lines of a type-safe layer over ContentValues that Room deletes. External mode (OpenTasks, tasks.org) is unaffected and keeps every file that describes somebody else's schema. STORAGE-DECISION.md is the reasoning; OWN-STORE.md is the architecture and the six-phase plan. :provider stays in-tree until phase 5 so recurrence parity can be tested against it before it goes. Also corrected here: the provider's JVM test count (51 -> 56, measured from the test-results XML) and a fourth site of the debunked "switching sync on is never a migration" claim, in StorageMode.kt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
193 lines
11 KiB
Markdown
193 lines
11 KiB
Markdown
# `:provider` — provenance
|
||
|
||
This module is **not our code**. It is the dmfs task provider, vendored, with
|
||
its namespace renamed to ours and a short list of changes recorded below.
|
||
|
||
| | |
|
||
|---|---|
|
||
| Upstream | [dmfs/opentasks](https://github.com/dmfs/opentasks) |
|
||
| Module taken | `opentasks-provider`, plus `opentasks-contract` (see [Why the contract came along](#why-the-contract-came-along)) |
|
||
| Version | `1.4.2` |
|
||
| Commit | `49ebf80b1eeee52a611e5a22f24f849852a6255f` (2021-03-21) |
|
||
| License | Apache-2.0 — see [`LICENSE`](LICENSE) and [`NOTICE`](NOTICE), both upstream's, unmodified |
|
||
| Database version | **23** |
|
||
|
||
Agendula itself is MIT. Apache-2.0 into MIT is fine in that direction, but this
|
||
module keeps its own `LICENSE`, `NOTICE`, and per-file Apache headers, and those
|
||
must survive any future edit here.
|
||
|
||
**1.4.2 specifically, for the database version.** DB 23 is the first to carry
|
||
`is_recurring`. tasks.org's fork is DB 22 and lacks it — which is why
|
||
`TaskMapper.task` on the app side derives recurrence from `rrule`/`rdate`
|
||
instead of trusting that column, and why it must keep doing so as long as
|
||
External mode supports tasks.org.
|
||
|
||
## Why vendored at all
|
||
|
||
Recorded properly in [`docs/STORAGE-AND-SYNC.md`](../docs/STORAGE-AND-SYNC.md);
|
||
in one line: **the permission names are hardcoded in the upstream AAR's
|
||
manifest.** No prebuilt artifact — Maven Central, JitPack, anything — can have
|
||
them renamed without `tools:` node surgery, and shipping under dmfs's own
|
||
permission names would make Agendula and OpenTasks mutually uninstallable
|
||
(`INSTALL_FAILED_DUPLICATE_PERMISSION`). In-tree also satisfies F-Droid's
|
||
from-source requirement, which a JitPack artifact would not.
|
||
|
||
In-tree rather than a git submodule, unlike floret-kit: we co-develop the kit,
|
||
whereas this is a fork we expect to resync from upstream approximately never.
|
||
|
||
## The namespace rename
|
||
|
||
Everything in this table is a rename and nothing more. The **contract shape is
|
||
untouched** — same tables, same column names, same URI paths — because that
|
||
shape is what our data layer, and every CalDAV engine, already speaks. We own
|
||
the namespace it lives in, not the schema.
|
||
|
||
| | Upstream | Ours |
|
||
|---|---|---|
|
||
| Authority | `org.dmfs.tasks` | `de.jeanlucmakiola.agendula.tasks` |
|
||
| Read permission | `org.dmfs.permission.READ_TASKS` | `de.jeanlucmakiola.agendula.permission.READ_TASKS` |
|
||
| Write permission | `org.dmfs.permission.WRITE_TASKS` | `de.jeanlucmakiola.agendula.permission.WRITE_TASKS` |
|
||
| Permission group | `org.dmfs.tasks.permissiongroup.Tasks` | `de.jeanlucmakiola.agendula.permissiongroup.Tasks` |
|
||
| Notification-alarm action | `org.dmfs.tasks.provider.NOTIFICATION_ALARM` | `de.jeanlucmakiola.agendula.provider.NOTIFICATION_ALARM` |
|
||
| Resource prefix | `opentasks_*` | `agendula_*` |
|
||
| R class | `org.dmfs.tasks.provider.R` | `de.jeanlucmakiola.agendula.provider.R` |
|
||
|
||
### What was deliberately *not* renamed
|
||
|
||
- **Java package names** stay `org.dmfs.provider.tasks` / `org.dmfs.tasks.contract`.
|
||
They are not a registered namespace — two apps may share them freely — and
|
||
keeping them means the diff against upstream stays legible. Only the AGP
|
||
`namespace` (which decides where `R` lands) is ours.
|
||
- **`TaskContract.LOCAL_ACCOUNT_TYPE`** stays `"org.dmfs.account.LOCAL"`. It is a
|
||
value stored in the database and recognised by dmfs-contract providers
|
||
generally, so the *same* app code has to write it whether it is talking to our
|
||
provider or, in External mode, to OpenTasks. Renaming it would fork the write
|
||
path in two for no gain.
|
||
- **`ACTION_BROADCAST_TASK_DUE` / `…_TASK_STARTING` / `ACTION_DATABASE_INITIALIZED`.**
|
||
Every send site calls `setPackage()` on its own package first, so these never
|
||
cross app boundaries and cannot collide with an installed OpenTasks.
|
||
|
||
## Changes to upstream source
|
||
|
||
Four files under `src/main/java`, one under `src/test/java`. Each edit is marked
|
||
with an `AGENDULA CHANGE` comment at the site, so this list and the code cannot
|
||
drift apart. Keep that convention.
|
||
|
||
### Behavioural
|
||
|
||
1. **`Utils.cleanUpLists` — only prune account types we authenticate ourselves.**
|
||
*The one change here that is about correctness rather than mechanics.*
|
||
|
||
Upstream holds `GET_ACCOUNTS` and enumerates every account on the device. We
|
||
dropped that permission (below), so `AccountManager` only ever reports
|
||
accounts of our own type. Upstream's cleanup deletes any task list whose
|
||
account is absent from that array — and an account we *cannot see* is
|
||
indistinguishable from one that has been *removed*. Left alone, the provider
|
||
would quietly delete synced lists. Not "sync stops": data disappears, with no
|
||
error anywhere.
|
||
|
||
Now a list is only prunable when its account type belongs to an authenticator
|
||
in **this package**. Agendula ships no authenticator yet, so the set is empty
|
||
and nothing is ever pruned; our sync adapter's type will join it on its own
|
||
when it lands, no edit needed here. Local lists were already exempt upstream.
|
||
|
||
2. **`TaskProvider.insert` — the same restriction for the stale-list signal.**
|
||
Upstream flags "list with unknown account" and broadcasts about it. With the
|
||
account cache holding only our own accounts, that fired on every insert into
|
||
any externally-synced list. Nothing listens today; the point is that whatever
|
||
listens tomorrow gets a signal that means something.
|
||
|
||
3. **`TaskProviderBroadcastReceiver.onReceive` — fall-through written out.**
|
||
Upstream's `switch` has no `break` in any branch, so `TIMEZONE_CHANGED` runs
|
||
all three content operations and `NOTIFICATION_ALARM` runs the last two. The
|
||
comment on the first branch ("don't trigger the notifications update yet")
|
||
describes breaks that were never written, so the code and the stated intent
|
||
contradict each other.
|
||
|
||
**Observed behaviour is preserved exactly**, just spelled out with `if`s
|
||
rather than reached by accident. A vendored fork is the wrong place to guess
|
||
at intent. ⚠️ **Open:** which of the two is the bug wants a device with a task
|
||
due across a timezone change to settle.
|
||
|
||
4. **`TaskProviderBroadcastReceiver.planNotificationUpdate` — inexact-alarm fallback.**
|
||
`setExact` throws `SecurityException` on API 31–32 when the user revokes
|
||
`SCHEDULE_EXACT_ALARM` — inside a receiver handling a system broadcast, so the
|
||
app would die on every timezone change. Falls back to `set` when exact alarms
|
||
aren't permitted. This alarm drives only the provider's own bookkeeping;
|
||
Agendula's user-visible reminders come from `ReminderScheduler`, which asks
|
||
for the permission properly.
|
||
|
||
### Required by modern Android (would not build or would crash otherwise)
|
||
|
||
5. **`PendingIntent.FLAG_IMMUTABLE`** added in `planNotificationUpdate`. Mandatory
|
||
since Android 12; throws `IllegalArgumentException` without it at targetSdk ≥ 31.
|
||
Upstream targets 29. Nothing mutates the intent later, so immutable is also
|
||
correct on the merits.
|
||
6. **`android:exported`** stated explicitly on the receiver. AGP hard-errors on an
|
||
intent-filtered component without it at targetSdk ≥ 31.
|
||
7. **`package=` attribute** removed from the manifest; AGP 8+ takes it from the
|
||
`namespace` in the build file.
|
||
8. **`<uses-sdk tools:overrideLibrary="org.dmfs.android.contenttestpal"/>`** removed
|
||
along with the androidTest sources it existed for (below).
|
||
|
||
### Permissions
|
||
|
||
9. **`android.permission.GET_ACCOUNTS` dropped.** We only ever need to see
|
||
accounts of our own type, and since API 26 an authenticator makes those
|
||
visible to its own package with no grant at all. Safe **only** in combination
|
||
with change 1 — the two must be read together.
|
||
|
||
### Build and test
|
||
|
||
10. **`build.gradle` → `build.gradle.kts`**, on the root version catalog. minSdk
|
||
21 → 29 (matching `:app`; the merger rejects a lower floor), Java 8 → 17.
|
||
11. **Test stack modernised**, sources otherwise untouched: JUnit 4.12 → 4.13.2,
|
||
Robolectric 3.5.1 → 4.16, Mockito 2.27 → 5.20, Hamcrest 1.3 → 3.0.
|
||
`org.dmfs:jems`, `rfc5545-datetime` and `lib-recur` stay on the versions
|
||
upstream pinned — all three resolve from Maven Central, so no new repository
|
||
was added (`settings.gradle.kts` is still `google()` + `mavenCentral()` under
|
||
`FAIL_ON_PROJECT_REPOS`).
|
||
12. **`ZippedTest.testAbsent`** — diamond `new Zipped<>` given an explicit type
|
||
argument. `absent()` pins no type, and javac 17 will not infer what javac 8
|
||
did. The assertion is unchanged.
|
||
13. **`src/test/resources/robolectric.properties` added** (`sdk=34`,
|
||
`conscryptMode=OFF`). A library module has no `targetSdk` for Robolectric to
|
||
read, and Robolectric installs Conscrypt unconditionally, whose uber jar has
|
||
no `linux-aarch_64` native — so without this the suite fails at setup on ARM64
|
||
machines while passing on x86_64 CI. See the file for the reasoning.
|
||
14. **`src/androidTest` dropped entirely.** It depends on `contentpal` /
|
||
`contenttestpal`, which are JitPack-only; adding JitPack would widen the
|
||
dependency trust surface for test-only code. ⚠️ This is the one place
|
||
vendoring lost coverage — those were the provider's *integration* tests
|
||
(recurrence, reparenting, instances, observers). The 56 JVM tests in
|
||
`src/test` all pass and are retained.
|
||
15. **`agendula_provider_changed_receivers` emptied.** Upstream notifies
|
||
`org.andstatus.todoagenda`, which listens for changes to the *dmfs* authority
|
||
and has never heard of ours. Anything re-added here also needs a `<queries>`
|
||
entry in the app manifest or package-visibility rules drop the broadcast.
|
||
16. **Translated `agendula_provider_label` overrides removed** (the other
|
||
translated strings are kept as upstream shipped them). The base label became
|
||
"Agendula tasks" so it is distinguishable from OpenTasks' own "Tasks" entry in
|
||
the system permission dialog; the inherited translations still said plain
|
||
"Tasks" in their language, which would have contradicted it.
|
||
|
||
## Resyncing from upstream
|
||
|
||
Unlikely to ever be worth it — upstream 1.4.2 is from 2021 — but if it is: the
|
||
`AGENDULA CHANGE` markers are the complete list of what to reapply, `git log` on
|
||
this directory is the audit trail, and the 56 JVM tests are the safety net.
|
||
Re-read change 1 before touching anything account-related.
|
||
|
||
## Known-unverified
|
||
|
||
Everything here is verified by the JVM test suite and a clean build. What is
|
||
**not** yet verified on a device with real data:
|
||
|
||
- The local-list path with **no account present at all** — the entirety of Local
|
||
mode. `cleanUpLists` exempts local lists explicitly and change 1 makes the
|
||
prunable set empty, so it should hold by construction; it is covered by
|
||
`ProviderAccountCleanupTest`, but that is Robolectric, not a device.
|
||
- The timezone-change behaviour in change 3.
|
||
- Any interaction with an external sync engine writing into our authority
|
||
(nothing does yet — that is the DAVx5 ask, step 4 of the sequencing).
|