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

Fix free storage space check for all APIs #10992

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Apr 23, 2024

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

The previous code for obtaining free storage space was broken:

  • It had some logical flaws in the checks being performed (e.g. why an SDK_INT check was before the function call, while another was inside it?)
  • It didn't work at all for sdcard or other storages that don't have an UUID (don't ask me why the sdcard on the emulator did not have an UUID, who knows).
  • It crashed with non-SAF files, see Release v0.27.0 (please TEST!) #10930 (comment)

This PR checks for free storage according to https://stackoverflow.com/q/31171838. It uses Os.statvfs(filePath) on non-SAF files, and Os.fstatvfs(fileDescriptor) on SAF files. Note that for SAF files a file descriptor needs to be opened in read mode, but I checked and this does not seem to create a file, and works when the file does not exist (which is basically always the case since it's the file we want to download). Also see https://pubs.opengroup.org/onlinepubs/9699919799/functions/fstatvfs.html.

I tested on API 22 emulated (both SAF and non-SAF), API 33 emulated (SAF), API 33 device (SAF) and checked the actual values returned by getFreeStorageSpace() both on sdcard and internal storage, and everything was correct.

Fixes the following issue(s)

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.

Due diligence

@github-actions github-actions bot added the size/medium PRs with less than 250 changed lines label Apr 23, 2024
@somos61
Copy link

somos61 commented Apr 23, 2024

Tested on Android 7.0 and 7.1.2, works flawless now, thanks!
But for Android 9 have a crash downloading with SAF turned on. Dont know if it matters, but dwnl string in settings looks like 'content://com.android.providers.downloads.documents/tree/downloads' now
When SAF is off, downloads fine yet (for A9)

Exception

  • User Action: ui error
  • Request: ACRA report
  • Content Country: US
  • Content Language: en-US
  • App Language: en_US
  • Service: none
  • Version: 0.27.0
  • OS: Linux Android 9 - 28
Crash log

java.lang.IllegalArgumentException: For input string: "downloads"
	at android.database.DatabaseUtils.readExceptionFromParcel(DatabaseUtils.java:165)
	at android.database.DatabaseUtils.readExceptionWithFileNotFoundExceptionFromParcel(DatabaseUtils.java:146)
	at android.content.ContentProviderProxy.openTypedAssetFile(ContentProviderNative.java:698)
	at android.content.ContentResolver.openTypedAssetFileDescriptor(ContentResolver.java:1458)
	at android.content.ContentResolver.openAssetFileDescriptor(ContentResolver.java:1295)
	at android.content.ContentResolver.openFileDescriptor(ContentResolver.java:1148)
	at android.content.ContentResolver.openFileDescriptor(ContentResolver.java:1102)
	at org.schabi.newpipe.streams.io.StoredDirectoryHelper.getFreeStorageSpace(StoredDirectoryHelper.java:199)
	at org.schabi.newpipe.download.DownloadDialog.prepareSelectedDownload(DownloadDialog.java:863)
	at org.schabi.newpipe.download.DownloadDialog.lambda$initToolbar$1(DownloadDialog.java:353)
	at org.schabi.newpipe.download.DownloadDialog.$r8$lambda$eftfC3lumhpNMhhwp_4dKbyeS0U(Unknown Source:0)
	at org.schabi.newpipe.download.DownloadDialog$$ExternalSyntheticLambda9.onMenuItemClick(Unknown Source:2)
	at androidx.appcompat.widget.Toolbar$1.onMenuItemClick(Toolbar.java:225)
	at androidx.appcompat.widget.ActionMenuView$MenuBuilderCallback.onMenuItemSelected(ActionMenuView.java:781)
	at androidx.appcompat.view.menu.MenuBuilder.dispatchMenuItemSelected(MenuBuilder.java:836)
	at androidx.appcompat.view.menu.MenuItemImpl.invoke(MenuItemImpl.java:159)
	at androidx.appcompat.view.menu.MenuBuilder.performItemAction(MenuBuilder.java:987)
	at androidx.appcompat.view.menu.MenuBuilder.performItemAction(MenuBuilder.java:977)
	at androidx.appcompat.widget.ActionMenuView.invokeItem(ActionMenuView.java:625)
	at androidx.appcompat.view.menu.ActionMenuItemView.onClick(ActionMenuItemView.java:156)
	at android.view.View.performClick(View.java:6597)
	at android.view.View.performClickInternal(View.java:6574)
	at android.view.View.access$3100(View.java:778)
	at android.view.View$PerformClick.run(View.java:25885)
	at android.os.Handler.handleCallback(Handler.java:873)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:193)
	at android.app.ActivityThread.main(ActivityThread.java:6669)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)


@TobiGr TobiGr added the downloader Issue is related to the downloader label Apr 23, 2024
It's not a critical check that needs to be perfomed, so in case something does not work on some device/version, let's just ignore the error.
@Stypox
Copy link
Member Author

Stypox commented Apr 23, 2024

@somos61 thank you for testing, I have no idea why docTree.getUri() would return "downloads", but now I added a catch that ignores all errors.

@Stypox Stypox merged commit d61b4b8 into TeamNewPipe:release-0.27.0 Apr 23, 2024
3 of 5 checks passed
Copy link

sonarcloud bot commented Apr 23, 2024

@somos61
Copy link

somos61 commented Apr 23, 2024

@somos61 thank you for testing, I have no idea why docTree.getUri() would return "downloads", but now I added a catch that ignores all errors.

dunno why it was 'downloads' instead of 'Download' either, but now dwnl path for SAF is 'content://com.android.providers.downloads.documents/tree/raw:/storage/emulated/0/Download' and downloads ok, for all tested. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
downloader Issue is related to the downloader size/medium PRs with less than 250 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants