Implements Backup Improvements for 3.4 #776

mattschlosser · 2020-07-11T02:23:47Z

Users on all Android versions are now asked where they want to save the file when picking the phone storage option from the backup system
When backing up modules, users now have an option to choose phone storage
When a backup is completed, the location no longer shows
Removed unnecessary WRITE_EXTERNAL_STORAGE permissions since it is no longer being used to directly write to external storage.

Closes #756

Describe the pull request content
A clear and concise description of what the pull request is about.
What are the benefits of this new code addition?
Possible negative side effects?

Screenshots
If applicable, add screenshots to help explain your pull request.

tuomas2 · 2020-07-11T08:26:24Z

After merging other PRs, there are now little merge conflicts

mattschlosser · 2020-07-11T16:26:08Z

Should be good to go now. Also added a commit to remove some of the now unused code...

JJK96

I've reviewed the PR.

It no longer gives an error when trying to save on Android 10, so that's a big improvement already!
I tested saving and restoring the application database and modules to/from phone storage which all worked fine on Android 10.

I ran into one problem, backup to phone storage does not overwrite an existing file.
So I end up with andBibleDatabase.db, andBibleDatabase.db (1) and andBibleDatabase.db (2).
That issue has to be resolved before merging, IMO.

app/src/main/java/net/bible/android/control/backup/BackupControl.kt

JJK96 · 2020-07-13T11:45:41Z

app/src/main/java/net/bible/android/view/activity/page/MenuCommandHandler.kt

@@ -220,20 +218,12 @@ constructor(private val callingActivity: MainBibleActivity,
                        .setTitle(callingActivity.getString(R.string.backup_backup_title))
                        .setMessage(callingActivity.getString(R.string.backup_backup_message))
                        .setNegativeButton(callingActivity.getString(R.string.backup_phone_storage)) { dialog, which ->
-                            if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) {


Since this is removed, I think this needs testing on those android versions.

It would indeed be good to roughly go through different android versions with emulator (from lollipop (5) to the latest models).

JJK96 · 2020-07-13T11:45:57Z

app/src/main/java/net/bible/android/view/activity/page/MenuCommandHandler.kt

@@ -259,29 +249,9 @@ constructor(private val callingActivity: MainBibleActivity,
                    requestCode = IntentHelper.UPDATE_SUGGESTED_DOCUMENTS_ON_FINISH
                }
                R.id.restore_app_database -> {
-                    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.Q) {


Since this is removed, I think this needs testing on those android versions.

mattschlosser · 2020-07-14T13:49:32Z

I've reviewed the PR.

It no longer gives an error when trying to save on Android 10, so that's a big improvement already!
I tested saving and restoring the application database and modules to/from phone storage which all worked fine on Android 10.

I ran into one problem, backup to phone storage does not overwrite an existing file.
So I end up with andBibleDatabase.db, andBibleDatabase.db (1) and andBibleDatabase.db (2).
That issue has to be resolved before merging, IMO.

Regarding the multiple file names, this is an android file level problem as the intent automatically creates a new file if one with the same name already exists.

Possible options are to show another screen when choosing backup when user clicks phone storage as described below

Current:

When a user selects the phone storage option:

show file picker and write to file

Possible Change:

When a user selects the phone storage option

if it is the user's first time backing up to phone storage
- ask them where to create a new file
- write backup to file
- call ContentResolver#takePersistableUriPermission(Uri, int) to keep write permissions to file across phone reboots
- possibly need to save URI somewhere or figure out how to use ContentResolver#getPersistedUriPermissions()
else the user has created a backup before
- ask them if they want to overwrite the old backup file or create a new one?
  - overwrite?
    - retrieve old backup Uri using getPersistedUriPermissions() and figure out which one is the correct type of backup?
    - write backup to Uri
    - maybe a menu so a user can select which backup to overwrite if user has created multiple different backups before
  - or create a new file
    - user is prompted to create a new file through the android UI file picker
    - write backup to file
    - takePersistableUriPermission...

JJK96 · 2020-07-14T19:31:09Z

I like the simplicity of your current solution. I think this is a problem that should be solved by Android indeed.

So, considering that the issue is not easily resolved at the moment, and the current status is an improvement with respect to the previous version, this PR can be merged IMO (After some testing on older android versions).

mattschlosser · 2020-07-15T13:56:17Z

was doing some testing on older versions and i discovered this error in general..
When a user backs up the database after opening the app, no "success" message is shown.
If a user immediately backs up again, the "success" message works as normal.

E/Dialogs: Error showing error message.  Original error msg:My Notes, Bookmarks, Reading Plans and Workspaces backed up successfully.
    java.lang.RuntimeException: Can't create handler inside thread that has not called Looper.prepare()
        at android.os.Handler.<init>(Handler.java:200)
        at android.os.Handler.<init>(Handler.java:114)
        at android.widget.Toast$TN.<init>(Toast.java:344)
        at android.widget.Toast.<init>(Toast.java:100)
        at android.widget.Toast.makeText(Toast.java:258)
        at net.bible.android.view.activity.base.Dialogs.showMsg(Dialogs.kt:119)
        at net.bible.android.view.activity.base.Dialogs.showErrorMsg(Dialogs.kt:87)
        at net.bible.android.view.activity.base.Dialogs.showErrorMsg$default(Dialogs.kt:86)
        at net.bible.android.view.activity.base.Dialogs.showMsg(Dialogs.kt:51)
        at net.bible.android.control.backup.BackupControl.backupDatabaseToUri(BackupControl.kt:96)
        at net.bible.android.view.activity.page.MainBibleActivity$onActivityResult$2.invokeSuspend(MainBibleActivity.kt:1158)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:56)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:571)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:738)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:678)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:665)

