Skip to content

Library backup/restore on the Compose modernisation stack - #655

Closed
PylotLight wants to merge 1 commit into
timusus:mainfrom
PylotLight:pr/library-backup
Closed

PylotLight wants to merge 1 commit into
timusus:mainfrom
PylotLight:pr/library-backup

Conversation

@PylotLight

@PylotLight PylotLight commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

split this one up like you asked - it's backup-only now, rebased on main.

settings -> library gets back up / restore library backup, writes a json file with play counts, positions, favourites, exclusions, dates and playlists. restore matches things up (exact path, then relative path so sd card volume changes don't break it, then remote ids, then tag fingerprint as a last resort) and writes the backup's snapshot back. no room ids, tokens or grant uris in the file.

one thing worth knowing: settingsviewmodel moved to :android:presentation since i started, so the android side of it sits behind a small LibraryBackupFlow interface. tests use a fake.

also in here: predictive back + locale config. left out the k8s builder (moved it out of the repo, it's just my local build setup now), jdk bump, wrapper bump, minSdk change, scanner and theme stuff.

scanner will be its own PR next. build + settings tests all green.

@timusus

timusus commented Oct 1, 2026

Copy link
Copy Markdown
Owner

Thanks for this, the backup/restore work is wanted. As it stands the PR can't land: it conflicts with main in 7 files (MediaProviderInitializer, LibraryScreen, strings_sources.xml, SettingsIntegrationTest, SettingsViewModelTest, designsystem Settings.kt, SettingsViewModel), and it bundles infrastructure changes that need separate decisions from the feature.

Could you split it?

  1. Library backup/restore only, rebased on current main: the settings/backup/* classes, settings entries, strings and LibraryBackupMatcherTest. That one can go through review straight away.
  2. Incremental TagLib rescan + observer/broadcast trigger as its own PR, since it changes scanning behaviour for every local-library user and needs its own verification.
  3. Drop from both: the k8s/ remote-build cluster (your build setup, not something the repo should carry), the JDK 21 toolchain and Gradle wrapper bump, and the minSdk 24 to 26 change. Min SDK 24 is deliberate; if you think there's a case for 26, open an issue and we can discuss it there.

Predictive back and the locale config are small enough to ride along with (1) if they rebase cleanly.

Settings -> Library gains Back up library / Restore library backup.
Export writes play counts, positions, favourites, exclusions,
dates and playlists (identity: path, remote id, tag fingerprint;
never Room ids, tokens or grant URIs). Restore matches exact,
relative-path, remote-id then fuzzy, and writes the backup's
snapshot back; anything unmatched is reported in the done snackbar.
Predictive back + per-app locale config ride along.

Includes matcher/schema unit tests.
@PylotLight

Copy link
Copy Markdown
Contributor Author

split it up like you asked - this one's backup-only and rebased on main now. scanner will be a separate PR. dropped the k8s dir (moved it out of the repo entirely), jdk/wrapper bumps and the minSdk change. will open an issue for the minSdk discussion.

@timusus

timusus commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

🤖 Claude here (an AI agent), reviewing for Tim.

Thanks for splitting this out, it's much easier to review now. It rebases cleanly, but a review turned up some issues to fix before it can land:

Matching

  1. LibraryBackupMatcher: DURATION_TOLERANCE_S = 2 is compared against Song.duration, which is in milliseconds. The fuzzy tolerance is really 2 ms (MediaStoreMediaProvider uses <= 1000 for one second). The test's 200 vs 201 values hide this.
  2. Export writes the artist as albumArtist ?: artists.joinToString(", "), but the library side keys on friendlyArtistName. Any song with an album artist, or with several artists, can never fuzzy-match. The test helper builds identities the same way, so it masks the problem.
  3. The fuzzy match takes the first candidate and isn't scoped by provider. Untagged files (blank title, album and artist) all share one key, and two backup entries can resolve to the same library song. Please scope the match by provider, skip it when the tags are empty, and consider confirming it with size, which is already in the identity.

Restore semantics
4. The restore overwrites playCount, lastPlayed, favourites and excluded with the backup's values. The summary string says "Merge… newer on-device activity is kept". Either make it a real merge (max of play counts, latest lastPlayed) or change the copy and add a confirmation dialog. Tim would prefer a merge.
5. It isn't atomic: there are one to three DAO writes per song with no @Transaction, so a failure part-way leaves a half-applied restore, and each write invalidates the Room flows. Please batch the writes in one transaction.
6. Favourites restored for remote providers skip pending_favourites, so they never reach the server. Either route them through the normal favourite path or leave remote favourites out of the restore.

Playlists
7. Operator precedence in LibraryBackupManager (~L159): if (externalId != null) … else null ?: nameMatch binds the elvis to the else branch only. When an externalId is present but finds no playlist, the name fallback never runs and repeated restores create duplicates.
8. Newly created playlists don't get sortOrder or sortDescending restored; only existing ones do.
9. clearPlaylist followed by addToPlaylist on an existing playlist drops unmatched members and anything added on the device, and an interruption between the two calls leaves the playlist empty. Please make it additive (append missing members) or do it in one transaction.

Robustness
10. readBytes() on a user-picked file has no size cap, and an OutOfMemoryError isn't caught by catch (e: Exception). Please cap the size. Also, the ViewModel's runCatching swallows CancellationException.
11. Values such as negative play counts are written without validation. Clamp them on import.

Scope
12. locales_config.xml lists only en, but the app ships 17 translations. On Android 13+ the per-app language picker would then offer English only. Please list the real locales or drop it.
13. enableOnBackInvokedCallback is probably fine, since back handling is all BackHandler/PredictiveBackHandler, but it changes back animations app-wide. A separate small PR would be better, so it can be tested on a device on its own.

Tests
14. Please add precedence tests (exact path, then relative path, then external id, then fuzzy), plus a collision test and a cross-provider test. Tests for the restore and playlist logic in the manager would also help; that class currently has none.

Items 1–3 and 7–9 are the blockers; the rest can follow in the same PR. I'll take another look once it's updated.

@timusus

timusus commented Oct 3, 2026 •

Copy link
Copy Markdown
Owner

🤖 Claude here (an AI agent), reviewing for Tim.

Update: Tim would like this finished on his side instead of sending you back around the loop, so I'm doing that. Your commit goes in with you as author, and my fixes for the review points above follow as separate commits, co-authored with you. Restore will merge rather than overwrite: higher play counts and later dates win, and favourites and exclusions are combined. The locale config and predictive back are left out for now; either is welcome as its own small PR, and the locale config would need to list all the shipped translations.

Once it's been tested on a device and has had a UI pass, it'll land on main and this PR will be closed with a link to the commits. Thanks for the feature.

@timusus

timusus commented Oct 3, 2026

Copy link
Copy Markdown
Owner

🤖 Claude here, reviewing for Tim.

Library backup and restore is now on main (52878c8..71bb1b3). Your commit (52878c8) went in with you as the author. My follow-up commits credit you as co-author.

What changed on top of your commit:

  • Restore merges instead of overwriting. Play counts and last played take the higher value, and favourites and exclusions are combined. Playlists are appended to without duplicates. This all happens in one database transaction, so restoring the same file twice changes nothing.
  • Size cap on imports. Imports are read through a size-capped stream, and failures are logged.
  • Settings. Backup has its own section, and the summaries are shorter. The result message gives counts ("Restore complete: N songs matched, M playlists added or updated"), and a different message appears when nothing in the file matches the library.
  • Tests. Unit tests, Room tests and a Maestro device check cover backup, wipe, restore and restoring a second time.
  • Dropped. The locales_config and predictive-back changes were dropped as out of scope for this PR. Tim is happy to look at them in a separate PR.

Thanks for the feature, @PylotLight. Closing, since it landed through those commits rather than through a merge.

@timusus timusus closed this Oct 3, 2026
@PylotLight

PylotLight commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor Author

yeah totally fine with that approach, thanks for picking it up and finishing it off, really appreciate it.

happy with the merge-instead-of-overwrite call, and the transaction plus size-cap handling all sounds sensible. also fine with the commit split as you described it, authorship/co-authorship works for me.

only question from my side: are we likely to get a release with this in soon? last android tag looks like 1.0.8 so this is main-only at the moment. happy to help verify or with notes if that helps get one out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants