Support SAF properly #5415

Stypox · 2021-01-14T20:43:37Z

What is it?

Bugfix (user facing)
Feature (user facing)
Codebase improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

This is the rebased #3777, and also contains:

Setting migration that enables SAF everywhere possible.

Other than that, it is identical to #3777 (by @wb9688):

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

TODO

"SAF" in the UI should be replaced with something more easily understandable for non-Android devs, like "system folder picker"
Remove "some devices are not compatible" from SAF setting description
Investigate whether renaming the zip works - it works
~~Keeping the setting in the Download menu doesn't make sense anymore~~ implementing a miscellaneous menu is left for another PR
Investigate "NewPipe saf Leaks" - I can't understand why the name changed, but it's not important
Explain SharpOutputStream
"filename.mp4 (1)" is created instead of "filename (1).mp4" - fixed by passing mime type to file picker
Fix strange behaviour when app loses access to saf download folder
Reimplement storing import/export path

Fixes the following issue(s)

Fixes #3465, fixes #5198, and perhaps some others.

Due diligence

I read the contribution guidelines.

opusforlife2 · 2021-01-14T20:44:57Z

Wow you're on fire.

opusforlife2 · 2021-01-14T20:53:39Z

#3777 (comment)?

Stypox · 2021-01-14T21:15:33Z

@opusforlife2 I don't know, with the setting migration everyone should seamlessly switch to SAF, but I'd keep the option anyway since on some phones Files apps can be difficult to use, and some users might complain. The code wouldn't be simplified much if we were to remove the setting.

opusforlife2 · 2021-01-14T21:16:15Z

Viva la democracy.

Stypox · 2021-01-14T21:25:05Z

Could you confirm that all of the following things work? in the ci APK

Export database
Import database
Export subscriptions
Import subscriptions from previous export
Import subscriptions from YouTube
Setting default audio download path
Setting default video download path
Downloading audio
Downloading video
Downloading audio when "Ask where to download" is enabled
Downloading video when "Ask where to download" is enabled

Thanks in advance.

@imcmjha @xibr @B0pol @domiuns @fajis1 @mqus @MD77MD @Poolitzer @mhmdanas @opusforlife2

triallax · 2021-01-14T22:23:54Z

@Stypox I tested all the things you listed and faced no issues.

fajis1 · 2021-01-14T22:45:45Z

everything you asked me to look at worked, but there were some crashes that I had. I reported them via email.

When I was in subscriptions it would crash when I tried to leave the subscription area.

when I tried to play the videos and audio I downloaded from within the app download area. It worked for the files that auto downloaded to the specified folder. But if it asked me for the folder those would cause the app to crash when I tried to play the videos or audio.

The videos and audio played just fine when I found them with the file explorer etc. So they did download successfully.

xibr · 2021-01-14T23:00:51Z

Everything works perfectly, I don't know if this crash is related.

Go to subscriptions and then press back button the app crashes.

Edit: the crash log above is Not related to this PR, See #5414 #5417

Stypox · 2021-01-15T08:09:40Z

I fixed the tests, thank you @XiangRongLin for suggesting me to Sync gradle ;-)
Thank you for testing! Yeah, the random crashes when going back are caused by #5414 and fixed in #5417, so no need to worry about those. This is the apk built by the ci: https://github.com/TeamNewPipe/NewPipe/suites/1834544416/artifacts/35447435

mqus · 2021-01-15T09:13:45Z

Everything works for me (Android 4.4.2 (Kitkat, API19)), just two things:

I couldn't test import from youtube(no subscriptions there), but I don't think there will be any errors.
The SAF option in the settings is disabled and greyed out, though I assume that is intentional/necessary for this android version.

triallax · 2021-01-15T09:31:10Z

@mqus yes, that is intentional.

Stypox · 2021-01-15T18:09:54Z

@dkraft @blauertee @k9janer @bingoxo @illustribe @RickyM7 I am also pinging you for testing since you have Android 11, by looking at the issues. Could you test the apk above with the steps above?

RickyM7 · 2021-01-15T18:38:41Z

@dkraft @blauertee @k9janer @bingoxo @illustribe @RickyM7 I am also pinging you for testing since you have Android 11, by looking at the issues. Could you test the apk above with the steps above?

@Stypox In fact the version of android I use is 7.0, you must have been confused with the version of MIUI I use that is 11. Sorry!

Stypox · 2021-01-15T18:40:42Z

@RickyM7 oh, sorry ;-)
If you have some time to test anyway, I would appreciate it, so that we can make sure this works everywhere

RickyM7 · 2021-01-15T19:05:58Z

@Stypox Of course!

RickyM7 · 2021-01-15T19:24:37Z

@Stypox Ok, I tested what I could and everything is working perfectly. The only thing I couldn't test was "Import subscriptions from YouTube" because I don't have an account.

Device: Redmi Note 4 (MIUI 11 - Android 7.0)

Fs00 · 2021-01-15T19:31:55Z

If I may add, I think that "SAF" in the UI should be replaced with something more easily understandable for non-Android devs, like "system folder picker".
Also, is it correct to say in settings that "some devices are not compatible" with SAF, given that this PR enables it by default? 🙂

Stypox · 2021-01-15T19:43:13Z

If I may add, I think that "SAF" in the UI should be replaced with something more easily understandable for non-Android devs, like "system folder picker".

You are right, good idea

Also, is it correct to say in settings that "some devices are not compatible" with SAF, given that this PR enables it by default?

No, that has to be removed now. Before this PR there were incompatibilities, but now there shouldn't be anymore.

opusforlife2 · 2021-01-15T20:29:01Z

Tested everything except "Import subscriptions from YouTube". Werks. 🎉

Exporting database with current release (so NNFP) doesn't let you rename the zip, but this PR does. Intentional? Wasn't the ability to rename the export zip removed in an earlier version for some reason?

Before this PR, SAF was limited to downloads. But now it works with various other IO operations, so keeping the setting in the Download menu doesn't make sense anymore. But no other menu suits it either. Time for a new Miscellaneous menu? :D

Aside: Other debug apks show "Leaks" as an option, but your apk shows "NewPipe saf Leaks". How did that happen?

app/src/main/java/org/schabi/newpipe/streams/io/SharpInputStream.java

app/src/main/java/org/schabi/newpipe/settings/ContentSettingsManager.kt

app/src/main/java/org/schabi/newpipe/util/ZipHelper.java

Poolitzer · 2021-01-16T10:51:19Z

since the others checked everything else, I confirmed that importing from youtube works

k9janer · 2021-01-16T13:04:09Z

So far everything works on my Pixel 3a with GrapheneOS based on Android 11.

Just 4 things I have noticed:

Downloading video/audio while a video is playing and with the option "Ask where to download" enabled opens the save dialogue but then it immediately asks for permission to "Display over other apps" as NewPipe switches to a different player. You have to press back a few times to go back to the save dialogue.
When you download a file twice with the "Ask where to download" option enabled, the new file will be named "filename.mp4 (1)". (The .mp4 part isn't at the end of the file). NewPipe crashes when you try to open this download and other apps on the device won't recognize this file as a video file.
This might be an Android/GrapheneOS bug: When you set up a default save location for video/audio my Pixel 3a automatically points to the folder "Download/NewPipe" I created previously. However there is no "use this folder" option. I have to go back to "Download" and re-enter the "NewPipe" folder for the "use this folder" option to appear.
NewPipe behaves a bit weird when you clear storage access:

I tried it a few different times and for the first few tries, NewPipe just gave the error message "couldn't create folder" or something like that and I could fix it by going to the settings and re-registering the folder.

But right now it's behaving differently: I'm in a situation where NewPipe recognizes that storage access was cleared and it displays the toast "no download folder set yet, choose the default downloader folder now". It then opens to "use this folder" dialogue and downloads the file perfectly fine. The problem is: According to the Android App Info, NewPipe now has access to the folder I choose in this dialogue, but the same error toast and procedure keeps repeating every time I try to download something. So for some reason NewPipe doesn't recognize it now has storage access again.

Update: Even re-registering the folder in the options menu didn't fix this. I had to force close the app and now downloading works without an error message again.

Revert "Annotate methode parameters as NonNull" This reverts commit 004907d. Revert "Commit path immediately when import backup" This reverts commit 05eb0d0. Revert "Set ImportExportDataPath only on successful import" This reverts commit f13a1b0. Revert "Set ImportExportDataPath only on successful export" This reverts commit fd4408e. Revert "Invert if condition in ContentSettingsFragment.setImportExportDataPath for better readability" This reverts commit 92ab9ca. Revert "Move ContentSettingsFragment.isValidPath to helpers and add unit test for it." This reverts commit fa2b11b. Revert "Save backup import/export location for feature import/exports" This reverts commit 82f43ac. Remove FilePathHelperTest file

TeamNewPipe#6319 and TeamNewPipe#6402 were reverted before adding SAF changes, and have been readded at the end of SAF changes

Stypox · 2021-06-08T08:42:09Z

Rebased @TobiGr

Stypox · 2021-06-08T08:59:41Z

Finally :-D :-D
Thank you @Poolitzer and everyone else for the tests you did and thank you @wb9688 for the initial work :-)

Poolitzer · 2021-06-08T09:33:10Z

What a day to be alive

Stypox force-pushed the saf branch from b82ddb2 to bcd0d0c Compare January 14, 2021 20:44

TobiGr added the downloader Issue is related to the downloader label Jan 14, 2021

Stypox mentioned this pull request Jan 14, 2021

Extract settings import #5225

Merged

2 tasks

This comment has been minimized.

Sign in to view

Stypox force-pushed the saf branch from c6df8be to 743248a Compare January 15, 2021 07:56

Stypox mentioned this pull request Jan 15, 2021

Database export to removable storage fails on Android 10 #5198

Closed

4 tasks

XiangRongLin reviewed Jan 16, 2021

View reviewed changes

app/src/main/java/org/schabi/newpipe/streams/io/SharpInputStream.java Show resolved Hide resolved

XiangRongLin reviewed Jan 16, 2021

View reviewed changes

app/src/main/java/org/schabi/newpipe/settings/ContentSettingsManager.kt Show resolved Hide resolved

app/src/main/java/org/schabi/newpipe/util/ZipHelper.java Show resolved Hide resolved

Stypox force-pushed the saf branch from 743248a to f270ee5 Compare January 16, 2021 14:47

Stypox and others added 15 commits June 8, 2021 10:40

Support SAF properly

0f75024

Move Stored(File|Directory)Helper into NewPipe

1164ea5

Use file picker for export DB

cb4e615

Fix non-SAF actions

febb21a

Only ask for storage permissions when not using SAF

7efe62e

Add setting migration to promote using SAF

1e8b382

Fix ContentSettingsManager tests

6a0c5a8

Improve SAF switch descriptions in settings

bcb1cf6

Add comments to SharpStreams

eea43d5

Pass mime type so that SAF treats file extension correctly

114dc8f

Fix strange behaviour when app loses access to saf download folder

b78ac7d

Check if file really exists before asking user to overwrite it

21b8df0

Remove misplaced comment

5ffc667

Reimplement storing backup import/export path

2a99e0e

TeamNewPipe#6319 and TeamNewPipe#6402 were reverted before adding SAF changes, and have been readded at the end of SAF changes

Stypox dismissed TobiGr’s stale review via 2a99e0e June 8, 2021 08:41

Stypox force-pushed the saf branch from bed445e to 2a99e0e Compare June 8, 2021 08:41

TobiGr approved these changes Jun 8, 2021

