Fixing image canvas resize problems when showing/hiding system UI.

Fixing image canvas resize problems when showing/hiding system UI.

The previous behavior for the image canvas was to allow the system to
resize the canvas based on the systemUI being displayed.  The two
problems with this approach were that the canvas could receive mulitple
resize events as systemUI was hidden (resulting in a jerky resize of the
canvas) and that area beneath the system bars would not match the color
of the background (white vs. black) so that area would appear to flash.

On a fit and finish side, it was odd to have the canvas resize because
the ActionBar or navigation bar was displayed as these are transient UI
elements which are infrequently needed.  I've also added an auto-hide
timer to hide the ActionBar after a period of inactivity.

To address these issues, I have updated the layout of the Desktop
activity such that the image canvas is effectively streched to fit the
screen and the /action/navigation/system UI is drawn over the top.  This
removes the need for frequent resizing and results in a smoother
experience.  The second change needed was to detect when the on screen
keyboard is present and shift the remote image canvas upwards to
accommodate the reduced screen size.

There is no straight-forward way to detect the OSK presence on Android
so my solution was to add a transparent UI element which is resized by
the system when systemUI is displayed and use an attached resize
listener to detect the new screen bounds.  I then use this info to shift
the canvas appropriately.  This mechanism will also allow us to display
adornments at the top of the on screen keyboard if necessary.

Note: This change is targeting KitKat and above as earlier API versions
do not support all of the flags needed to properly support this work.

