Skip to content

Suppress download dialog if user already confirmed download with a different dialog #766

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Apr 10, 2020
Merged

Suppress download dialog if user already confirmed download with a different dialog #766

merged 11 commits into from
Apr 10, 2020

Conversation

@subsymbolic
Copy link
Contributor

@subsymbolic subsymbolic commented Apr 6, 2020

Task/Issue URL: https://app.asana.com/0/414730916066338/1170085036962994

Description:
PR #663 added a new download confirmation dialog (currently merged into a feature branch). This PR suppresses that dialog in cases where the user has already interacted with a dialog e.g accepted the Android system permissions dialog or long pressed and selected "download image".

Steps to test this PR:

  1. Perform a fresh installation of the app
  2. Go to the serp and search for "test"
  3. Tap on "images"
  4. Long press an image and select "download image"
  5. Note that you are shown the system permissions dialog
  6. Grant permission and note you are not shown the download confirmation dialog
  7. Long press another image and select "download image" again
  8. Note you are not shown the download confirmation dialog
  9. Go to https://github.com/duckduckgo/Android/releases and download an apk
  10. Note you are shown the download confirmation dialog this time
Internal references:

Software Engineering Expectations
Technical Design Template

@subsymbolic subsymbolic changed the title Suppress download dialog if other a dialog already seen Suppress download dialog if another dialog already seen Apr 6, 2020
@subsymbolic subsymbolic changed the title Suppress download dialog if another dialog already seen Suppress download dialog if user already confirmed download with a different dialog Apr 6, 2020
@cmonfortep cmonfortep self-assigned this Apr 9, 2020
cmonfortep
cmonfortep reviewed Apr 9, 2020
View reviewed changes
Copy link
Contributor

@cmonfortep cmonfortep left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I would like to double check if the following described scenario is the expected one:

  1. Fresh install
  2. Go to https://github.com/duckduckgo/Android/releases and download an apk
  3. Permissions dialog is shown
  4. Download starts and download dialog did not appear

I will continue the review, as I think the behavior is correct.

Edit: While reviewing I see that this is intentional. All good.

cmonfortep
cmonfortep reviewed Apr 9, 2020
View reviewed changes
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.

Nice cleanup and I like the how the user flow is now.
Just few comments, everything looks good but I have a question related to some changes we introduced.

@subsymbolic subsymbolic requested a review from cmonfortep April 9, 2020 23:24
cmonfortep
cmonfortep reviewed Apr 10, 2020
View reviewed changes
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.

Very good work, and nice clean up of things that were not related to this PR 🤘

Just a question related to the dialog dismiss, and some optional changes to simplify some parts of the code. But so far it looks great.

app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
override fun onPause() {
daxDialog = null
logoHidingListener.onPause()
dismissDownloadFragment()
Copy link
Contributor

@cmonfortep cmonfortep Apr 10, 2020

Choose a reason for hiding this comment

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

Not sure if we need this dismiss here. The dialog will be dismissed automatically when activity is destroyed, and automatically shown when recreated. You are already covering that case setting again the listener.

Keeping the line here, dialog will be dismissed if user sends the app to background or screen turns off (Maybe that's the expected behavior).
Removing the line keeps the dialog and also work on recreation. Thoughts?

Copy link
Contributor Author

@subsymbolic subsymbolic Apr 10, 2020

Choose a reason for hiding this comment

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

I made the rest of the code safe in terms of fragment recreation so we definitely don't need this.
I went with removing it for usability reasons, I was testing my changes and found it awkward to leave the app and return to it later with the dialog still up. The dialog feels like a decision the user should make immediately and I thinks it's a better experience if the dialog is dismissed if the user leaves the screen. It's an edge case that probably won't impact many users so that's not a strong opinion on my side. If you feel really strongly about keeping it, I'm happy to go with that.

Copy link
Contributor

@cmonfortep cmonfortep Apr 10, 2020

Choose a reason for hiding this comment

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

I agree, it's an edge case and worst case scenario user only needs to click again on the link, like you said. I'm good keeping it like this.

@subsymbolic subsymbolic requested a review from cmonfortep April 10, 2020 11:50
cmonfortep
cmonfortep approved these changes Apr 10, 2020
View reviewed changes
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, nice work here. Also thank you for all the extra changes to improve parts not related to this PR.

@subsymbolic
Merge branch 'feature/multiple_authors/download_confirmation' into fe…
0225596 
…ature/mia/suppress_dialog_if_other_dialogs_seen
@subsymbolic subsymbolic merged commit 75092ae into feature/multiple_authors/download_confirmation Apr 10, 2020
@subsymbolic subsymbolic deleted the feature/mia/suppress_dialog_if_other_dialogs_seen branch April 10, 2020 15:40
subsymbolic added a commit that referenced this pull request Apr 10, 2020
@subsymbolic
Suppress download dialog if user already confirmed download with a di…
b5b1341 
…fferent dialog (#766)
@subsymbolic subsymbolic mentioned this pull request Apr 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cmonfortep cmonfortep cmonfortep approved these changes

@CDRussell CDRussell Awaiting requested review from CDRussell

@malmstein malmstein Awaiting requested review from malmstein

@marcosholgado marcosholgado Awaiting requested review from marcosholgado

Assignees

@cmonfortep cmonfortep

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@subsymbolic @cmonfortep