View reviewed changes

TobiGr merged commit c96bdfc into TeamNewPipe:dev Jun 8, 2021

Stypox deleted the saf branch June 8, 2021 08:58

This was referenced Jun 26, 2021

Release 0.21.6 #6576

Merged

Release 0.21.6 #6577

Closed

throwaway242685 mentioned this pull request Aug 11, 2021

. fm-sys/snapdrop-android#126

Closed

litetex mentioned this pull request Sep 15, 2021

Not able to download the videos #7076

Closed

4 tasks

This was referenced May 2, 2022

About allowing X-plore - like file managers to be allowed to be chosen on subscription import #8268

Closed

Can't import newpipe subscriptions in Android TV #8195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SAF properly #5415

Support SAF properly #5415

Stypox commented Jan 14, 2021 •

edited

Loading

opusforlife2 commented Jan 14, 2021

opusforlife2 commented Jan 14, 2021

Stypox commented Jan 14, 2021

opusforlife2 commented Jan 14, 2021

Stypox commented Jan 14, 2021 •

edited

Loading

triallax commented Jan 14, 2021

fajis1 commented Jan 14, 2021

This comment has been minimized.

xibr commented Jan 14, 2021 •

edited

Loading

Stypox commented Jan 15, 2021 •

edited

Loading

mqus commented Jan 15, 2021 •

edited

Loading

triallax commented Jan 15, 2021

Stypox commented Jan 15, 2021

RickyM7 commented Jan 15, 2021 •

edited

Loading

Stypox commented Jan 15, 2021

RickyM7 commented Jan 15, 2021

RickyM7 commented Jan 15, 2021

Fs00 commented Jan 15, 2021 •

edited

Loading

Stypox commented Jan 15, 2021

opusforlife2 commented Jan 15, 2021

Poolitzer commented Jan 16, 2021

k9janer commented Jan 16, 2021 •

edited

Loading

Stypox commented Jun 8, 2021

Stypox commented Jun 8, 2021

Poolitzer commented Jun 8, 2021

Support SAF properly #5415

Support SAF properly #5415

Conversation

Stypox commented Jan 14, 2021 • edited Loading

What is it?

Description of the changes in your PR

TODO

Fixes the following issue(s)

Due diligence

opusforlife2 commented Jan 14, 2021

opusforlife2 commented Jan 14, 2021

Stypox commented Jan 14, 2021

opusforlife2 commented Jan 14, 2021

Stypox commented Jan 14, 2021 • edited Loading

triallax commented Jan 14, 2021

fajis1 commented Jan 14, 2021

This comment has been minimized.

xibr commented Jan 14, 2021 • edited Loading

Stypox commented Jan 15, 2021 • edited Loading

mqus commented Jan 15, 2021 • edited Loading

triallax commented Jan 15, 2021

Stypox commented Jan 15, 2021

RickyM7 commented Jan 15, 2021 • edited Loading

Stypox commented Jan 15, 2021

RickyM7 commented Jan 15, 2021

RickyM7 commented Jan 15, 2021

Fs00 commented Jan 15, 2021 • edited Loading

Stypox commented Jan 15, 2021

opusforlife2 commented Jan 15, 2021

Poolitzer commented Jan 16, 2021

k9janer commented Jan 16, 2021 • edited Loading

Stypox commented Jun 8, 2021

Stypox commented Jun 8, 2021

Poolitzer commented Jun 8, 2021

Stypox commented Jan 14, 2021 •

edited

Loading

Stypox commented Jan 14, 2021 •

edited

Loading

xibr commented Jan 14, 2021 •

edited

Loading

Stypox commented Jan 15, 2021 •

edited

Loading

mqus commented Jan 15, 2021 •

edited

Loading

RickyM7 commented Jan 15, 2021 •

edited

Loading

Fs00 commented Jan 15, 2021 •

edited

Loading

k9janer commented Jan 16, 2021 •

edited

Loading