Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #4438: Fix crash when opening any URL from URL bar w/ long press #4547

Merged
merged 5 commits into from
Aug 7, 2019

Conversation

jyeontaek
Copy link
Contributor

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

@jyeontaek jyeontaek requested a review from a team as a code owner August 5, 2019 22:04
Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! Did you verify this also fixes the crashes described in #4486 (they have the same cause)

Can we also add a comment to onNewIntent why we're calling popBackStack there? It's not obvious to me :).

@jyeontaek
Copy link
Contributor Author

jyeontaek commented Aug 6, 2019

I fixed the issue in a different way, so we don't need popBackStack after all! And yes, this fixes the crashes in #4486.

EDIT: Actually, this new solution does not work for some other case I found.

@jyeontaek
Copy link
Contributor Author

I cleared both flags because FLAG_ACTIVITY_CLEAR_TASK was causing the issue, and since HomeActivity is SingleTask anyway, FLAG_ACTIVITY_NEW_TASK seemed redundant.

These flags were used to fix the "brief flash" problem in #3131, but even after removal, this problem didn't show up in several of my test devices.

@jyeontaek jyeontaek requested a review from csadilek August 6, 2019 23:32
Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

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

Change looks good to me!

I confirmed that it fixes the linked issues. I'd like to defer to @ekager and @sblatz though to make call re: custom tabs esp. #3131

@csadilek csadilek requested review from ekager and sblatz August 7, 2019 15:51
@ekager
Copy link
Contributor

ekager commented Aug 7, 2019

Haha sorry I didn't realize when we chatted yesterday that I had added those flags 🙃 I will test this locally to check re #3131 today!

Copy link
Contributor

@ekager ekager left a comment

Choose a reason for hiding this comment

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

Yep this looks good!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants