Skip to content

Conversation

@CDRussell
Copy link
Member

Task/Issue URL: https://app.asana.com/0/488551667048375/1187803392412385/f
Tech Design URL:
CC:

Description:
We saw crash instances when a user attempts to save a data URI. This PR fixes that, and has a few related improvements to that area

  • Simplifies the DownloadConfirmationFragment

    • Previously it was responsible for asking what the user wanted to do, as well as handling the response and trying to delete files, trigger the download etc...
    • Now it is just responsible for passing the user's choice back to the parent fragment
    • Removed a leak with this dialog
  • Limits the title length for the long press menu. An exceedingly long data URI could trigger an ANR as we were trying to populate a TextView with potentially megabytes of data (and the UI already capped what it could output)

Steps to reproduce crash

  1. Use an emulator or a device with a large URI data file.
  2. In the developer settings, enable "Don't keep activities".
  3. Go https://dopiaza.org/tools/datauri/examples/index.php
  4. Long click on the image and save image. As soon as you save the image, move the app to the background.
  5. Crash should show in the logcat. (java.lang.NullPointerException)

Steps to test this PR:

  1. Repeat the above steps on this branch; verify no crash happens and the data uri image is saved ok
  2. Try other downloads; verify they work as expected (e.g., https://file-examples.com/)
  3. Test blank tabs for new downloads are still closed; follow steps in Issue #753 - Downloads that open in a new tab leave user on an empty tab #883

Internal references:

Software Engineering Expectations
Technical Design Template

This also tidies up some of the code around the DownloadConfirmationFragment, which had a leak and was doing a fair bit; moving some of the logic out of here to keep this dialog focussed on obtaining the user choice makes things simpler.
@CDRussell CDRussell requested a review from cmonfortep August 7, 2020 08:33
@cmonfortep cmonfortep self-assigned this Aug 7, 2020

override fun handleLongPress(longPressTargetType: Int, longPressTargetUrl: String?, menu: ContextMenu) {
menu.setHeaderTitle(longPressTargetUrl ?: context.getString(R.string.options))
menu.setHeaderTitle(longPressTargetUrl?.take(MAX_TITLE_LENGTH) ?: context.getString(R.string.options))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM All works as described. Good work fixing this issue, thanks!

@CDRussell CDRussell merged commit 7bf0ba1 into develop Aug 10, 2020
@CDRussell CDRussell deleted the feature/craig/fix_download_crash branch August 10, 2020 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants