For #18727: Exit PiP when launched externally with Intents #20486

czlucius · 2021-07-23T17:13:59Z

Linked issue: #18727

Adds exit PiP code in HomeFragment, exiting PiP on onCreate.

To check:

App doesn't exit PiP when in PiP and screen rotates
Check that intent in HomeActivity isn't null before exiting

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

Video of the fixed issue:

Screen_Recording_20210724-112718_Firefox.Preview.mp4

firefoxci-taskcluster · 2021-07-23T17:14:17Z

No Taskcluster jobs started for this pull request

The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

czlucius · 2021-07-24T04:26:40Z

Screen_Recording_20210724-112718_Firefox.Preview.mp4

Here's the video of the fixed issue. Tested on Android 11 on Samsung P610 (Tab S6 Lite).
I'll need to investigate into the possible side effects first.

Mugurell · 2021-08-06T09:42:59Z

To me this looks good. We obtain the desired effect and seems the generally accepted solution in place of an official Android api for this.
Curious if @mcomella agrees with the this approach from a performance standpoint.

mcomella

Curious if @mcomella agrees with the this approach from a performance standpoint.

Thanks for looping me in. From a perf perspective, I was concerned about HomeActivity.onCreate getting called twice but that doesn't seem to be the case. Subjectively, I tested on a Moto G5 and it doesn't seem unreasonable. It'd take too long for me to objectively benchmark the change because it doesn't have a clear start and ending point so I'll say this seems reasonable from a perf perspective.

From a correctness perspective, I'm wary about triggering the android lifecycle unnecessarily. It's difficult to code correctly around the common lifecycle patterns but the Android framework does unexpected things when we're intentionally recreating activities to reset state. For example, we use activity.recreate() to switch between normal and private browsing and it calls unexpected code paths such as HomeFragment.onCreateView being called before HomeActivity.onCreate even inflates the layout containing HomeFragment (because Android is automatically restoring the fragment stack without us having to inflate and define the state).

That being said, I don't know what moveTaskToBack(...); startActivity(... this) does specifically, it may be simpler than the activity.recreate case, and it may be the right solution for the problem here. I didn't look at this carefully from the correctness perspective but I just wanted to mention my general preference for finding solutions that don't trigger the lifecycle rather than ones that do. So I'm not blocking the review on this – thanks for the patch!

mcomella · 2021-08-06T21:18:52Z

app/src/main/java/org/mozilla/fenix/HomeActivity.kt

+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N && isInPictureInPictureMode) {
+            // Exit PiP mode
+            moveTaskToBack(false)
+            startActivity(Intent(this, this::class.java).addFlags(Intent.FLAG_ACTIVITY_NEW_TASK))


issue (non-blocking): I'm not sure how results in us exiting PiP mode.

suggestion: can you explain in the comment how this lets us exit PiP mode?

https://stackoverflow.com/a/43288507/12204281
The code moves the activity to the back of the activity stack, then restores it to the front, bringing it out of PiP mode.

I'm not sure I understand why moving the activity to the back of the activity stack and restoring it to the front would bring it out of PiP – it seems like it might be a hack. However, if it seems like this is the only way to do it, I'm fine with that – just link to the Stackoverflow post in a comment so others have context on where this solution came from.

In any case, I don't need to approve or reject the correctness changes: I'll defer to @Mugurell for those. @Mugurell Please finish reviewing – it looks good from my perspective on performance! Sorry if I was unclear about my intent.

Mugurell · 2021-08-11T10:46:58Z

Remembered we were doing something similar on Fennec also.
Speaking of which, @czlucius can we change to use FLAG_ACTIVITY_REORDER_TO_FRONT as the flag for the startActivity Intent? Seems more appropriate than FLAG_ACTIVITY_NEW_TASK.
Then we can give this a shot in the new 93 train.

czlucius · 2021-08-13T10:23:20Z

Sorry, I forgot to add the import statement before.

czlucius · 2021-08-13T10:43:00Z

I've tested the fix with the new flag, it seems to work fine.

Mugurell · 2021-08-16T06:23:20Z

@Mergifyio refresh

mergify · 2021-08-16T06:23:53Z

Command refresh: success

Pull request refreshed

Mugurell · 2021-08-16T06:33:05Z

@czlucius It seems there is an issue with the script we're using for landing PRs and right now this PR is also blocking others from landing so I'll close this just to let the others pass.
Can you please squash your work and rebase on the latest main to then try to merge it again?

czlucius · 2021-08-16T10:16:16Z

Ok, I’ll try. Do you mean squash my commits into 1 commit then rebase into the main branch?

czlucius · 2021-08-16T10:47:41Z

I'll try soon, but lately I've been quite busy, will do soon when I'm free
Thanks

Mugurell · 2021-08-16T13:58:13Z

Ok, I’ll try. Do you mean squash my commits into 1 commit then rebase into the main branch?

Yes. Thank you!

czlucius · 2021-08-30T10:40:32Z

Ok.

czlucius

I hope the indentation is fine

Mugurell · 2021-08-31T11:20:06Z

@czlucius The indentation seems fine but detekt is failing because the onCreate method is not too complex - with the if block added by this patch there are too many branches.
The easiest (and probably the best here) solution is to just add the @Suppress("ComplexMethod") annotation to onCreate.

You can also run detekt and ktlint locally with
./gradlew detekt && ./gradlew ktlint to check everything is ok.

czlucius · 2021-08-31T12:00:44Z

Ok, I will make these changes.
On a side note, would it be ok to extract the code exiting PiP into a function to reduce ˋonCreateˋ's complexity?

Thanks.

Mugurell · 2021-08-31T13:26:21Z

On a side note, would it be ok to extract the code exiting PiP into a function to reduce ˋonCreateˋ's complexity?

Having that code in onCreate makes it easier to follow but if the new method has an enough of a suggestive name that would work also 👍.
Maybe something like checkAndExitPip?

czlucius · 2021-08-31T13:41:09Z

I've made the relevant changes, if dekekt still shows errors, I will replace this with the @Suppress solution.

Mugurell · 2021-08-31T13:59:19Z

@czlucius Sorry for this being a drag but now it seems the ktlint task is failing because of

/home/runner/work/fenix/fenix/app/src/main/java/org/mozilla/fenix/HomeActivity.kt:273:1: Needless blank line(s)
/home/runner/work/fenix/fenix/app/src/main/java/org/mozilla/fenix/HomeActivity.kt:276:1: Unexpected indentation (9) (should be 8)

czlucius · 2021-08-31T15:14:56Z

ktlint seems to be ok with this locally.

Mugurell · 2021-08-31T15:43:59Z

ktlint seems to be ok with this locally.

That warning can be ignored for now.
But I see in the CI tasks ktlint failing because of

/home/runner/work/fenix/fenix/app/src/main/java/org/mozilla/fenix/HomeActivity.kt:273:1: Needless blank line(s)

With this also fixed we can look into merging the patch.

czlucius · 2021-09-01T04:02:38Z

ktlint is ok, but now a test is failing (hence test-debug fails)

Mugurell · 2021-09-01T12:08:42Z

With everything green I think we can merge this.

czlucius · 2021-09-01T16:25:23Z

@Mugurell may I ask, if in PiP, the activity still undergoes lifecycle changes? I did not see anything anywhere on the Android Developer website, nor on Stackoverflow that suggests this, but just to be sure.
Thanks.

Mugurell · 2021-09-02T10:59:06Z

@Mugurell may I ask, if in PiP, the activity still undergoes lifecycle changes? I did not see anything anywhere on the Android Developer website, nor on Stackoverflow that suggests this, but just to be sure.
Thanks.

Don't think there is any change for the activity while in PiP though?
Nothing changes about it. Still in foreground but in the onPause state Android puts it in for the entire duration of PiP.
From here it would either get onResume if returning to the app or onStop if PiP and the activity is closed.

Mugurell · 2021-09-02T11:28:24Z

#20831 merged these changes so I'll close the PR.
Thank you @czlucius !

czlucius · 2021-09-02T13:47:26Z

Thank you @czlucius !

Welcome.

Thank you for merging the fix.

czlucius · 2021-09-02T13:47:32Z

Don't think there is any change for the activity while in PiP though?
Nothing changes about it. Still in foreground but in the onPause state Android puts it in for the entire duration of PiP.
From here it would either get onResume if returning to the app or onStop if PiP and the activity is closed.

Noted.

czlucius requested review from a team as code owners July 23, 2021 17:14

czlucius marked this pull request as draft July 23, 2021 17:14

czlucius changed the title ~~Exit PiP in HomeActivity after onCreate, addressing #18727~~ For #18727 Jul 23, 2021

czlucius changed the title ~~For #18727~~ For #18727: Exit PiP when launched externally with Intents Jul 23, 2021

Mugurell requested review from mcomella and Mugurell August 6, 2021 09:37

mcomella reviewed Aug 6, 2021

View reviewed changes

czlucius marked this pull request as ready for review August 8, 2021 11:22

Mugurell previously approved these changes Aug 13, 2021

View reviewed changes

Mugurell mentioned this pull request Aug 13, 2021

CI for contributor PR - 20486 #20831

Merged

Mugurell force-pushed the fix-issue-18727 branch from ee051e3 to 31fac79 Compare August 13, 2021 10:16

Mugurell added the pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify] label Aug 13, 2021

Mugurell closed this Aug 16, 2021

Mugurell reopened this Aug 16, 2021

mergify bot dismissed Mugurell’s stale review via 5e70c2e August 19, 2021 14:04

czlucius force-pushed the fix-issue-18727 branch from 5e70c2e to d6d9378 Compare August 20, 2021 07:34

czlucius requested a review from a team as a code owner August 20, 2021 07:34

czlucius force-pushed the fix-issue-18727 branch from 003aee5 to 7f159cb Compare August 31, 2021 10:30

czlucius commented Aug 31, 2021

View reviewed changes

czlucius force-pushed the fix-issue-18727 branch from 7f159cb to 1996f67 Compare August 31, 2021 13:38

czlucius force-pushed the fix-issue-18727 branch from 1996f67 to 6ab633c Compare August 31, 2021 14:04

czlucius force-pushed the fix-issue-18727 branch 2 times, most recently from 0ee8b18 to 56610ec Compare August 31, 2021 15:25

For mozilla-mobile#18727: Exit PiP when launched externally with Intents

75acc52

czlucius force-pushed the fix-issue-18727 branch from 56610ec to 75acc52 Compare August 31, 2021 16:10

Mugurell approved these changes Sep 1, 2021

View reviewed changes

Merge branch 'main' into fix-issue-18727

64ef0ec

mergify bot added 2 commits September 1, 2021 20:29

Merge branch 'main' into fix-issue-18727

7c7c1c8

Merge branch 'main' into fix-issue-18727

5093d5e

pocmo removed the pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify] label Sep 2, 2021

Mugurell closed this Sep 2, 2021

czlucius mentioned this pull request Nov 17, 2021

[Bug]: Orientation lock after launching from external intents from PiP #22471

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #18727: Exit PiP when launched externally with Intents #20486

For #18727: Exit PiP when launched externally with Intents #20486

czlucius commented Jul 23, 2021 •

edited

Loading

firefoxci-taskcluster bot commented Jul 23, 2021

czlucius commented Jul 24, 2021

Mugurell commented Aug 6, 2021

mcomella left a comment •

edited

Loading

mcomella Aug 6, 2021

czlucius Aug 8, 2021

mcomella Aug 11, 2021

Mugurell commented Aug 11, 2021 •

edited

Loading

czlucius commented Aug 13, 2021

czlucius commented Aug 13, 2021

Mugurell commented Aug 16, 2021

mergify bot commented Aug 16, 2021

Mugurell commented Aug 16, 2021

czlucius commented Aug 16, 2021 •

edited

Loading

czlucius commented Aug 16, 2021 •

edited

Loading

Mugurell commented Aug 16, 2021

czlucius commented Aug 30, 2021

czlucius left a comment

Mugurell commented Aug 31, 2021

czlucius commented Aug 31, 2021

Mugurell commented Aug 31, 2021

czlucius commented Aug 31, 2021 •

edited

Loading

Mugurell commented Aug 31, 2021

czlucius commented Aug 31, 2021

Mugurell commented Aug 31, 2021

czlucius commented Sep 1, 2021

Mugurell commented Sep 1, 2021

czlucius commented Sep 1, 2021

Mugurell commented Sep 2, 2021 •

edited

Loading

Mugurell commented Sep 2, 2021

czlucius commented Sep 2, 2021

czlucius commented Sep 2, 2021

For #18727: Exit PiP when launched externally with Intents #20486

For #18727: Exit PiP when launched externally with Intents #20486

Conversation

czlucius commented Jul 23, 2021 • edited Loading

Pull Request checklist

To download an APK when reviewing a PR:

firefoxci-taskcluster bot commented Jul 23, 2021

czlucius commented Jul 24, 2021

Mugurell commented Aug 6, 2021

mcomella left a comment • edited Loading

Choose a reason for hiding this comment

mcomella Aug 6, 2021

Choose a reason for hiding this comment

czlucius Aug 8, 2021

Choose a reason for hiding this comment

mcomella Aug 11, 2021

Choose a reason for hiding this comment

Mugurell commented Aug 11, 2021 • edited Loading

czlucius commented Aug 13, 2021

czlucius commented Aug 13, 2021

Mugurell commented Aug 16, 2021

mergify bot commented Aug 16, 2021

Mugurell commented Aug 16, 2021

czlucius commented Aug 16, 2021 • edited Loading

czlucius commented Aug 16, 2021 • edited Loading

Mugurell commented Aug 16, 2021

czlucius commented Aug 30, 2021

czlucius left a comment

Choose a reason for hiding this comment

Mugurell commented Aug 31, 2021

czlucius commented Aug 31, 2021

Mugurell commented Aug 31, 2021

czlucius commented Aug 31, 2021 • edited Loading

Mugurell commented Aug 31, 2021

czlucius commented Aug 31, 2021

Mugurell commented Aug 31, 2021

czlucius commented Sep 1, 2021

Mugurell commented Sep 1, 2021

czlucius commented Sep 1, 2021

Mugurell commented Sep 2, 2021 • edited Loading

Mugurell commented Sep 2, 2021

czlucius commented Sep 2, 2021

czlucius commented Sep 2, 2021

czlucius commented Jul 23, 2021 •

edited

Loading

mcomella left a comment •

edited

Loading

Mugurell commented Aug 11, 2021 •

edited

Loading

czlucius commented Aug 16, 2021 •

edited

Loading

czlucius commented Aug 16, 2021 •

edited

Loading

czlucius commented Aug 31, 2021 •

edited

Loading

Mugurell commented Sep 2, 2021 •

edited

Loading