-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Enable downloads from private sites #10610
Enable downloads from private sites #10610
Conversation
You can test the changes on this Pull Request by downloading the APK here. |
You can test the changes on this Pull Request by downloading the APK here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @planarvoid, I checked the PR and looks good! I just had the following issue I want to report for your evaluation.
I tested both with Private and Public site and it worked as described on my device (Xiaomi redmi note
5 - Android 9). I then tried with emulators and it worked fine with Pixel 3 API 28, but with Pixel 2 API 23 I have the following Exception (could be good if you can check if you can reproduce that or it is something with my emulator; let me know if I can make some additional check from my side 😊):
E/AndroidRuntime: FATAL EXCEPTION: main
Process: org.wordpress.android, PID: 5760
java.lang.RuntimeException: Error receiving broadcast Intent { act=android.intent.action.DOWNLOAD_COMPLETE flg=0x10 pkg=org.wordpress.android (has extras) } in org.wordpress.android.ui.reader.ReaderFileDownloadManager@1a78b0c
at android.app.LoadedApk$ReceiverDispatcher$Args.run(LoadedApk.java:891)
at android.os.Handler.handleCallback(Handler.java:739)
at android.os.Handler.dispatchMessage(Handler.java:95)
at android.os.Looper.loop(Looper.java:148)
at android.app.ActivityThread.main(ActivityThread.java:5417)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
Caused by: android.content.ActivityNotFoundException: No Activity found to handle Intent { act=android.intent.action.VIEW dat=content://org.wordpress.android.provider/external_files/Download/https:/myfirsttesthome.files.wordpress.com/2019/10/dummy-5.pdf typ=application/pdf flg=0x10000001 }
at android.app.Instrumentation.checkStartActivityResult(Instrumentation.java:1798)
at android.app.Instrumentation.execStartActivity(Instrumentation.java:1512)
at android.app.ContextImpl.startActivity(ContextImpl.java:677)
at android.app.ContextImpl.startActivity(ContextImpl.java:659)
at org.wordpress.android.ui.utils.DownloadManagerWrapper.openDownloadedAttachment(DownloadManagerWrapper.kt:66)
at org.wordpress.android.ui.reader.ReaderFileDownloadManager.openDownloadedAttachment(ReaderFileDownloadManager.kt:52)
at org.wordpress.android.ui.reader.ReaderFileDownloadManager.onReceive(ReaderFileDownloadManager.kt:22)
at android.app.LoadedApk$ReceiverDispatcher$Args.run(LoadedApk.java:881)
at android.os.Handler.handleCallback(Handler.java:739)
at android.os.Handler.dispatchMessage(Handler.java:95)
at android.os.Looper.loop(Looper.java:148)
at android.app.ActivityThread.main(ActivityThread.java:5417)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
Just one small note on a different side (not related with the above). Not fully sure if it can apply but while looking into the code I was also wondering if it could make sense to make the DownloadManagerWrapper
and the AuthenticationUtils
a @Singleton
or even @Reusable
. In any case it is not necessary and also the usage is not wide to make a difference I guess.
@develric thanks for the check. The crash is caused by the emulator not having a browser. I've caught the exception (we don't do anything when this happens as the user can still open the downloaded file and this is a very rare case). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @planarvoid ! LGTM 👍!
Fixes #8208
The main step that in this PR is reusing the logic we use in Glide library to load images on private sites. I've extracted this logic in
authenticationUtils.getAuthHeaders(url)
. This function adds authentication headers for private requests for both image loading and file download. As an additional step I've added logic to open the file after it's downloaded (otherwise it can be confusing as nothing happens when you click a file - in Android 10 you don't even get a notification).To test:
PR submission checklist:
I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes and have added them to
RELEASE-NOTES.txt
if necessary.