Use Storage Access Framework on Android running SDK >= 21 so writing to
sdcard will work again
 |  |  |
| API 21+ | API 19- | Edit |
Details
Details
- Reviewers
albertvaka sredman - Group Reviewers
KDE Connect - Commits
- R225:a6fdddf84379: Use Storage Access Framework on SDK >= 21 (Lollipop and above)
Install patch on Android phone with Build.Version < 19 (Kitkat)
- Without a sdcard: Verify that dolphin displays an "All Files" entry that is empty
- With a sdcard and with "Add camera folder shortcut" turned off: Verify that dolphin displays the configured display name of the sdcard
- With a sdcard and with "Add camera folder shortcut" turned on: Verify that dolphin displays the configured display name of the sdcard and also lists a "Camera pictures" shortcut
- With a sdcard: Verify that when changing the display name or the "Add camera folder shortcut" preference dolphin displays the updated items (after pressing F5)
- With a sdcard: Verify that files can be read and written to/from the sdcard
Install patch on Android phone with Build.Version < 19 (Kitkat)
- Repeat the above tests except for the read/write test: Verify that files can be read from the sdcard
Install patch on Android phone with Build.Version > 21 (Lollipop)
- Without any configured storage locations: Verify dolphin displays an "All Files" entry that is empty
- With configured storage locations: Verify dolphin displays the display names of the configured storage locations and that entering a location displays the correct directory entries
- Make one or several changes to the configured storage locations: Verify dolphin displays the display names of the configured storage locations (after pressing F5) and that entering a location displays the correct directory entries
Diff Detail
Diff Detail
- Repository
- R225 KDE Connect - Android application
- Branch
- arcpatch-D18212
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 9299 Build 9317: arc lint + arc unit
Comment Actions
- Properly handle the situation where no storage locations have been configured
- Handle preference changes
- Allow deleting of the last storage location
- Always create the right FileSystemView depending on Build.VERSION
- Remove erroneous leftover from re-fractoring SftpSettingsActivity into SftpSettingsFragment
- Commented out entries needed to debug org.apache.sshd
- Remove unused variables
- Remove optional WRITE_EXTERNAL_STORAGE permission because it is always granted on Build.VERSION < KITKAT
- Remove required WRITE_EXTERNAL_STORAGE permission because it is always granted on Build.VERSION < 23 at installation time and we now use SaF starting Build.VERSION >= 21
- Remove unneeded PreferenceFragmentCompat.OnPreferenceStartScreenCallback
Comment Actions
USB OTG Testing looks good:
- Before this patch, I was able to read/write USB OTG storage already
- With this patch, I was able to add USB OTG as an exposed filesystem
- After adding as an exposed filesystem, I was able to read/write the USB OTG drive
Tested device: OnePlus 6T, Android version 9.0
Comment Actions
The downside of this patch is it changes the default: I was very confused that I couldn't immediately see my USB drive and internal storage from Dolphin. Our per-plugin settings are very non-obvious :/. Is it possible to either display a dummy "you need to configure settings" file, or to show everything by default (as things currently are) and leave the settings so advanced users can hide filesystems if they like?
Comment Actions
Default behavior is the same for kitkat and earlier. For anything newer the Storage access Framework is used and that requires the user to pick a tree document (Directory) before read/write access is granted. That's what you do with the plugin settings. Also there is no way for me to determine which document providers there are (need to check android bug system). Dummy file or readme.md would be possible.
Comment Actions
Since the configuration for plugins is quite hidden, I agree with Simon it can be confusing. I have two ideas:
- If there are no storage locations configured, show the SFTP plugin in the list of "not loaded" plugins. The same way we do for plugins that need permissions. When the user taps it we show the config screen. We can also add a small text explanation in the config screen.
- Fallback to current implementation unless SAF locations are added.
Comment Actions
I'll implement Alberts 1st idea because the second is just not fool-proof and will only allow read-only access to the sdcard.
As soon as we can handle disabled plugins properly I think it's better to disable the plugin until storage locations have been configured
Comment Actions
- Properly handle the situation where no storage locations have been configured
- Handle preference changes
- Allow deleting of the last storage location
- Always create the right FileSystemView depending on Build.VERSION
- Remove erroneous leftover from re-fractoring SftpSettingsActivity into SftpSettingsFragment
- Commented out entries needed to debug org.apache.sshd
- Remove unused variables
- Remove optional WRITE_EXTERNAL_STORAGE permission because it is always granted on Build.VERSION < KITKAT
- Remove required WRITE_EXTERNAL_STORAGE permission because it is always granted on Build.VERSION < 23 at installation time and we now use SaF starting Build.VERSION >= 21
- Remove unneeded PreferenceFragmentCompat.OnPreferenceStartScreenCallback
- Show plugin as failed on KitKat and older devices when no sdcard is detected
Comment Actions
Cool! I like the warning popup!
Does the system automatically configure the device 'root' now? I can't tell if that was there by default or if it was just left over from when I had this before...
Is there a length limit to the warning? I would still vote for something along the lines of "Please configure some storage locations in the plugin settings", just to make it clear what the resolution to the problem is
Comment Actions
No it does not and cannot because the user has to grant access
Is there a length limit to the warning? I would still vote for something along the lines of "Please configure some storage locations in the plugin settings", just to make it clear what the resolution to the problem is
Should be possible
Comment Actions
This has been stuck for super long, I'm really sorry... testing now.
I don't get any relevant place to pick from as a "storage location" on OnePlus 6 (only "Downloads"):
Why?
Also, really minor but I think the first field shouldn't be editable:
Comment Actions
This has been stuck for super long, I'm really sorry... testing now.
I don't get any relevant place to pick from as a "storage location" on OnePlus 6 (only "Downloads"):
Why?
I don't know if this is a OnePlus thing (I think it is not; I think you have to do the same thing on stock Android)
You need to go into the menu (Triple dots at the top right of the shown screenshot) and click "Show Internal Storage" -- This is a setting of the storage-picking thing, so you will now be able to see the internal storage regardless of which app caused it to open (KDE Connect, WhatsApp document share, email attachments, etc.)
In other words, I don't think this is something we have any control over
I'm giving this patch my 👍 since I have been the one reviewing before. It's a great change!
Comment Actions
This has been stuck for super long, I'm really sorry... testing now.
I don't get any relevant place to pick from as a "storage location" on OnePlus 6 (only "Downloads"):
Why?
Also, if you have a USB-C adapter, you can plug in a USB flash drive and it will show up under the list of storage locations along with Downloads and Internal Storage!
Comment Actions
This has been stuck for super long, I'm really sorry... testing now.
I don't get any relevant place to pick from as a "storage location" on OnePlus 6 (only "Downloads"):
Why?
Also, really minor but I think the first field shouldn't be editable:
The field shouldn't be editable only clickable what API are you testing on? I'll have a look tomorrow. Signed my contract today so having a little party tonight
Comment Actions
Sorry for all this updates to your patch >.< I thought rebasing would be way simpler...
Comment Actions
I've tested it on API 19, API 21, and API 28 and seems to work well.
Just a few minor things inlined, plus that field being editable on my OnePlus 6 with API 28.
Iron those tiny things and you have my +1.
Note that I rebased the code onto master! You can get my changes with arc patch D18212 instead of rebasing it yourself and re-doing the work.
| src/org/kde/kdeconnect/Plugins/SftpPlugin/SftpPlugin.java | ||
|---|---|---|
| 197 ↗ | (On Diff #53387) | This default should be true to match the default state of the setting. I had to disable and re-enable the setting for the Camera folder to appear. |
| src/org/kde/kdeconnect/Plugins/SftpPlugin/SftpSettingsFragment.java | ||
| 323 ↗ | (On Diff #53387) | I can write to the internal phone storage (sdcard0) on API 19, although it appears as read only because of this condition. Maybe you had a different experience while testing? To simplify this code, since it's only for old APIs, maybe remove appending the "read only" string? It could be that every device is different, and determining if it is actually going to be read-only is difficult. If that's the case, I don't think it's worth trying to get it right just for old APIs. |
| src/org/kde/kdeconnect/Plugins/SftpPlugin/SftpSettingsFragment.java | ||
|---|---|---|
| 323 ↗ | (On Diff #53387) | I apparently did not test if writing to API 19 was possible. I just assumed that because writing to sdcard on KitKat was not possible using the File API it was also not possible using SaF. |
Comment Actions
- Use same default preference value for "add camera shortcuts" preference as defined in xml
- Attempt to make EditText not editable on OnePlus 6