use `SaveInstanceState` to avoid crash in export file when Activity is recreated #14797

criticalAY · 2023-11-22T19:05:53Z

Purpose / Description

Remove the lateinit var and use shared pref and nullable string to avoid crash in case the developer's option is turned on

Fixes

Fixes #14795

How Has This Been Tested?

google emulator
Screen_recording_20231124_152918.webm

Links to blog posts, patterns, libraries or addons used to solve this problem

Checklist

Please, go through these checks before submitting the PR.

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

criticalAY · 2023-11-22T19:08:26Z

I came across this when I was reproducing #14795, and regarding this issue we can use shared prefs to overcome it what say?
I can update this PR only if the reviewer agrees to this updating the message and commits do let me know

lukstbit · 2023-11-24T09:35:41Z

Is this really needed? I've tested the collection/apkg export and the SnackBar appears where it should, above the FAB without the changes from this PR.

Edit, forgot: I'm working on #14604 and I might be able to delete ActivityExportingDelegate, so you shouldn't spend time on #14795 .

criticalAY · 2023-11-24T09:52:43Z

This was in case the "dont keep activity" was turned on by the user but after fixing the crash I noticed it still persist I am updating the PR to fix the crash first

criticalAY · 2023-11-24T10:02:45Z

Edit, forgot: I'm working on #14604 and I might be able to delete ActivityExportingDelegate, so you shouldn't spend time on #14795 .

ok, leaving it for now then 👍

github-actions · 2023-12-08T10:36:50Z

Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

criticalAY · 2023-12-08T10:38:14Z

@lukstbit any updates on the issue?

lukstbit

I've implemented the changes I wanted but it didn't touched the mExportFileName part of the code. So this PR is needed, I left some review notes on this.

AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt

lukstbit · 2023-12-09T08:08:58Z

AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt

@@ -277,7 +284,7 @@ class ActivityExportingDelegate(private val activity: AnkiActivity, private val
     * This will allow to check whether a recent export was made, hence scoped storage migration is safe.
     */
    private fun saveSuccessfulCollectionExportIfRelevant() {


Method should be changed to receive the path as a parameter.

this was not handled.

This method is called from two other methods, in one the path is available and could be passed directly and in the other method, saveFileCallback you could retrieve the filename from preferences and pass it to saveSuccessfillColl... and also to exportToProvider() which would allow us to avoid exportfilename?:"".

david-allison · 2023-12-11T00:35:37Z

Minor: we should remove the name from ACRA reports in the method: createAcraCoreConfigBuilder

AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt

mikehardy · 2023-12-11T14:06:22Z

AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt

@@ -277,7 +279,7 @@ class ActivityExportingDelegate(private val activity: AnkiActivity, private val
     * This will allow to check whether a recent export was made, hence scoped storage migration is safe.
     */
    private fun saveSuccessfulCollectionExportIfRelevant() {
-        if (::mExportFileName.isInitialized && !mExportFileName.endsWith(".colpkg")) return
+        if (!mExportFileName!!.endsWith(".colpkg")) return


we are in a new method here, and the activity may be recreated, but I don't see the filename fetched from prefs and initialized again?

If lukstbit had been handled instead of just resolved and closed without handling, then it would be a method parameter (non-null...) and you wouldn't need the dangerous !! on a var that I think is not initialized at this point

there is still a risk of it being null (it is 99.9% sure though it won't be null) as getString(EXPORT_FILE_NAME_KEY, "") automatically assigns null value to the pref in case it is null.

I thought it automatically assigned "" to the pref in case it is null.
This doesn't seem to actually address the substance of my comment tho?

i am pushing the changes for that part I have used elvis operator though in case

we can't escape the evil "!!" here if (!mExportFileName!!.endsWith(".colpkg")) return

david-allison · 2023-12-11T17:09:39Z

Is it feasible to use savedInstanceState here instead of preferences?

criticalAY · 2023-12-11T18:04:41Z

Is it feasible to use savedInstanceState here instead of preferences?

I guess that creates the need for extra calls from the activity where we are using this class, we can avoid that by using the prefs here itself

criticalAY · 2023-12-17T15:56:54Z

Using saved state now instead shared pref

david-allison

one issue regarding invalid states, rest looks great, thanks!

AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt

david-allison

Looks great, thanks!

mikehardy · 2023-12-18T18:52:53Z

At some point @criticalAY you're going to need to learn how to use git rebase and clean up your commits - this should be a one commit PR, I have no idea why there are 9 different commits most with same title

criticalAY · 2023-12-18T18:53:51Z

At some point @criticalAY you're going to need to learn how to use git rebase and clean up your commits - this should be a one commit PR, I have no idea why there are 9 different commits most with same title

Give me a moment, will do it

mikehardy · 2023-12-18T18:54:42Z

AnkiDroid/src/main/java/com/ichi2/anki/CrashReportService.kt

@@ -83,7 +83,7 @@ object CrashReportService {
    private fun createAcraCoreConfigBuilder(): CoreConfigurationBuilder {
        val builder = CoreConfigurationBuilder()
            .withBuildConfigClass(com.ichi2.anki.BuildConfig::class.java) // AnkiDroid BuildConfig - Acrarium#319
-            .withExcludeMatchingSharedPreferencesKeys("username", "hkey")
+            .withExcludeMatchingSharedPreferencesKeys("username", "hkey", "file_prefs")


Is this still a thing? Seems related to the previous design that was saved preferences based, but now you're using the bundle passed between previous-killed-activity and newly-resumed-activity ?

mikehardy

This looks clean + correct to me know and also "lighter" using the saved instance state style. It's changed a lot and with the rebase + push I'm going to ask for one more approval but I think it's smooth sailing. Thanks!

david-allison

Sorry, another round due to the use of "" as a path

AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt

david-allison · 2023-12-19T06:17:53Z

squash-merge The pull request currently requires maintainers to "Squash Merge"

criticalAY · 2023-12-19T06:23:21Z

there was a linting error causing the code quality failure so the latest commit fixes that nothing else

…s recreated (ankidroid#14797) * use saved instance state to avoid the crash if activity is destroyed * fixing test issues

github-actions · 2024-04-13T13:29:52Z

Hi there @criticalAY! This is the OpenCollective Notice for PRs merged from 2023-12-01 through 2023-12-31

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 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!

lukstbit added the Needs Author Reply Waiting for a reply from the original author label Nov 24, 2023

criticalAY changed the title ~~anchor the snackbar on fab when exporting col~~ use shared pref to avoid crash in case "don't keep activity" is turned on Nov 24, 2023

github-actions bot added the Stale label Dec 8, 2023

github-actions bot removed the Stale label Dec 8, 2023

lukstbit requested changes Dec 9, 2023

View reviewed changes

david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Dec 9, 2023

criticalAY requested a review from david-allison December 10, 2023 11:49

mikehardy changed the title ~~use shared pref to avoid crash in case "don't keep activity" is turned on~~ use shared pref to avoid crash in export file when Activity is recreated Dec 11, 2023

mikehardy reviewed Dec 11, 2023

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt Outdated Show resolved Hide resolved

mikehardy reviewed Dec 11, 2023

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt Outdated Show resolved Hide resolved

mikehardy reviewed Dec 11, 2023

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt Outdated Show resolved Hide resolved

mikehardy reviewed Dec 11, 2023

View reviewed changes

mikehardy added the Needs Author Reply Waiting for a reply from the original author label Dec 11, 2023

david-allison requested changes Dec 18, 2023

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt Outdated Show resolved Hide resolved

criticalAY requested review from david-allison and mikehardy December 18, 2023 01:59

david-allison approved these changes Dec 18, 2023

View reviewed changes

david-allison removed Needs Author Reply Waiting for a reply from the original author Needs Review labels Dec 18, 2023

mikehardy requested changes Dec 18, 2023

View reviewed changes

criticalAY force-pushed the export_deck_snackbar branch from 85cb5a7 to 18f1618 Compare December 18, 2023 19:10

criticalAY closed this Dec 18, 2023

criticalAY force-pushed the export_deck_snackbar branch from 18f1618 to 393f61b Compare December 18, 2023 19:17

criticalAY reopened this Dec 18, 2023

criticalAY requested a review from mikehardy December 18, 2023 19:33

mikehardy approved these changes Dec 18, 2023

View reviewed changes

mikehardy requested a review from david-allison December 18, 2023 19:39

david-allison requested changes Dec 19, 2023

View reviewed changes

criticalAY requested a review from david-allison December 19, 2023 05:47

criticalAY changed the title ~~use shared pref to avoid crash in export file when Activity is recreated~~ use SaveInstanceState to avoid crash in export file when Activity is recreated Dec 19, 2023

david-allison reviewed Dec 19, 2023

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/export/ActivityExportingDelegate.kt Outdated Show resolved Hide resolved

david-allison approved these changes Dec 19, 2023

View reviewed changes

use saved instance state to avoid the crash if activity is destroyed

352afe5

criticalAY force-pushed the export_deck_snackbar branch from d3d3286 to 352afe5 Compare December 20, 2023 07:28

fixing test issues

8419f66

david-allison merged commit 675f5f4 into ankidroid:main Dec 20, 2023
7 checks passed

github-actions bot added this to the 2.17 release milestone Dec 20, 2023

github-actions bot removed Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) squash-merge The pull request currently requires maintainers to "Squash Merge" labels Dec 20, 2023

SanjaySargam mentioned this pull request Dec 23, 2023

fix crash when sharing Export Collection #15059

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use `SaveInstanceState` to avoid crash in export file when Activity is recreated #14797

use `SaveInstanceState` to avoid crash in export file when Activity is recreated #14797

criticalAY commented Nov 22, 2023 •

edited by lukstbit

criticalAY commented Nov 22, 2023

lukstbit commented Nov 24, 2023 •

edited

criticalAY commented Nov 24, 2023

criticalAY commented Nov 24, 2023

github-actions bot commented Dec 8, 2023

criticalAY commented Dec 8, 2023

lukstbit left a comment •

edited

lukstbit Dec 9, 2023

mikehardy Dec 11, 2023

lukstbit Dec 11, 2023

david-allison commented Dec 11, 2023

mikehardy Dec 11, 2023

criticalAY Dec 11, 2023

mikehardy Dec 11, 2023

criticalAY Dec 11, 2023

criticalAY Dec 11, 2023

david-allison commented Dec 11, 2023

criticalAY commented Dec 11, 2023

criticalAY commented Dec 17, 2023

david-allison left a comment

david-allison left a comment

mikehardy commented Dec 18, 2023

criticalAY commented Dec 18, 2023

mikehardy Dec 18, 2023

mikehardy left a comment

david-allison left a comment

david-allison commented Dec 19, 2023

criticalAY commented Dec 19, 2023

github-actions bot commented Apr 13, 2024

use SaveInstanceState to avoid crash in export file when Activity is recreated #14797

use SaveInstanceState to avoid crash in export file when Activity is recreated #14797

Conversation

criticalAY commented Nov 22, 2023 • edited by lukstbit

Purpose / Description

Fixes

How Has This Been Tested?

Checklist

criticalAY commented Nov 22, 2023

lukstbit commented Nov 24, 2023 • edited

criticalAY commented Nov 24, 2023

criticalAY commented Nov 24, 2023

github-actions bot commented Dec 8, 2023

criticalAY commented Dec 8, 2023

lukstbit left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-allison commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-allison commented Dec 11, 2023

criticalAY commented Dec 11, 2023

criticalAY commented Dec 17, 2023

david-allison left a comment

Choose a reason for hiding this comment

david-allison left a comment

Choose a reason for hiding this comment

mikehardy commented Dec 18, 2023

criticalAY commented Dec 18, 2023

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

david-allison left a comment

Choose a reason for hiding this comment

david-allison commented Dec 19, 2023

criticalAY commented Dec 19, 2023

github-actions bot commented Apr 13, 2024

use `SaveInstanceState` to avoid crash in export file when Activity is recreated #14797

use `SaveInstanceState` to avoid crash in export file when Activity is recreated #14797

criticalAY commented Nov 22, 2023 •

edited by lukstbit

lukstbit commented Nov 24, 2023 •

edited

lukstbit left a comment •

edited