Support SAF properly #3777

wb9688 · 2020-06-13T15:51:10Z

What is it?

Bug fix (user facing)
Feature (user facing)
Code base improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

Use SAF or NoNonsense-FilePicker everywhere based on the setting.
Don't have requestLegacyExternalStorage anymore in the manifest.
Only allow NoNonsense-FilePicker for versions below Android 5 and only allow SAF starting at Android 10.
Move SharpInputStream into NewPipe and add SharpOutputStream.
Show folder picker in download dialog if no download folder set yet.

I probably need to clean up some code and we should definitely do some more testing.

Fixes the following issue(s)

Fixes #3465 and perhaps some others.

Testing apk

app-debug.apk.zip

Agreement

I carefully read the contribution guidelines and agree to them.

app/src/main/java/org/schabi/newpipe/settings/ContentSettingsFragment.java

wb9688 · 2020-06-14T11:27:44Z

app/src/main/java/us/shandian/giga/io/StoredFileHelper.java

+            if (startPath != null && Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
+                i.putExtra(DocumentsContract.EXTRA_INITIAL_URI, Uri.parse(startPath));
+            }


Currently we're not using this

app/src/main/java/us/shandian/giga/io/StoredFileHelper.java

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

wb9688 · 2020-06-14T14:33:15Z

@imcmjha, @domiuns, @opusforlife2: Would you mind testing this? You should test whether all file pickers (import/export DB, import/export subscription, download) show the correct file picker (SAF or NoNonsense-FilePicker) and whether saving or reading that file works correctly.

On Android 4.4-, SAF should be disabled and impossible to enable.

On Android 5-9, SAF should be enabled by default.

On Android 10+, SAF should be enabled and impossible to disable.

opusforlife2 · 2020-06-14T21:42:23Z

I have Android 9:

Export/Import subscriptions works perfectly fine with both SAF and NNFP.
Instead of showing the full content URI, maybe the text up to "/tree/primary:" should be replaced by "Internal storage: " followed by the user's selected path?
SAF is enabled by default on first run, which is correct.
- However, SAF is also automatically enabled whenever you open the Download menu. Sure enough, the app uses SAF when you try to import/export. The preference should stay off if the user toggles it off. If you don't open the Download menu after toggling it off, the app uses NNFP.
- Even though SAF is automatically toggled on, the download location strings still show the NNFP locations. Toggling it twice shows the default SAF strings ("Downloaded video/audio files are stored here").
- Repeatedly toggle the pref a few times. Notice that the app sets a default location in case of NNFP, but not in case of SAF. Either the same folders should be set as content URIs by default in case of SAF too, or the app should ask for new default locations for NNFP as well.
- I suspect you should also check from logs if SAF is toggled on if you close then open the app. I can't check this from just the UI because of this bug.

wb9688 · 2020-06-14T21:55:35Z

@opusforlife2:

Instead of showing the full content URI, maybe the text up to "/tree/primary:" should be replaced by "Internal storage: " followed by the user's selected path?

No, as different storage providers will start with a different thing.

However, SAF is also automatically enabled whenever you open the Download menu. Sure enough, the app uses SAF when you try to import/export. The preference should stay off if the user toggles it off. If you don't open the Download menu after toggling it off, the app uses NNFP.

You mean when you open the downloads activity where you could see all the things you downloaded? Hmm… that setting shouldn't be changed unless the user changes it (or you upgrade to Android 10+, as then only SAF will be possible).

Even though SAF is automatically toggled on, the download location strings still show the NNFP locations. Toggling it twice shows the default SAF strings ("Downloaded video/audio files are stored here").

I'm pretty sure I fixed that bug, though maybe only for Android 10 or maybe I forgot to update the APK in this issue.

Repeatedly toggle the pref a few times. Notice that the app sets a default location in case of NNFP, but not in case of SAF. Either the same folders should be set as content URIs by default in case of SAF too, or the app should ask for new default locations for NNFP as well.

That can't be done with SAF, because you don't know what that path would be and you have to acquire permission to be able to use that (only possible by opening the directory picker for that).

I suspect you should also check from logs if SAF is toggled on if you close then open the app. I can't check this from just the UI because of this bug.

What do you mean?

opusforlife2 · 2020-06-14T21:58:32Z

Some questions that came up while bug testing:

Is Subscription Import a full replacement operation? Can/Shouldn't there be an additional option to compare the JSON to be imported with one's already existing Subscription database so that only new channels are imported?
Why is the default download location for NNFP "Music/Movies > Newpipe"? I think it should be "Newpipe > Videos/Audios". We can't assume the user wants media downloaded through Newpipe to be added to their music/video player libraries by default. I tend to think of it as 'contamination', because those two folders are usually exclusively accessed by music/video player apps. It's perfectly alright if the user wants to download straight to those folders, but it shouldn't be the default behaviour.
Is this a bug or expected behaviour? : Delete a file (not just its entry) from the Downloads activity. It will still be present in the file system and even be playable. It only gets deleted from the file system when you back out of the app entirely.

opusforlife2 · 2020-06-14T22:23:53Z

You mean when you open the downloads activity where you could see all the things you downloaded?

No, I mean the download settings menu. Sorry, that was ambiguous.

What do you mean?

Leave it. It's just a precaution. I'll test it once the above bug is fixed.

opusforlife2 · 2020-06-14T22:39:26Z

Export/Import DB also seems to work fine with both SAF and NNFP.

Question: Does export/import DB work like a complete app backup, or does it leave something out?

wb9688 · 2020-06-14T22:40:55Z

@opusforlife2: It should be a complete app backup, other than the download history.

opusforlife2 · 2020-06-14T22:42:16Z

Alright. Then everything works. 👍

imcmjha · 2020-06-15T02:06:08Z

I tested in Android 9 and it is working perfectly. Bug 3465 resolved. Thanks a lot.

wb9688 · 2020-06-15T07:03:20Z

@opusforlife2: I fixed the bug regarding SAF looking like it was enabled again when opening the download settings. Could you try the new APK (it also includes #3781, which shouldn't change anything for users)?

Also, did you notice how the download dialog now shows a directory picker when you haven't set a download folder yet?

Btw the only thing that I haven't tested yet is an upgrade from Android 5-9 to Android 10+ and an upgrade from previous NewPipe to NewPipe with this PR.

opusforlife2 · 2020-06-15T08:04:14Z

Confirmed fixed.

Also, did you notice how the download dialog now shows a directory picker when you haven't set a download folder yet?

I didn't know that it didn't before. This is the first time I'm using SAF. Good, though. Adds to the seamlessness.

xibr · 2020-06-15T08:17:02Z

Everything seems to work correctly on Android 10

B0pol · 2020-06-15T11:11:45Z

With SAF it works fine on android 10 … but the import / export will pick the last used folder, both import or download

Let's say you export in NewPipe/Save, download musics in NewPipe/Music, videos in NewPipe/Videos. Music will always be downloaded in NewPipe/Music, and videos in NewPipe/Videos.
Monday, you export your database (in NewPipe/Save). You download music tuesday (NewPipe/Music), make a playlist wesnesday therefore you want to export your database. But it will show the last used folder, i.e. NewPipe/Music. That make no sense, you want NewPipe to show up NewPipe/Save as you picked it on monday.

wb9688 · 2020-06-15T11:32:20Z

@B0pol: That's just default behavior of SAF. Default behavior of NNFP is to always start at /sdcard/. The only other thing we could do is add a new preference for that (not necessarily user-visible in the settings) and use that here.

Stypox

Looks good to me, though I haven't worked much with the downloader so far, so I don't know if my opinion is really useful. Could you provide a decryption-fix-updated apk, please? Also for the other PRs of yours

app/src/main/java/org/schabi/newpipe/settings/ContentSettingsFragment.java

app/src/main/java/org/schabi/newpipe/settings/DownloadSettingsFragment.java

app/src/main/java/org/schabi/newpipe/settings/NewPipeSettings.java

ghost · 2020-07-29T15:01:25Z

@wb9688 when try to import all setting as zip app crash.

wb9688 · 2020-07-29T15:07:06Z

@domiuns: On 4.4? What error are you getting?

Stypox · 2020-09-03T17:56:07Z

@domiuns could you further explain what you mean by "not works"? Does the app crash? If that's the case and by "log is empty" you mean that there is no stacktrace in the bugreport screen, then @wb9688 you might need to provide a rebased apk with the bugreport fix

ghost · 2020-09-04T14:45:43Z

@Srypox this apk work #3777 (comment)
But the apk uploaded on start of conversation not work (from this instruction #3777 (comment) )

Stypox · 2020-09-22T05:14:09Z

After merging this we should add a setting migration that enables SAF on every device possible, and then let the user disable it if they want, otherwise almost nobody would know the switch exists

opusforlife2 · 2020-10-02T05:01:31Z

Only allow NoNonsense-FilePicker for versions below Android 5 and only allow SAF starting at Android 10.

Is there any downside to only allowing SAF on Android 5+ instead of 10+?

Stypox · 2020-10-02T11:49:37Z

@opusforlife2 I think the "only" is misplaced there. What he means is that users with Android 10+ will only see saf, and users with <5 will only see nononsense, while all versions in between will see both.

opusforlife2 · 2020-10-02T15:22:38Z

I know what he meant. What I'm asking is if there is any downside of having SAF only for Android 5+. It will remove a setting and make things more consistent and code easier to maintain: NNFP only for Kitkat, SAF only for 5+.

Stypox · 2020-10-02T16:38:51Z

Oh ok
@wb9688 is there a reason not to do this?

wb9688 commented Jun 14, 2020

View reviewed changes

wb9688 marked this pull request as ready for review June 14, 2020 11:35

wb9688 added this to the 0.19.6 milestone Jun 14, 2020

wb9688 mentioned this pull request Jun 14, 2020

Move Stored(File|Directory)Helper into NewPipe #3781

Closed

5 tasks

This was referenced Jun 15, 2020

Use file picker for export DB #3785

Closed

Videos from specific channel only not playing #3795

Closed

wb9688 mentioned this pull request Jun 25, 2020

YT OPLM file import issue #3818

Closed

TobiGr added downloader Issue is related to the downloader GUI Issue is related to the graphical user interface labels Jun 27, 2020

wb9688 modified the milestones: 0.19.6, 0.20.0 Jul 2, 2020

wb9688 mentioned this pull request Jul 13, 2020

Release 0.19.6 #3862

Closed

Stypox requested changes Jul 29, 2020

View reviewed changes

wb9688 requested a review from Stypox July 29, 2020 13:59

opusforlife2 mentioned this pull request Aug 13, 2020

NewPipe not Downloading #2961

Closed

avently mentioned this pull request Sep 3, 2020

bug tap show controll on player (unified UI) #4179

Closed

TobiGr modified the milestones: 0.20.0, 0.20.1 Sep 27, 2020

wb9688 mentioned this pull request Oct 2, 2020

AutoBackup #3157

Closed

3 tasks

Stypox modified the milestones: 0.20.1, 0.20.2 Oct 4, 2020

Stypox removed this from the 0.20.2 milestone Oct 4, 2020

wb9688 added 5 commits October 9, 2020 20:01

Support SAF properly

bc6e216

Move Stored(File|Directory)Helper into NewPipe

fb6d862

Use file picker for export DB

facfb24

Fix non-SAF actions

c4f76b3

Only ask for storage permissions when not using SAF

4efb1ec

wb9688 closed this Oct 24, 2020

Isira-Seneviratne mentioned this pull request Nov 21, 2020

Use List.of(). #4816

Closed

5 tasks

ShareASmile mentioned this pull request Dec 19, 2020

Database export to removable storage fails on Android 10 #5198

Closed

4 tasks

Stypox mentioned this pull request Jan 14, 2021

Support SAF properly #5415

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SAF properly #3777

Support SAF properly #3777

wb9688 commented Jun 13, 2020 •

edited

Loading

wb9688 Jun 14, 2020

wb9688 commented Jun 14, 2020

opusforlife2 commented Jun 14, 2020

wb9688 commented Jun 14, 2020

opusforlife2 commented Jun 14, 2020

opusforlife2 commented Jun 14, 2020

opusforlife2 commented Jun 14, 2020

wb9688 commented Jun 14, 2020

opusforlife2 commented Jun 14, 2020

imcmjha commented Jun 15, 2020 •

edited

Loading

wb9688 commented Jun 15, 2020 •

edited

Loading

opusforlife2 commented Jun 15, 2020

xibr commented Jun 15, 2020

B0pol commented Jun 15, 2020

wb9688 commented Jun 15, 2020 •

edited

Loading

Stypox left a comment

ghost commented Jul 29, 2020

wb9688 commented Jul 29, 2020

Stypox commented Sep 3, 2020

ghost commented Sep 4, 2020

Stypox commented Sep 22, 2020

opusforlife2 commented Oct 2, 2020

Stypox commented Oct 2, 2020

opusforlife2 commented Oct 2, 2020

Stypox commented Oct 2, 2020

Support SAF properly #3777

Support SAF properly #3777

Conversation

wb9688 commented Jun 13, 2020 • edited Loading

What is it?

Description of the changes in your PR

Fixes the following issue(s)

Testing apk

Agreement

wb9688 Jun 14, 2020

Choose a reason for hiding this comment

wb9688 commented Jun 14, 2020

opusforlife2 commented Jun 14, 2020

wb9688 commented Jun 14, 2020

opusforlife2 commented Jun 14, 2020

opusforlife2 commented Jun 14, 2020

opusforlife2 commented Jun 14, 2020

wb9688 commented Jun 14, 2020

opusforlife2 commented Jun 14, 2020

imcmjha commented Jun 15, 2020 • edited Loading

wb9688 commented Jun 15, 2020 • edited Loading

opusforlife2 commented Jun 15, 2020

xibr commented Jun 15, 2020

B0pol commented Jun 15, 2020

wb9688 commented Jun 15, 2020 • edited Loading

Stypox left a comment

Choose a reason for hiding this comment

ghost commented Jul 29, 2020

wb9688 commented Jul 29, 2020

Stypox commented Sep 3, 2020

ghost commented Sep 4, 2020

Stypox commented Sep 22, 2020

opusforlife2 commented Oct 2, 2020

Stypox commented Oct 2, 2020

opusforlife2 commented Oct 2, 2020

Stypox commented Oct 2, 2020

wb9688 commented Jun 13, 2020 •

edited

Loading

imcmjha commented Jun 15, 2020 •

edited

Loading

wb9688 commented Jun 15, 2020 •

edited

Loading

wb9688 commented Jun 15, 2020 •

edited

Loading