Use Storage Access Framework on SDK >= 21 (Lollipop and above)
ClosedPublic

Authored by eduisters on Jan 12 2019, 6:32 PM.

Details

Summary

Use Storage Access Framework on Android running SDK >= 21 so writing to
sdcard will work again

API 21+API 19-Edit
Test Plan

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

Repository
R225 KDE Connect - Android application
Branch
sftp_saf
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7415
Build 7433: arc lint + arc unit
eduisters created this revision.Jan 12 2019, 6:32 PM
Restricted Application added a project: KDE Connect. · View Herald TranscriptJan 12 2019, 6:32 PM
Restricted Application added a subscriber: kdeconnect. · View Herald Transcript
eduisters requested review of this revision.Jan 12 2019, 6:32 PM
eduisters edited the summary of this revision. (Show Details)Jan 12 2019, 6:45 PM
eduisters edited the test plan for this revision. (Show Details)
eduisters added a reviewer: KDE Connect.
eduisters planned changes to this revision.Jan 12 2019, 10:37 PM

Forgot to remove some WIP stuff

eduisters updated this revision to Diff 49386.Jan 13 2019, 2:27 PM
eduisters edited the summary of this revision. (Show Details)
eduisters edited the test plan for this revision. (Show Details)
  • 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
eduisters edited the test plan for this revision. (Show Details)Jan 13 2019, 2:52 PM
eduisters edited the test plan for this revision. (Show Details)
sredman added a subscriber: sredman.EditedJan 13 2019, 4:24 PM

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

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?

eduisters added a comment.EditedJan 13 2019, 4:36 PM

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?

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.

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.
eduisters planned changes to this revision.Jan 16 2019, 3:41 PM

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

eduisters updated this revision to Diff 50069.Jan 22 2019, 3:54 PM
  • 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

Kling klokje klingelingeling

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

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

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

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:

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!

sredman accepted this revision as: sredman.Mar 7 2019, 7:03 PM
This revision is now accepted and ready to land.Mar 7 2019, 7:03 PM

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!

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

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

Android Pie / 9 / API 28.

Enjoy your party :) Cheers from here!

albertvaka updated this revision to Diff 53388.Mar 7 2019, 7:26 PM

Now without conflicts

albertvaka updated this revision to Diff 53391.Mar 7 2019, 7:56 PM

Updated to new getOptionalPermissionExplanationDialog interface

Sorry for all this updates to your patch >.< I thought rebasing would be way simpler...

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
196

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

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.

eduisters marked 2 inline comments as done.Mar 8 2019, 9:27 AM
eduisters added inline comments.
src/org/kde/kdeconnect/Plugins/SftpPlugin/SftpSettingsFragment.java
323

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.

eduisters updated this revision to Diff 53433.Mar 8 2019, 12:45 PM
eduisters marked an inline comment as done.
  • Use same default preference value for "add camera shortcuts" preference as defined in xml
  • Attempt to make EditText not editable on OnePlus 6

Can you try if the EditText is now not editable anymore on you OnePlus 6?

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.

albertvaka accepted this revision.Mar 8 2019, 2:00 PM

Fixes it for the OnePlus 6

eduisters closed this revision.Mar 8 2019, 2:02 PM