-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: Scoped Storage Migration Service, Notification & Deck Picker Implementation #13152
Conversation
TaskDelegate was used only as a generic 'start/progress/complete + result' interface, which isn't necessary and only increases coupling and complexity
Message to maintainers, this PR contains strings changes.
Read more about updating strings on the wiki, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the overall process:
- My AnkiDroid directory still exists after the migration with the "essential files". Shouldn't they be deleted as well?
- I can't see the progress bar here, and the progress report string shows
0.00MB
for the total. I think that the progress bar is enough, so we may not need theMigrated %s of %s MB
string, as there won't be much time to translate it on most languages- Tested on my phone, Samsung Galaxy Note 10 Lite, Android 13, One UI 5
- https://user-images.githubusercontent.com/69634269/214580402-3f2c1aed-f680-436f-aee6-d6abb752c27a.mp4
AnkiDroid/src/main/java/com/ichi2/anki/services/MigrationService.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/services/MigrationService.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/services/ServiceConnection.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/services/ServiceConnection.kt
Outdated
Show resolved
Hide resolved
} | ||
|
||
/** | ||
* Show a dialog that explains no sync can occur during migration. | ||
*/ | ||
private fun warnNoSyncDuringMigration() { | ||
// TODO: Fetch and display real numbers | ||
// TODO: handle value updates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest handling it by using a progress dialog (not for this PR, just an idea for the TODO task)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I was waiting for it, I have that part ready at my end
AnkiDroid/src/main/java/com/ichi2/anki/services/ServiceConnection.kt
Outdated
Show resolved
Hide resolved
Oh, and why have you removed the |
@BrayanDSO I am the one working with the progress bar that is to be shown during migration and the UI element, Basically STOR-04 and STOR-05 so will pull the updates and then push the PR after this PR gets merged |
Forgot to reply about your points:
I think it is stable enough to alpha testers, and they can always refure to migrate or keep using an older version
Seems correct to me.
|
0ea5a24
to
3711434
Compare
3711434
to
15b7bae
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Left an implementer's choice comment
A service which migrates the AnkiDroid collection from a legacy directory to an app-specific directory. Contains a persistent notification and is designed to be run as a foreground service Issue 5304
* Tapping the Migration icon on the Deck Picker offers to migrate storage * Implements binding to MigrationService to allow: * Refresh when migration is completed * Update of progress, for an in-app dialog
This hasn't worked for directories on any of my devices: ``` Recovering failed rename /storage/emulated/0/AnkiDroid/collection.media to /storage/emulated/0/Android/data/com.ichi2.anki/files/AnkiDroid20/collection.media Rename recovery failed android.system.ErrnoException: rename failed: EXDEV (Cross-device link) at libcore.io.Linux.rename(Native Method) ... ``` We previously assumed this would fail for a directory first. So did not set `attemptRename` to false for a file copy. Now we no longer handle directories, so set this value on a file copy.
Added in 0b869c7 - doesn't seem intentional
15b7bae
to
1c59e65
Compare
Can someone please explain the expected testing flow? I did this: prep1) Now I try to test: test0) make sure you have the github command line utility installed, because it's useful, it is called If I sync etc, the collection is already scoped storage so I'm not offered any migration. 🤔 |
TL;DR:
A new collection is now initialised at An existing user would have a collection at note Since changes to We have not yet moved to API 30, so we can manually set the collection location to a legacy location ( |
Hmm - what I thought was strange was that I had a collection there in /sdcard/AnkiDroid - why didn't the app (.debug suffix or not...) see it and use it? Even if the migration was optional this seems like the prime testing flow doesn't it? (start with 2.15.6 synced with ankiweb, install this, check it? It was the most "happy path" thing I could think of...) I'll try with a manual storage location setting as new step test4 and current step test4 will become test5 Thanks! |
Okay that worked, but here's another testing note, that is not fixable really: Don't do this on a Play Services emulator or you can't use Anyway, it seemed to work fine. A couple UX notes: 1- The migrate button on deck picker stayed there post-migration until deck picker was made to UI refresh (app background + foreground did it for me) But it worked it seems! More testing in a bit on a non-play services emulator and with a large collection, and I'll read through the code etc as well. |
New installs default to the in-app directory (e.g. storage/0/emulated/com.ichi2.anki.debug/files). If you want to make new installs default to the legacy storage, add |
The snackbar shouldn't be shown if the essential files migration is too fast (800ms). Apparently this PR breaks that. Good catch
|
Yes waiting for this to be merged then will implement and push |
Great - thanks and sorry if I'm duplicating already known stuff. I'm attempting to help with review + testing on this one but I'm catching up on progress and have obviously missed huge amounts of work + tracking, so I'm sadly duplicating some comments. Hopefully it's still a net positive :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good on a first and second scan, and it didn't crash or anything when I ran it locally - I'd just be slowing it down further by making it wait more. Let's go! Also, of course: this is awesome as a chunk of progress, really excited
@criticalAY it seemed like you had some work queued up behind this? I'm going to do a string sync now but hopefully you're unblocked 🚂 |
Yep! Thankyou I'll go ahead as soon as it gets merged |
* See: #5304 | ||
* @return true: Interrupt startup. `false`: continue as normal | ||
*/ | ||
open fun startingStorageMigrationInterruptsStartup(): Boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this function is super hard to read and does not sufficiently reflect what it does. The documentation is not very helpful. I suggest to put into the documentation a high-level overview of the issue it solves.
You could perhaps slightly improve it by thinking of more simple names, e.g.
enum class ShouldContinueActivityStartup { Yes, No }
fun migrateStorageIfNeeded(): ActivityStartup { ... }
fun handleStartup() { // bad name, but w/e
if (migrateStorageIfNeeded() == ShouldContinueActivityStartup.No) return
...
}
But really, the main problem is that this function, being itself very high-level, does very much high-level actions. Consider removing it altogether:
fun handleStartup() {
val migrationStatus = ScopedStorageService.migrationStatus(this)
when (migrationStatus) {
Status.NEEDS_MIGRATION -> TODO("Propose migration")
Status.IN_PROGRESS -> startMigrateUserDataService()
Status.REQUIRES_PERMISSION -> requestStoragePermission()
}
val shouldHaltStartup = migrationStatus == Status.REQUIRES_PERMISSION
if (shouldHaltStartup) return
...
}
@@ -771,7 +831,7 @@ open class DeckPicker : | |||
} | |||
R.id.action_scoped_storage_migrate -> { | |||
Timber.i("DeckPicker:: migrate button pressed") | |||
showDialogThatOffersToMigrateStorage() | |||
showDialogThatOffersToMigrateStorage(onPostpone = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not very important, but I'm not sure if onPostpone
should have been a part of this PR
@@ -936,6 +996,18 @@ open class DeckPicker : | |||
mFloatingActionMenu.isFABOpen = savedInstanceState.getBoolean("mIsFABOpen") | |||
} | |||
|
|||
fun onStorageMigrationCompleted() { | |||
migrationService.unbind(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The service should stop itself after finishing the migration. I am assuming that we don't have to unbind from it here, or at all; if we do, it should probably happen not in this method. If this needs to be here, I'd like a comment regarding the necessity of this line.
@@ -936,6 +996,18 @@ open class DeckPicker : | |||
mFloatingActionMenu.isFABOpen = savedInstanceState.getBoolean("mIsFABOpen") | |||
} | |||
|
|||
fun onStorageMigrationCompleted() { | |||
migrationService.unbind(this) | |||
invalidateOptionsMenu() // reapply the sync icon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“reapply the sync icon” is hard to understand
|
||
override fun onStart() { | ||
super.onStart() | ||
if (userMigrationIsInProgress(this)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that we should use either this, or ScopedStorageService.migrationStatus()
, throughout.
* | ||
* @param T The service to encapsulate. Note: service's [Service.onBind] must return a [SimpleBinder] | ||
*/ | ||
abstract class ServiceConnection<T : Service> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'm not entirely sure about this.
When I had to use a 1 activity + 1 service combination, I finally settled on a simplified approach of putting the service into a static variable (Java times). No binders, no boilerplate. Coupled with an event bus (actually EventBus
) approach (now obsolete but yeah), it was, I think, an approach that should be still valid today with some reasonable modifications.
To be fair, on of my gripes was the async nature of onServiceConnected
IIRC. I wanted that to be sync. So I chose other options that allowed me to do more seamless animations and stuff. This might be irrelevant for AnkiDroid and my memories might be obsolete, so this is no more than food for thought
The bottom line here is, I think this entire class might be an overcomplication. I see that it's generic and is supposed to simplify things, but can't we get away with just not using a connection, or using one that merely forwards to the service object itself?
return file.length() | ||
} else if (!file.exists()) { | ||
return 0L | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be doing the same thing that CoroutineScope.calculateSize(file: File)
does. Can we use that?
@@ -84,3 +84,11 @@ object HandlerUtils { | |||
) | |||
} | |||
} | |||
|
|||
fun runOnUiThread(runnable: () -> Unit) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest consolidating this with stuff like executeFunctionUsingHandler
, executeFunctionWithDelay
, etc
* @param delayMs the interval to wait after the execution of [runnable] before the next call is made | ||
* @param runnable Called once every [delayMs] until [terminate] is called | ||
*/ | ||
class Repeater private constructor(val delayMs: Long, val runnable: () -> Unit) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest replacing this class with a coroutine, or removing the logic entirely in favor of StateFlow
and friends
Hi there @david-allison! This is the OpenCollective Notice for PRs merged from 2023-02-01 through 2023-02-28 If you are interested in compensation for this work, the process with details is here: https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month Please note that GSoC contributions are okay for this process. Our philosophy is that our users have donated to AnkiDroid for all contributions. The only PRs that will not go through the OpenCollective process are ones directly related to am accepted GSoC project from a selected participant, since those receive a stipend from GSoC itself. Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding. Thanks! |
Pull Request template
Purpose / Description
This Pull Request implements the foreground service which performs the Scoped Storage Migration, and hooks it up to the Deck Picker
Review Goals
onStartCommand
isStarted logicThis also contains a couple refactorings which could be split out if requested in the review.
Fixes
Related #5304
Approach
How Has This Been Tested?
Known Issues
Reviewer
is not tied in to the migration process yetMedia
is not blockedChecklist