📰 Add visibility change triggers for bottom sheet content

chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java

Index: chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java b/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java
new file mode 100644
index 0000000000000000000000000000000000000000..277e61019d3cf0da65d5491d1b6eea44e2159a1d
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/widget/bottomsheet/SheetContentStateChangeObserver.java
@@ -0,0 +1,98 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.widget.bottomsheet;
+
+/**
+ * Notifies of events dedicated to changes in visibility of a specific
+ * {@link BottomSheet.BottomSheetContent}.
+ */
+public abstract class SheetContentStateChangeObserver extends EmptyBottomSheetObserver {
+    @BottomSheet.SheetState
+    private int mCurrentSheetState;
+    private boolean mCurrentVisibility;
+
+    private final BottomSheet.BottomSheetContent mContentObserved;
+    private final BottomSheet mBottomSheet;
+
+    /**
+     * @param bottomSheetContent BottomSheetContent to observe visibility for.
+     * @param bottomSheet The BottomSheet the observed content is registered with. Note: the
+     *                    constructor does not register the object as observer!
+     */
+    public SheetContentStateChangeObserver(
+            BottomSheet.BottomSheetContent bottomSheetContent, BottomSheet bottomSheet) {
+        mContentObserved = bottomSheetContent;
+        mBottomSheet = bottomSheet;
+
+        onStateChange();
+    }
+
+    /** Called when the observed content becomes visible. */
+    public abstract void onContentShown();
+
+    /** Called when the observed content becomes invisible. */
+    public abstract void onContentHidden();
+
+    /**
+     * Called when the bottom sheet stabilises with a new state or visibility.
+     * @param sheetState The new {@link BottomSheet.SheetState}, restricted to stable states:
+     *                   {@link BottomSheet#SHEET_STATE_FULL},{@link BottomSheet#SHEET_STATE_HALF}
+     *                   or {@link BottomSheet#SHEET_STATE_PEEK}
+     */
+    public abstract void onContentStateChanged(int sheetState);

[Bernhard Bauer, 2017/05/05 13:39:54]

@BottomSheet.SheetState?

[dgn, 2017/05/09 16:55:51]

Done.

+
+    @BottomSheet.SheetState
+    protected int getCurrentState() {
+        return mCurrentSheetState;
+    }
+
+    @Override
+    public void onSheetOpened() {
+        onStateChange();
+    }
+
+    @Override
+    public void onSheetClosed() {
+        onStateChange();
+    }
+
+    @Override
+    public void onSheetContentChanged(BottomSheet.BottomSheetContent newContent) {
+        onStateChange();
+    }
+
+    @Override
+    public void onSheetStateChanged(int newState) {
+        if (newState == BottomSheet.SHEET_STATE_SCROLLING) return; // Ignore intermediate states.

[Bernhard Bauer, 2017/05/05 13:39:55]

Don't you already have a seemingly more accurate check in onStateChange()?

[dgn, 2017/05/09 16:55:51]

Done.

+        onStateChange();
+    }
+
+    private void onStateChange() {
+        @BottomSheet.SheetState
+        int newSheetState = mBottomSheet.getSheetState();
+        boolean newVisibility = mBottomSheet.isSheetOpen()
+                && mBottomSheet.getCurrentSheetContent() == mContentObserved;
+
+        if (newVisibility) {
+            mBottomSheet.getCurrentSheetContent();

[Bernhard Bauer, 2017/05/05 13:39:55]

What's this for? Ensuring that the sheet content is created? It seems that would
already happen in line 76 (in order for |newVisibility| to be true, both
conditions need to be true and therefore get evaluated).

[dgn, 2017/05/09 16:55:51]

oops ignore, that has nothing to do here.

+        }
+
+        if (newVisibility == mCurrentVisibility && mCurrentSheetState == newSheetState) return;

[Bernhard Bauer, 2017/05/05 13:39:55]

The first part of this check is already in line 84, and you could just move the
second part to line 93.

[dgn, 2017/05/09 16:55:51]

Hum that's the kind of stuff that makes me think I should not present this as a
general purpose class. 

I would still want to keep the whole check for the onContentStateChanged
detection, because even if the current state didn't change, if the visibility
did I still want the notification.

+
+        if (newVisibility != mCurrentVisibility) {
+            if (newVisibility) {
+                onContentShown();
+            } else {
+                onContentHidden();
+            }
+            mCurrentVisibility = newVisibility;
+        }
+
+        if (BottomSheet.isStateStable(newSheetState)) {
+            onContentStateChanged(newSheetState);
+            mCurrentSheetState = newSheetState;
+        }
+    }
+}