[Remoting Android] Refactor GlDesktopView

remoting/android/java/src/org/chromium/chromoting/DesktopView.java

Index: remoting/android/java/src/org/chromium/chromoting/DesktopView.java
diff --git a/remoting/android/java/src/org/chromium/chromoting/DesktopView.java b/remoting/android/java/src/org/chromium/chromoting/DesktopView.java
index 84b0421361b4ca3e755449ce21c60f481f40630a..ee3a4242bfe9c2a90ee95e5c6ca31e4ab6b29f60 100644
--- a/remoting/android/java/src/org/chromium/chromoting/DesktopView.java
+++ b/remoting/android/java/src/org/chromium/chromoting/DesktopView.java
@@ -5,9 +5,8 @@
 package org.chromium.chromoting;
 
 import android.content.Context;
-import android.graphics.Matrix;
-import android.graphics.PointF;
 import android.text.InputType;
+import android.util.AttributeSet;
 import android.view.MotionEvent;
 import android.view.SurfaceView;
 import android.view.inputmethod.EditorInfo;
@@ -17,45 +16,55 @@ import android.view.inputmethod.InputMethodManager;
 import org.chromium.chromoting.jni.Client;
 
 /**
- * The abstract class for viewing and interacting with a specific remote host. Handles logic
- * for touch input and render data.
+ * The class for viewing and interacting with a specific remote host. The lifecycle of DesktopView
+ * is roughly the same as the Desktop activity.

[joedow, 2016/09/13 00:08:51]

What does 'roughly' mean?  What should be documented is whether an instance is
tied to the activity (i.e. there is a 1:1 mapping between class and activity) or
if it lives past that.  If it lives about the same amount of time as the other
activity bound classes, then you don't need this comment.

[Yuwei, 2016/09/13 01:22:46]

Sorry for the vagueness. Here are the details:

* The constructor is called inside Desktop.onCreate() by setContentView(), and
init() is called later also inside onCreate.
* onAttachedToWindow is called slightly after onCreate() by a task runner.
* onDetachedFromWindow is called slightly after onDestroy() by a task runner,
and before constructing the new Desktop activity.

[Yuwei, 2016/09/13 01:42:26]

Removed.

  */
-public abstract class DesktopView extends SurfaceView {
+public class DesktopView extends SurfaceView {
 
-    protected final TouchInputHandler mInputHandler;
+    private final Event.Raisable<TouchEventParameter> mOnTouch = new Event.Raisable<>();
 
-    /**
-     * Subclass should trigger this event when the client view size is changed.
-     */
-    protected final Event.Raisable<SizeChangedEventParameter> mOnClientSizeChanged =
-            new Event.Raisable<>();
+    /** The parent Desktop activity. */
+    private Desktop mDesktop;
 
-    /**
-     * Subclass should trigger this event when the host (desktop frame) size is changed.
-     */
-    protected final Event.Raisable<SizeChangedEventParameter> mOnHostSizeChanged =
-            new Event.Raisable<>();
+    private RenderStub mRenderStub;
 
-    /**
-     * Subclass should trigger this event when a frame is rendered.
-     */
-    protected final Event.Raisable<Void> mOnCanvasRendered = new Event.Raisable<>();
+    private TouchInputHandler mInputHandler;
 
-    /** The parent Desktop activity. */
-    private final Desktop mDesktop;
+    private InputEventSender mInputEventSender;
 
-    private final Event.Raisable<TouchEventParameter> mOnTouch = new Event.Raisable<>();
+    public DesktopView(Context context, AttributeSet attributeSet) {
+        super(context, attributeSet);
+        // Give this view keyboard focus, allowing us to customize the soft keyboard's settings.
+        setFocusableInTouchMode(true);
+    }
 
-    public DesktopView(Desktop desktop, Client client) {
-        super(desktop);
+    /**
+     * Initializes the view.
+     */
+    public void init(Client client, Desktop desktop, RenderStub renderStub) {
+        Preconditions.isNull(mDesktop);
+        Preconditions.isNull(mRenderStub);
+        Preconditions.isNull(mInputEventSender);
         Preconditions.notNull(desktop);
-        Preconditions.notNull(client);
+        Preconditions.notNull(renderStub);
         mDesktop = desktop;
-        mInputHandler = new TouchInputHandler(this, desktop);
-        mInputHandler.init(desktop, new InputEventSender(client));
+        mRenderStub = renderStub;
+        mInputEventSender = new InputEventSender(client);
+    }
 
-        // Give this view keyboard focus, allowing us to customize the soft keyboard's settings.
-        setFocusableInTouchMode(true);
+    @Override
+    public void onAttachedToWindow() {
+        Preconditions.isNull(mInputHandler);
+        super.onAttachedToWindow();
+        mRenderStub.setDesktopView(this);
+        mInputHandler = new TouchInputHandler(this, mDesktop, mRenderStub, mInputEventSender);
+    }
+
+    @Override
+    public void onDetachedFromWindow() {
+        Preconditions.notNull(mInputHandler);

[joedow, 2016/09/13 00:08:51]

I think the check on line 57 is fine (to prevent multiple attaches) however if
there are multiple detaches, you are going to detect it really fast (when the
app crashes on the next line).  I'm not sure this check is useful here.

[Yuwei, 2016/09/13 01:22:46]

detachEventListeners() doesn't check whether it has already called since what it
does is just clearing the list...

Shall I also add assert that the event list is not empty?

Arguably we may not need to add any check here since onAttachedToWindow and
onDetachedFromWindow are called by the Android library.

[Yuwei, 2016/09/13 01:42:26]

Removed this check and added emptiness check in detachEventListeners().

+        mInputHandler.detachEventListeners();
+        super.onDetachedFromWindow();
     }
 
     // TODO(yuweih): move showActionBar and showKeyboard out of this abstract class.
@@ -76,22 +85,6 @@ public abstract class DesktopView extends SurfaceView {
         return mOnTouch;
     }
 
-    /** An {@link Event} which is triggered when the client size is changed. */
-    public final Event<SizeChangedEventParameter> onClientSizeChanged() {
-        return mOnClientSizeChanged;
-    }
-
-    /** An {@link Event} which is triggered when the host size is changed. */
-    public final Event<SizeChangedEventParameter> onHostSizeChanged() {
-        return mOnHostSizeChanged;
-    }
-
-    /** An {@link Event} which is triggered when a frame is rendered. */
-    public final Event<Void> onCanvasRendered() {
-        return mOnCanvasRendered;
-    }
-
-    // View overrides.
     /** Called when a software keyboard is requested, and specifies its options. */
     @Override
     public final InputConnection onCreateInputConnection(EditorInfo outAttrs) {
@@ -117,25 +110,4 @@ public abstract class DesktopView extends SurfaceView {
         mOnTouch.raise(parameter);
         return parameter.handled;
     }
-
-    /** Triggers a brief animation to indicate the existence and location of an input event. */
-    public abstract void showInputFeedback(RenderStub.InputFeedbackType feedbackToShow, PointF pos);
-
-    /**
-     * Informs the view that its transformation matrix (for rendering the remote desktop bitmap)
-     * has been changed by the TouchInputHandler, which requires repainting.
-     */
-    public abstract void transformationChanged(Matrix matrix);
-
-    /**
-     * Informs the view that the cursor has been moved by the TouchInputHandler, which requires
-     * repainting.
-     */
-    public abstract void cursorMoved(PointF position);
-
-    /**
-     * Informs the view that the cursor visibility has been changed (for different input mode) by
-     * the TouchInputHandler, which requires repainting.
-     */
-    public abstract void cursorVisibilityChanged(boolean visible);
 }