BUG=548003
Committed: https://crrev.com/decde3542a6456595150345eb99e8afe705a3397
Cr-Commit-Position: refs/heads/master@{#357445}

[joedow, 2015-10-28 20:19:29]

joedow@chromium.org changed reviewers:
+ lambroslambrou@chromium.org

[joedow, 2015-10-28 20:19:29]

This change updates the behavior on KitKat+ Android devices such that the remote
image canvas is always fullscreen which addresses the resizing and flashing
problems we were seeing.  

I have tested this on a JellyBean, KitKat, and Marshmallow device.

PTAL!

Thanks,
Joe

[Lambros, 2015-10-29 23:39:16]

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/r...
File remoting/android/java/res/layout/desktop.xml (right):

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/r...
remoting/android/java/res/layout/desktop.xml:14:
android:layout_height="wrap_content"/>
match_parent?

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/r...
remoting/android/java/res/layout/desktop.xml:18: android:orientation="vertical"
remove orientation?

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/r...
remoting/android/java/res/layout/desktop.xml:28: <RelativeLayout
Maybe FrameLayout or View or some other generic placeholder?

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/r...
remoting/android/java/res/layout/desktop.xml:33: android:orientation="vertical"
remove orientation?

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right):

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:181:
mActionBarAutoHideTask = new Runnable() {
optional: Don't really need to keep setting mActionBarAutoHideTask to null and
re-creating it. Just initialize it once (at declaration). Then, here, you can
unconditionally stop the timer, which will do nothing if the timer isn't
running.

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:222: private int
getSystemUiFlags() {
rename to getFullscreenFlags() ?

Also, this should be static :)

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:230: public void
showActionBar() {
Maybe need @SuppressLint("InlinedApi") here?

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:237: flags |=
View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN;
Since these are duplicated below, maybe have a getLayoutFlags() or similar
function?

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:356: if
(mInitialRect == null || bottom > mInitialRect.bottom) {
Maybe initialize mInitialRect and mutate it instead of creating new Rect() each
time?

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:369:
mSoftInputVisible = ((mInitialRect.bottom - bottom) > 0);
Maybe replace Rect with int, since you only use |bottom|?

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java
(right):

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:45:
void onSoftInputMethodVisibilityChanged(boolean inputMethodVisible, Rect
innerBounds);
Please document what the left/top of innerBounds are measured relative to. If
these are always zero, pass width/height instead as in the other methods of this
interface.

[joedow, 2015-10-30 17:41:23]

PTAL!

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/r...
File remoting/android/java/res/layout/desktop.xml (right):

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/r...
remoting/android/java/res/layout/desktop.xml:14:
android:layout_height="wrap_content"/>
On 2015/10/29 23:39:16, Lambros wrote:
> match_parent?

Done.

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/r...
remoting/android/java/res/layout/desktop.xml:18: android:orientation="vertical"
On 2015/10/29 23:39:16, Lambros wrote:
> remove orientation?

Done.

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/r...
remoting/android/java/res/layout/desktop.xml:28: <RelativeLayout
On 2015/10/29 23:39:16, Lambros wrote:
> Maybe FrameLayout or View or some other generic placeholder?

Done.

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/r...
remoting/android/java/res/layout/desktop.xml:33: android:orientation="vertical"
On 2015/10/29 23:39:16, Lambros wrote:
> remove orientation?

Done.

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right):

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:181:
mActionBarAutoHideTask = new Runnable() {
On 2015/10/29 23:39:16, Lambros wrote:
> optional: Don't really need to keep setting mActionBarAutoHideTask to null and
> re-creating it. Just initialize it once (at declaration). Then, here, you can
> unconditionally stop the timer, which will do nothing if the timer isn't
> running.

Done.

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:222: private int
getSystemUiFlags() {
On 2015/10/29 23:39:16, Lambros wrote:
> rename to getFullscreenFlags() ?
> 
> Also, this should be static :)

Done.

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:230: public void
showActionBar() {
On 2015/10/29 23:39:16, Lambros wrote:
> Maybe need @SuppressLint("InlinedApi") here?

Acknowledged.

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:237: flags |=
View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN;
On 2015/10/29 23:39:16, Lambros wrote:
> Since these are duplicated below, maybe have a getLayoutFlags() or similar
> function?

Done.

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:356: if
(mInitialRect == null || bottom > mInitialRect.bottom) {
On 2015/10/29 23:39:16, Lambros wrote:
> Maybe initialize mInitialRect and mutate it instead of creating new Rect()
each
> time?

Done.

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:369:
mSoftInputVisible = ((mInitialRect.bottom - bottom) > 0);
On 2015/10/29 23:39:16, Lambros wrote:
> Maybe replace Rect with int, since you only use |bottom|?

Done.

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java
(right):

https://codereview.chromium.org/1427453004/diff/20001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/TouchInputHandler.java:45:
void onSoftInputMethodVisibilityChanged(boolean inputMethodVisible, Rect
innerBounds);
On 2015/10/29 23:39:16, Lambros wrote:
> Please document what the left/top of innerBounds are measured relative to. If
> these are always zero, pass width/height instead as in the other methods of
this
> interface.

Done.

[joedow, 2015-10-30 18:21:54]

One last tweak, removed the 'Hide' action from the ActionBar for API levels
which support the immersive fullscreen experience.

[joedow, 2015-11-02 16:39:37]

Ping!

[Lambros, 2015-11-02 19:48:54]

lgtm with comments

https://codereview.chromium.org/1427453004/diff/40002/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right):

https://codereview.chromium.org/1427453004/diff/40002/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:201: View
decorView = getWindow().getDecorView();
optional: getWindow().getDecorView().removeCallbacks(...)
and maybe invert the if (..) check?
Similarly for startActionBarAutoHideTimer() above?

https://codereview.chromium.org/1427453004/diff/40002/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:250: flags |=
getImmersiveLayoutFlags();
optional:
int flags = getImmersiveLayoutFlags() | View.SYSTEM_UI_FLAG_VISIBLE;

https://codereview.chromium.org/1427453004/diff/40002/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:251:
decorView.setSystemUiVisibility(flags);
optional: remove |decorView| temporary.

https://codereview.chromium.org/1427453004/diff/40002/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:362: private int
mInitialBottomValue = 0;
optional: remove '= 0', int members are 0 by default.

https://codereview.chromium.org/1427453004/diff/40002/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:362: private int
mInitialBottomValue = 0;
Maybe rename to mMaximumBottomValue ?

https://codereview.chromium.org/1427453004/diff/40002/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right):

https://codereview.chromium.org/1427453004/diff/40002/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/DesktopView.java:143: public
void onSoftInputMethodVisibilityChanged(boolean inputMethodVisible, Rect bounds)
{
Public methods ought to have JavaDoc comments. I'm surprised the linter checks
in the presubmit didn't complain here? You could add {@link} or {@see}
annotation to refer to the other similar method so you don't need to repeat all
the gory details.

https://codereview.chromium.org/1427453004/diff/40002/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/TrackingInputHandler.java
(right):

https://codereview.chromium.org/1427453004/diff/40002/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/TrackingInputHandler.java:311:
mInputMethodOffsetY = mRenderData.screenHeight - bounds.bottom;
Add synchronized (mRenderData) { .. }

[joedow, 2015-11-02 21:44:17]

The CQ bit was checked by joedow@chromium.org

[joedow, 2015-11-02 21:44:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
lambroslambrou@chromium.org
Link to the patchset: https://codereview.chromium.org/1427453004/#ps70001
(title: "Addressing code review feedback")

[commit-bot: I haz the power, 2015-11-02 21:45:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1427453004/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1427453004/70001

[commit-bot: I haz the power, 2015-11-02 22:09:26]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2015-11-02 22:10:49]

Patchset 5 (id:??) landed as
https://crrev.com/decde3542a6456595150345eb99e8afe705a3397
Cr-Commit-Position: refs/heads/master@{#357445}