Skip to content
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

Enhancement: AnkiDroid should not use Storage permission (Deadline November 2021) #5304

Closed
3 tasks done
saleemrashid opened this issue May 11, 2019 · 53 comments · Fixed by #13261
Closed
3 tasks done

Comments

@saleemrashid
Copy link

saleemrashid commented May 11, 2019

Reproduction Steps
  1. Start AnkiDroid for the first time
  2. Click "Deny" on the "Allow AnkiDroid to access photos, media and files on your device?" dialog
Expected Result

AnkiDroid should default to a directory that doesn't require WRITE_EXTERNAL_STORAGE. For example, /data/data/com.ichi2.anki/files/ (Context.getFilesDir()) or /sdcard/Android/data/com.ichi2.anki/ (Context.getExternalFilesDirs(null)).

Actual Result

AnkiDroid defaults to /sdcard/AnkiDroid and exits with "AnkiDroid directory is inaccessible".

Research
  • I have read the support page and am reporting a bug or enhancement request specific to AnkiDroid

  • I have checked the manual and the FAQ and could not find a solution to my issue

  • I have searched for similar existing issues here and on the user forum

@mikehardy
Copy link
Member

I don't disagree. Please feel free to propose a PR, it must be backwards compatible with existing installs

@MonikaJethani
Copy link

@mikehardy Is this done?Or Can I work on this?

@mikehardy
Copy link
Member

This is not done - and no one is working on it that I am aware of. I'm unsure of the requirements for acceptance - I mentioned "backwards-compatible with existing installs" but I have not thought through what that means. @timrae was thinking about this before and likely has ideas - Tim do you have any specific thoughts?

And @MonikaJethani I will say that since this affects the app very deeply - it is where people have their beloved collections stored :-) - probably best to post your detailed implementation idea here before starting work, or if you use code to flesh out the idea propose a PR but prepared for the whole approach to be questioned at the idea level until we're sure it will work.

But hopefully that doesn't sound too discouraging, it would be great for AnkiDroid to use the system directory access APIs and not require special permission, and a PR that did that would be great in my opinion

@saleemrashid
Copy link
Author

saleemrashid commented May 14, 2019

@mikehardy I would implement it thusly:

  • Add a new preference named along the lines of "Use internal storage". On existing installations, it should be disabled, but on new installations it should default to enabled.

  • If it is enabled, the "AnkiDroid directory" preference should be disabled and its value overridden with Context.getFilesDir().

  • If it is disabled, then use the value of "AnkiDroid directory" and prompt for Storage permissions if necessary.

Using the new preference should have the same semantics as changing the "AnkiDroid directory" preference which, as far as I know, just initializes the new directory and doesn't touch the old directory.

In the long run, the "AnkiDroid directory" preference will become obsolete because apps targeting Android Q will not be able to access external storage (they will be given a sandboxed view and must use specific APIs which is a huge privacy improvement).

@mikehardy
Copy link
Member

Solid start - interesting, I try to read the preview API changes but haven't had time for Q as of yet.

I have read it now though, and I have a slightly different take on Q's changes though - 2 big things:

  • With regard to internal vs external - https://developer.android.com/reference/android/content/Context.html#getExternalFilesDir(java.lang.String) will be a viable route to external storage. This is a big deal. You're discussing "internal storage", but note the enhancement is to drop the "Storage permission". They are separate things. I submit we should target dropping the permission and becoming Q-sandbox compliant as a criteria for any PR here, but that's still separate from internal vs external. I do not want to enforce or set a default for internal storage as the collections many people use are large. So external storage should be default, but to meet the drop-permission and Q-sandbox criteria, using Context#getExternalFilesDir() seems best. I support the idea of internal storage as an option (again through the Q-sandbox compliant Context API) but I would not default to internal.

  • With regard to access on existing /sdcard/AnkiDroid - I quote from your link:

An app that has a sandboxed view always has access to the files that it creates, both 
inside and outside its app-specific directory. 

as well as:

File management and media creation apps typically manage groups of files in a 
directory hierarchy. These apps can invoke the ACTION_OPEN_DOCUMENT_TREE intent to 
allow the user to grant access to an entire directory tree. Such an app would be able 
to edit any file in the selected directory, as well as any of its sub-directories.

Using this interface, users can access files from any installed instance of 
DocumentsProvider, which any locally-backed or cloud-based solution can support.

This makes me think that we could continue to access /sdcard/AnkiDroid even after Q, because we know the locations. We'd have to test whether we can just maintain access since we created the files originally or whether we need to obtain permission in Q-sandbox mode With an intent and a conversion to a DocumentsProvider but it should be possible? Not ideal, but "external, legacy mode" does not appear to be forbidden even in the scoped-storage Q-sandbox, just irritating.

So I would propose a slightly different idea -

  • have a 3-state drop-down for where to store things
    -- "external, legacy mode" visible in and default only for existing installs that still have data in /sdcard/AnkiDroid. if selected, there is a preference for setting the directory to use, defaults to /sdcard/AnkiDroid. For new installs does not exist
    -- "external" as default for new installs, option for others, if selected, uses Context#getExernalFilesDir()
    -- "internal" as option for others, uses Context#getFilesDir()