Does anyone know how to fix?

timbze · 2020-07-15T14:24:59Z

Does anyone know how to fix?

I great to see this stuff being tested thoroughly. :) This issue is because the dialog is trying to start on a background thread, but it must be started on the main thread.

In MainBibleActivity.kt, L1133+ (master branch)

GlobalScope.launch(Dispatchers.IO) {
  val inputStream = contentResolver.openInputStream(data!!.data!!)
  if (backupControl.restoreDatabaseViaIntent(inputStream!!)) {
    windowControl.windowSync.setResyncRequired()
    withContext(Dispatchers.Main) {
      documentViewManager.clearBibleViewFactory()
      currentWorkspaceId = 0
    }
  }
}

restoreDatabaseViaIntent is being started under Dispatchers.IO, and that is where the error occurs. It either needs to start in Dispatchers.Main, or the Dialogs.instance.showMsg within restoreDatabaseViaIntent needs to be within another GlobalScope.launch(Dispatchers.Main)

timbze · 2020-07-17T15:11:03Z

@mattschlosser can you either merge or rebase develop into your PR so that CI can pass? There was a CI issue that has been fixed.

Users on all Android versions are now asked where they want to save the file when picking the phone storage option from the backup system When backing up modules, users now have an option to choose phone storage WHen a backup is completed, the location no longer shows Removed unnecessary WRITE_EXTERNAL_STORAGE permissions since it is no longer being used to directly write to external storage. Closes AndBible#756

JJK96 · 2020-08-12T11:33:46Z

I just tested the new version, it works fine for me.
In the previous version I could no longer choose a filename for the backup to phone storage, but that is fixed in this version.

tuomas2 · 2020-09-03T11:32:43Z

app/src/main/res/values/strings.xml

@@ -481,7 +481,8 @@
    <string name="restore_confirmation">Overwrite My Notes, Bookmarks, Reading Plans and Workspaces?</string>
    <string name="restore_success">My Notes, Bookmarks, Reading Plans and Workspaces restored successfully</string>
    <string name="loading_backup">Loading database from backup…</string>
-    <string name="backup_success">My Notes, Bookmarks, Reading Plans and Workspaces backed up successfully to %s.</string>
+    <string name="backup_success">My Notes, Bookmarks, Reading Plans and Workspaces backed up successfully.</string>


Because of translations, we should not make such changes to strings that change parameters. Otherwise users with translations that have not been updated will have crashes.

When parameters need to be changed, like here, we need to introduce a new string (with new key, for example backup_success2), use it, and remove old string.

tuomas2 · 2020-09-03T11:38:18Z

Looks okay. I will do the fix related to my own comment in develop branch. Let's continue testing there.

mattschlosser force-pushed the feature/#756_android_backup branch from d6e8e18 to 38bd08c Compare July 11, 2020 14:17

tuomas2 requested a review from a team July 11, 2020 19:27

JJK96 requested changes Jul 13, 2020

View reviewed changes

tuomas2 changed the base branch from master to develop July 15, 2020 14:05

mattschlosser added 3 commits July 17, 2020 19:02

Remove unneeded backup code

df7bcbc

Show success/error messages on main thread

4fbc76e

mattschlosser force-pushed the feature/#756_android_backup branch from a58c9ad to 4fbc76e Compare July 18, 2020 01:03

tuomas2 requested a review from JJK96 August 12, 2020 08:24

tuomas2 reviewed Sep 3, 2020

View reviewed changes

tuomas2 merged commit 53481f2 into AndBible:develop Sep 3, 2020

timbze mentioned this pull request Nov 24, 2020

Fix app crash from backup module to file return without selecting file #906

Merged

timbze mentioned this pull request Sep 3, 2021

Make release notes for 4.0 #1648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements Backup Improvements for 3.4 #776

Implements Backup Improvements for 3.4 #776

mattschlosser commented Jul 11, 2020 •

edited

Loading

tuomas2 commented Jul 11, 2020

mattschlosser commented Jul 11, 2020

JJK96 left a comment

JJK96 Jul 13, 2020

tuomas2 Jul 13, 2020

JJK96 Jul 13, 2020

mattschlosser commented Jul 14, 2020 •

edited

Loading

JJK96 commented Jul 14, 2020 •

edited

Loading

mattschlosser commented Jul 15, 2020

timbze commented Jul 15, 2020

timbze commented Jul 17, 2020

JJK96 commented Aug 12, 2020

tuomas2 Sep 3, 2020

tuomas2 commented Sep 3, 2020

Implements Backup Improvements for 3.4 #776

Implements Backup Improvements for 3.4 #776

Conversation

mattschlosser commented Jul 11, 2020 • edited Loading

tuomas2 commented Jul 11, 2020

mattschlosser commented Jul 11, 2020

JJK96 left a comment

Choose a reason for hiding this comment

JJK96 Jul 13, 2020

Choose a reason for hiding this comment

tuomas2 Jul 13, 2020

Choose a reason for hiding this comment

JJK96 Jul 13, 2020

Choose a reason for hiding this comment

mattschlosser commented Jul 14, 2020 • edited Loading

Current:

Possible Change:

JJK96 commented Jul 14, 2020 • edited Loading

mattschlosser commented Jul 15, 2020

timbze commented Jul 15, 2020

timbze commented Jul 17, 2020

JJK96 commented Aug 12, 2020

tuomas2 Sep 3, 2020

Choose a reason for hiding this comment

tuomas2 commented Sep 3, 2020

mattschlosser commented Jul 11, 2020 •

edited

Loading

mattschlosser commented Jul 14, 2020 •

edited

Loading

JJK96 commented Jul 14, 2020 •

edited

Loading