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
[TIMOB-26246] Android: Added Ti.UI.Window "extendSafeArea", "safeAreaPadding", and inset/notch support #10383
Conversation
…Padding", and inset/notch support - [TIMOB-26246] Added handling of Android P inset/notch/cutout. * Added Ti.UI.Window property "extendSafeArea" to render content beneath insets. - [TIMOB-26427] Added Ti.UI.Window property "safeAreaPadding". * Applies to Android 4.4 and newer OS versions when "extendSafeArea" is true. * Needed to detect size of insets for translucent StatusBar/NavBar and Android P notches. - [TIMOB-26459] Added constants for Ti.UI.Window property "windowFlags". * Ti.UI.Android.FLAG_TRANSLUCENT_NAVIGATION * Ti.UI.Android.FLAG_TRANSLUCENT_STATUS - [TIMOB-25810] Fixed bug where ActionBar height won't resize upon orientation changes. - [TIMOB-26460] Fixed bug where Toolbar height set to Ti.UI.SIZE won't resize upon orientation change. - [TIMOB-26442] Fixed bug where Toolbar with "extendBackground" to true is too tall if both status bar and nav bar are translucent. - Modified DrawerLayout XML to not fit its "center" view under top insets. * The XML setting was wrong. It was intended to place left/right drawers under top inset, but it wasn't doing that. * Better solution is to allow left/center/right layouts to overlap insets and use a Toolbar with "extendBackground" true to position itself beneath inset.
@@ -39,6 +41,9 @@ | |||
import android.util.Pair; | |||
import android.view.Display; | |||
import android.view.View; | |||
import android.view.ViewParent; | |||
import android.view.Window; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import.
} | ||
|
||
/** | ||
* To be called by the owner when the activity's overriden onConfigurationChanged() method has been called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in "overridden".
/** | ||
* To be called by the owner when the activity's overriden onConfigurationChanged() method has been called. | ||
* Updates the ActionBar's height and font size base on the given configuration. | ||
* @param newConfig The udpated configuration applied to the activity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in "updated".
toolbarStyleHandler = new TiToolbarStyleHandler((Toolbar) view); | ||
} | ||
} catch (Exception ex) { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe logging a warning or a debug level message here ? To make it easier for troubleshooting at some point if a problem comes from failing this try block. ( I am not a fan of empty catch blocks )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Done.
|
||
// Calculate the safe-area, which is the region between the insets. | ||
// Note: We must dynamically add ActionBar height here (if enabled) since its | ||
// visbility and height changes can only be detected via an onLayoutChange() event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in "visibility".
} | ||
|
||
/** | ||
* To be called by the owner when the activity/view's overriden onConfigurationChanged() method has been called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in "overridden".
/** | ||
* To be called by the owner when the activity/view's overriden onConfigurationChanged() method has been called. | ||
* Updates the toolbar's height and font size base on the given configuration. | ||
* @param newConfig The udpated configuration applied to the activity/view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in "updated".
* Updates the toolbar's height and font size base on the given configuration. | ||
* @param newConfig The udpated configuration applied to the activity/view. | ||
*/ | ||
public void onConfigurationChanged(Configuration newConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The newConfig
parameter seems to not be used in the method. It is used when the method is called from:
https://github.com/appcelerator/titanium_mobile/pull/10383/files#diff-9ea98253bfde40055a91a9df7a25cb33R52
and
https://github.com/appcelerator/titanium_mobile/pull/10383/files#diff-9ce4241f5edf3cb3a0918ec183f9f59bR83
Maybe the former one may allow us to remove the parameter from the definition in TiActionBarStyleHandler
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it this way to make it clear that this method should be called when the Activity/View object's onConfigurationChanged()
method has been called. That's also partly why I decided to name the class a "handler" because it's like an event handler.
Also, I was originally thinking about optimizing this by only updating the ActionBar/Toolbar when the given configuration object indicated that the orientation changed. But onConfigurationChanged()
is actually rarely called. So, I didn't think it was worth it.
Are you getting a compiler warning? I may need to add a @SuppressWarnings("unused")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw it was unused. I am OK to leave it as is, matching the declaration makes sense for it being more clear what is happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome PR! Besides a few minor things I have commented on it looks good!
Thanks for the review @ypbnv . :) I suspect the following may not work with your refactored |
…tend their background - Fixed issue where if multiple toolbars had "extendBackground" true, it would only apply to 1st one. * Had to override default behavior to prevent insets from being consumed. - Fixed regression where toolbar "extendBackground" true would no longer work unless window "extendSafeArea" was also true. * Caused by root Titanium view calling setFitsSystemWindows(true) on startup, instead of default false setting.
Updated PR:
|
apidoc/Titanium/UI/Window.yml
Outdated
type: Boolean | ||
default: true | ||
since: 6.3.0 | ||
default: `true` on iOS. `false` on Android. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is causing the docgen validation to error out as it's a string (which fails the parsing of Window.yml, throwing all the other parse errors)
It's super easy to allow the validation script to allow us to set {android: false, ios: true}
here, but I have no idea how this then plays into the generation of the various templates (html, jdsuck, typescript etc.). @jawa9000 do you know how I can check the output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll give it a go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FR Passed
Verified using the test cases above, extendSafeArea
and safeAreaPadding
work as expected with the notch supports
ENV
Pixel 2 XL (9.0)
Emulator (9.0, 6.0, 4.4.2)
Appc NPM: 4.2.13
Appc CLI: 7.0.7-9
Ti CLI: 5.1.1
Node: 8.12.0
NPM: 6.4.1
Generated by 🚫 dangerJS |
JIRA:
Summary:
Ti.UI.Window
property "extendSafeArea" to render content beneath insets.false
by default on Android. This is because insets can be of any size and on side, making it more problematic.Ti.UI.Window
property "safeAreaPadding".true
.false
.Ti.UI.Window
property "windowFlags":Ti.UI.Android.FLAG_TRANSLUCENT_NAVIGATION
Ti.UI.Android.FLAG_TRANSLUCENT_STATUS
ActionBar
height won't resize upon orientation changes.Toolbar
height set toTi.UI.SIZE
won't resize upon orientation change.Toolbar
with "extendBackground" totrue
is too tall if both status bar and nav bar are translucent.Toolbar
with "extendBackground"true
to position itself beneath top inset.Test Prerequisite:
You must obtain an Android P device with a notch before doing the below tests.
If the device doesn't have a notch, then you can simulate it as follows:
Toolbar Test:
Toolbar
. (See TIMOB-26442 screenshots.)Toolbar
height and font size shrinks. (This tests TIMOB-26460.)Toolbar
height and font size increase in portrait.ActionBar Test:
Translucent StatusBar and NavBar Test:
Note that Android 4.4 will show an opaque navigation bar when displayed landscape. This is a limitation on Google's end.
Extend Safe-Area Window Test:
ExtendSafeAreaContainerTest.js
attached to TIMOB-26427.Extend Safe-Area ScrollView Test:
ExtendSafeAreaScrollViewTest.js
attached to TIMOB-26427.Extend Safe-Area TabGroup Test:
ExtendSafeAreaTabsTest.js
attached to TIMOB-26427.Extend Safe-Area DrawerLayout Test:
ExtendSafeAreaDrawerTest.js
attached to TIMOB-26427.