This would get us to a place where we were able to work in either of the three areas - a big step and functional / usable at this point. But it would just initialize the directory, as you say.

  • Then - and most important for the future I think - we have code that performs migrations between the locations when the user changes storage location. A dialog indicating what will happen and that it may take a while for large collections, then an AsyncTask doing the work with a modal dialog showing progress (all very very similar to the database check code). Implementation should be just closing the database, calling filesystem moves on the directory, and restarting the activity with an intent that has extra info indicating what we did so we can show a success or fail dialog to the user.

This part is vital or if I understand the scoped-storage Q-sandbox we may have to implement DocumentsProvider access and the DOCUMENT_TREE intent thing just to keep accessing /sdcard/AnkiDroid or we will orphan the data of a 1.3 million active user installed base (!)

Finally, I note this warning on the Q link, which is I think talking about the year 2020 R platform:

Scoped storage will be required in next year's major platform release for all apps, 
independent of target SDK level. Therefore, you should ensure that your app works 
with scoped storage well in advance. To do so, make sure that the behavior is enabled 
on Android Q devices running your app.

So that gives us around a year to get this done and have it rolled out long enough to migrate people, which leads to the final step:

  • Finally on version upgrades in the startup show dialogs code flow, we start pushing people with a popup explaining the need to migrate, the deadline, and offering the choice to go the migrate preference that does it, so we can move people from external legacy to external and move towards sandboxed Q compliance

What do you think? Not especially different for the first step I think, but a little twist there (prefer external), but the 2nd (migration) and 3rd (popup to push migration) are big differences. ?

@timrae
Copy link
Member

timrae commented May 15, 2019

Add a new preference named along the lines of "Use internal storage". On existing installations, it should be disabled, but on new installations it should default to enabled.

We used to have a setting for this, which nobody used. We can't make it the default location as many people have collections on the order of hundreds of megabytes. This is not an approach that I would be willing to take.

@timrae
Copy link
Member

timrae commented May 15, 2019

Ideally I'd like to avoid add a preference for this. We should just migrate to the new APIs as needed. Most likely that means increasing the minimum SDK, so we'll need to discuss the best way to handle that.

@timrae
Copy link
Member

timrae commented May 15, 2019

From an implementation perspective, I think this issue is the same as #3106. It's not a small job, but if you have the time and patience to see it through to the end then I'd be very happy to start a design discussion.

@mikehardy
Copy link
Member

Ideally I'd like to avoid add a preference for this. We should just migrate to the new APIs as needed. Most likely that means increasing the minimum SDK, so we'll need to discuss the best way to handle that.

No preference - fine by me. I can imagine that being very simplifying

As for the API mention, the only necessary API I think is https://developer.android.com/reference/android/content/Context#getExternalFilesDir(java.lang.String) which is API8

If we had to DocumentsProvider for some reason (which we would not, assuming we moved inside of Context#getExternalFilesDir, if I understand correctly), that's API19 / KitKat / 4.4 and represents about 11,000 active users out of about 1,127,000 according to the play store just now. Easily a 2.10 bump and justifiable for 2.9 this moment if it opened things up for this work to proceed cleanly, IMHO

@saleemrashid
Copy link
Author

We used to have a setting for this, which nobody used. We can't make it the default location as many people have collections on the order of hundreds of megabytes. This is not an approach that I would be willing to take.

@timrae All devices that AnkiDroid supports should use the same underlying storage for internal storage and external storage. If users are using a microSD card, that is not the same as the path returned by Context#getExternalStorageDirectory. Therefore, there's little to no point in supporting both Context#getFilesDir and Context#getExternalStorageDirectory

@timrae
Copy link
Member

timrae commented May 23, 2019

As far as I know, that's incorrect. Most devices have a separate partition on the internal SD card with root only permissions, and this is where the applications and application data (Context#getFilesDir) gets stored. This partition is much smaller than the non-root partition, so you can't store large files there.

@mikehardy
Copy link
Member

I was thinking the same thing but wanted to back it up with docs or a test and didn't have time - I do believe they have differences though, just wanted proof

@saleemrashid
Copy link
Author

saleemrashid commented May 25, 2019

@timrae Most devices that ship with Android 4 and above (which is the minimum API level supported by AnkiDroid) use a single partition, and a FUSE filesystem to emulate the SD card.

The only devices that support AnkiDroid and use a separate partition will be devices that shipped with an old version of Android and are running a custom ROM or a few low-end exceptions.

@timrae
Copy link
Member

timrae commented May 25, 2019 via email

@Konotorious
Copy link

I'm not so familiar with storage and permission management on Android and so I can't say for sure whether I got it all correctly, but from my understanding my issue (which I was hesitant to open a new thread for) is related to the above.

I have an Android 6.0.1 and the latest Ankidroid app installed. I have an SD card to which I moved the Anki app via the application manager, since it has more storage capacity than my phone's internal storage.
However––– all the media (and backups) are still on the internal storage. The application manager shows that AnkiDroid uses 22.88MB of (external) storage, including "364KB of data". My naive understanding therefore is that Android doesn't treat the collection.media as part of the AnkiDroid app, but simply as any other files which AnkiDroid simply happens to manipulate.
By itself it would not be a problem, it would even be an advantage, if only it was possible (as the end user) to dictate AnkiDroid the path to the media directory, but I'm not aware that this is possible. As it is now, I have Anki's (indeed a few hundreds of MBs of) media dir stored in an immovable manner, which is a problem given the precious real estate value of the internal phone's storage.

@timrae timrae closed this as completed Jul 19, 2019
@timrae timrae reopened this Jul 19, 2019
@timrae
Copy link
Member

timrae commented Jul 19, 2019

Hi sorry I accidentally pressed the close button. Your assessment that Ankidroid does not treat the flashcard and media databases as part of the app is correct. We do not officially support using external SD cards. However there is a workaround available to get this working if you wish to s do it anyway. Please read the FAQ for more info on this.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2020

Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Jun 3, 2020
@mikehardy
Copy link
Member

I had some recent experience with this where if you use the directories provided as getExternalStorage I think you can actually drop the permission. Problem being that we don't use that directory currently. Either way this is a popular request, I'll leave it open

@mikehardy mikehardy added Keep Open avoids the stale bot and removed Stale labels Jun 3, 2020
@AndreHero007
Copy link

Ah, I see. You have slow syncing. The thing is, slow sync should only happen until the first sync is completed. Stated differently: once you are in sync once, all future syncs should be fast. If that is not the case something is going on, and even when this is closed that will not fix it. Yes, righ tnow, that first full sync can take multiple attempts, and hours perhaps for 1GB, but it's a one time effort while the media is downloaded. I cannot determine from your comment if you have ever stuck with it (tried multiple times) until all media files were truly in sync, or not.

Hello, can you tell me if the Alpha Version are better than Play Store version? Do you think I should upgrade to an Alpha version or continue using the Google Play version?

@mikehardy
Copy link
Member

@AndreHero007 it depends. Do you want the V3 scheduler? Alpha is the only way. Are you willing to accept some instability that could disrupt your studies and/or create the need to reinstall AnkiDroid, redownload everything? Then the Alpha is a terrible idea. Need some specific new mathjax thing? Alpha is the only way right now. Are not comfortable with reporting app crashes, helping fix problems? Alpha is a terrible idea.

I can't recommend anything to anyone, it depends on use case and person

@AndreHero007
Copy link

@AndreHero007 it depends. Do you want the V3 scheduler? Alpha is the only way. Are you willing to accept some instability that could disrupt your studies and/or create the need to reinstall AnkiDroid, redownload everything? Then the Alpha is a terrible idea. Need some specific new mathjax thing? Alpha is the only way right now. Are not comfortable with reporting app crashes, helping fix problems? Alpha is a terrible idea.

I can't recommend anything to anyone, it depends on use case and person

For example, this feature "Cloze deletions can now be nested inside other cloze deletions" doesn't seem to work on AnkiDroid. Does it work on Alpha versions?

@mikehardy
Copy link
Member

@AndreHero007 it may? Try a parallel version and see - that's relatively low risk

@david-allison david-allison self-assigned this Mar 4, 2023
david-allison added a commit to david-allison/Anki-Android that referenced this issue Mar 4, 2023
A script to enable/disable scoped storage without the need to
uninstall/upgrade from `targetSdkVersion 29`

Makes testing of the disable/enable routine quicker

Issue ankidroid#5304
david-allison added a commit to david-allison/Anki-Android that referenced this issue Mar 4, 2023
A script to enable/disable scoped storage without the need to
uninstall/upgrade from `targetSdkVersion 29`

Makes testing of the disable/enable routine quicker

Issue ankidroid#5304
david-allison added a commit to david-allison/Anki-Android that referenced this issue Mar 5, 2023
A script to enable/disable scoped storage without the need to
uninstall/upgrade from `targetSdkVersion 29`

Makes testing of the disable/enable routine quicker

Issue ankidroid#5304
david-allison added a commit to david-allison/Anki-Android that referenced this issue Mar 5, 2023
A script to enable/disable scoped storage without the need to
uninstall/upgrade from `targetSdkVersion 29`

Makes testing of the disable/enable routine quicker

Issue ankidroid#5304
david-allison added a commit to david-allison/Anki-Android that referenced this issue Mar 5, 2023
A script to enable/disable scoped storage without the need to
uninstall/upgrade from `targetSdkVersion 29`

Makes testing of the disable/enable routine quicker

Issue ankidroid#5304
mikehardy pushed a commit that referenced this issue Mar 5, 2023
A script to enable/disable scoped storage without the need to
uninstall/upgrade from `targetSdkVersion 29`

Makes testing of the disable/enable routine quicker

Issue #5304
@lervag
Copy link

lervag commented Mar 20, 2023

🥇

KendallPark pushed a commit to KendallPark/Anki-Android that referenced this issue May 30, 2023
A script to enable/disable scoped storage without the need to
uninstall/upgrade from `targetSdkVersion 29`

Makes testing of the disable/enable routine quicker

Issue ankidroid#5304
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet