Skip to content
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

Support for camera cutouts #2264

Closed
wants to merge 9 commits into from

Conversation

Projects
None yet
2 participants
@fercarcedo
Copy link
Contributor

commented Mar 8, 2018

This PR fixes content overlapping with the status bar on devices with a camera cutout.

The fix consists of replacing the hardcoded 25dp values with the actual status bar height via the WindowInsets API.

Below you can find screenshots of before and after the change was made (taken from an Android P emulator thanks to P's notch simulation feature).

Before:
focus_cameracutout_before_1
focus_cameracutout_before_2
focus_cameracutout_before_3
focus_cameracutout_before_4

After:
focus_cameracuout_after_1
focus_cameracutout_after_2
focus_cameracutout_after_3
focus_cameracutout_after_4

#2047

@pocmo pocmo self-requested a review Mar 8, 2018

@pocmo pocmo added the needs review label Mar 8, 2018

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2018

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2018

I noticed two things while testing the patch:

  • The search hint seems to depend on the height too and is off now:

search-hint

  • When clicking on the URL to edit it, the input field is at the top first and then moves to the correct position with a delay. Is this something we can change? Maybe we need to delay the animation here until we know the height?
@pocmo

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2018

Maybe we need to delay the animation here until we know the height?

Or pass the height to the other fragment (like we do with other coordinates for the animation)

@pocmo
Copy link
Contributor

left a comment

See above

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2018

ktlint found two issues:

/opt/focus-android/app/src/main/java/org/mozilla/focus/fragment/UrlInputFragment.kt:186:88: Unnecessary semicolon
/opt/focus-android/app/src/main/java/org/mozilla/focus/fragment/UrlInputFragment.kt:187:19: Unnecessary semicolon

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2018

Other than that this looks pretty sweet! Thank you so far! :)

@fercarcedo

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2018

I have solved both problems. For the second one (input field first at the top and then moves to the correct position) I have created a new class StatusBarUtils which caches the status bar height so that it isn't necessary to wait for the listener every time we need that value

@mozilla-mobile mozilla-mobile deleted a comment from fercarcedo Mar 8, 2018

@pocmo

pocmo approved these changes Mar 8, 2018

/**
* Created by Fer on 08/03/2018.
*/

This comment has been minimized.

Copy link
@pocmo

pocmo Mar 8, 2018

Contributor

Please remove this auto-generated comment :)

This comment has been minimized.

Copy link
@fercarcedo

fercarcedo Mar 8, 2018

Author Contributor

Done

@@ -0,0 +1,31 @@
package org.mozilla.focus.utils;

This comment has been minimized.

Copy link
@pocmo

pocmo Mar 8, 2018

Contributor

Please add our license header at the top of the file:

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

This comment has been minimized.

Copy link
@fercarcedo

fercarcedo Mar 8, 2018

Author Contributor

Done

public WindowInsets onApplyWindowInsets(View v, WindowInsets insets) {
STATUS_BAR_SIZE = insets.getSystemWindowInsetTop();
listener.onStatusBarHeightFetched(STATUS_BAR_SIZE);
return insets;

This comment has been minimized.

Copy link
@pocmo

pocmo Mar 8, 2018

Contributor

Is it required to remove the listener here again?

This comment has been minimized.

Copy link
@fercarcedo

fercarcedo Mar 8, 2018

Author Contributor

I'm not sure if I'm understanding the question correctly, what this code does is if we don't have the status bar height cached it registers a listener and when it gets the status bar height saves it for later usage and sends it back to the caller.

This comment has been minimized.

Copy link
@pocmo

pocmo Mar 9, 2018

Contributor

What I mean is: Should we call view.setOnApplyWindowInsetsListener(null) here once we got a value or do we need to stay subscribed?

This comment has been minimized.

Copy link
@pocmo

pocmo Mar 9, 2018

Contributor

I was thinking of things like OnPreDrawListener where you explicitly clear the listener after receiving what you want. To avoid keeping strong references in memory.

This comment has been minimized.

Copy link
@fercarcedo

fercarcedo Mar 9, 2018

Author Contributor

Yes, after all it's no longer needed, so why keep references to the listener (and thus to the Activity). Added on latest commit

fercarcedo added some commits Mar 8, 2018

@pocmo

pocmo approved these changes Mar 9, 2018

Copy link
Contributor

left a comment

This looks good! I'll give it a last try and land this.

@pocmo pocmo added needs merge and removed needs review labels Mar 9, 2018

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2018

Thank you @fercarcedo! I landed this as a squashed commit: ba97314

@pocmo pocmo closed this Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.