[Remoting Android] Create Interfaces for GlDisplay

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

 // Copyright 2016 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.chromoting;
 
 import android.graphics.Matrix;
 import android.graphics.PointF;
 import android.view.SurfaceHolder;
 
 import org.chromium.chromoting.jni.Client;
 import org.chromium.chromoting.jni.GlDisplay;
 
 /**
  * The user interface for viewing and interacting with a specific remote host. Uses OpenGL to draw
  * the desktop and cursor. Should be used entirely on the UI thread.
  */
 public class GlDesktopView extends DesktopView implements SurfaceHolder.Callback {
     private final GlDisplay mDisplay;
 
     private Object mOnHostSizeChangedListenerKey;
     private Object mOnCanvasRenderedListenerKey;
 
     private Event.ParameterRunnable<Void> mProcessAnimationRunnable;
 
-    private float mScaleFactor;
-
     public GlDesktopView(GlDisplay display, Desktop desktop, Client client) {
         super(desktop, client);
         Preconditions.notNull(display);
         mDisplay = display;
+        display.setDesktopView(this);
 
         mProcessAnimationRunnable = new Event.ParameterRunnable<Void>() {
             @Override
             public void run(Void p) {
                 mInputHandler.processAnimation();
             }
         };
 
-        mScaleFactor = 0;
-
         getHolder().addCallback(this);
     }
 
     @Override
-    public void showInputFeedback(InputFeedbackType feedbackToShow, PointF pos) {
-        float diameter = getFeedbackRadius(feedbackToShow, mScaleFactor) * 2.0f;
-        if (diameter <= 0.0f) {
-            return;
-        }
-        mDisplay.showCursorInputFeedback(pos.x, pos.y, diameter);
+    public void showInputFeedback(RenderStub.InputFeedbackType feedbackToShow, PointF pos) {
+        mDisplay.showInputFeedback(feedbackToShow, pos);

[Hzj_jie, 2016/08/30 23:15:03]

I believe you will remove this and following four functions in the coming
change.

But I still think a beautiful solution would be, let TouchInputHandler (or some
other replacement, as an even better solution would be an interface for these
events) to raise events for these actions, and let GlDisplay directly listen to
them. Once a new TouchInputHandler is created with its DesktopView, you only
need to send it to the GlDisplay.

The advantage would be,
1. An event can be consumed with several consumers. Considering one day you need
to show both an feedback animation and a vibrating notification for a touch, do
you think it would be better if we can separate two implementations.
2. Less duplicate code, but same functionality. You do not need to write an
interface for these functions, each consumer can decide which events it listens
to. You do not need to leave a blank implementation for consumers who do not
care about certain functions.
3. Never have a chance to produce this kind of recursive delegates. You do not
even have a chance to forward a function call from one interface to another.

This pattern can also help the tests, i.e. you can fake an event provider
(TouchInputHandler in this case), and let GlDisplay consume it.

What do you think?

p.s. I have gone through your design doc, but it seems you have not mentioned
this.

[Yuwei, 2016/08/31 00:15:56]

Yep. The whole GlDesktopView class will go away.
For this CL I just tried to make RenderStub a very simple interface since
* When we want to have a future implementation, we just need to go through the
interface and implement all functions there then we can get a working
RenderStub.
* No burden for registering itself to events. (I know you may not consider this
as a burder :P)
* Looks like we don't have a solid design pattern for forwarding and grouping
events so I would like to wait and see how you use them before switching to the
new pattern. Having a simple RenderStub can unblock me for the goal of
simplifying GlDesktopView for now.

Basically my plan was to have RenderStub and in the future have a helper class
to register it to different events in the future. I'm happy to change my design
if you have a better design pattern :)
Acknowledged.
But you will need to have an EventProvider interface, right? If the class
triggers the event also owns the event then we may end up having lots of
interfaces, say one for TouchInputHandler and another for DesktopCanvas, or any
member objects of TouchInputHandler.
Not sure whether I understand this well... Isn't it f(a, b, c) { g.f(a, b, c); }
?
For testing GlDisplay if I already have access to all its functions then why do
I need to fake TouchInputHandler?
Feel slightly mixed. I can try to make a prototype and see how that works.
Yes I just tried to hold off this change since this involves fundamental change
of TouchInputHandler and its member.

[Hzj_jie, 2016/08/31 00:31:57]

You may have a look at InputMonitor class as an example. Sorry, I have no time
these days to continually work on it. But I will try to provide more samples as
soon as possible.

[Yuwei, 2016/08/31 00:47:40]

Thanks!

It looks like A RenderStub to events adapter?

     }
 
     @Override
     public void transformationChanged(Matrix matrix) {
-        float[] matrixArray = new float[9];
-        matrix.getValues(matrixArray);
-        mDisplay.pixelTransformationChanged(matrixArray);
-        mScaleFactor = matrix.mapRadius(1);
+        mDisplay.setTransformation(matrix);
     }
 
     @Override
     public void cursorMoved(PointF position) {
-        mDisplay.cursorPixelPositionChanged(position.x, position.y);
+        mDisplay.moveCursor(position);
     }
 
     @Override
     public void cursorVisibilityChanged(boolean visible) {
-        mDisplay.cursorVisibilityChanged(visible);
+        mDisplay.setCursorVisibility(visible);
     }
 
     @Override
     public void setAnimationEnabled(boolean enabled) {
         if (enabled && mOnCanvasRenderedListenerKey == null) {
             mOnCanvasRenderedListenerKey = mDisplay.onCanvasRendered()
                     .add(mProcessAnimationRunnable);
             mInputHandler.processAnimation();
         } else if (!enabled && mOnCanvasRenderedListenerKey != null) {
             mDisplay.onCanvasRendered().remove(mOnCanvasRenderedListenerKey);
             mOnCanvasRenderedListenerKey = null;
         }
     }
 
     @Override
     public void surfaceCreated(SurfaceHolder holder) {
         mOnHostSizeChangedListenerKey = mDisplay
                 .onHostSizeChanged().add(new Event.ParameterRunnable<SizeChangedEventParameter>() {
                     @Override
                     public void run(SizeChangedEventParameter p) {
                         mOnHostSizeChanged.raise(p);
                     }
                 });
 
-        mDisplay.surfaceCreated(holder.getSurface());
+        mDisplay.surfaceCreated(holder);
     }
 
     @Override
     public void surfaceChanged(SurfaceHolder holder, int format, int width, int height) {
-        mDisplay.surfaceChanged(width, height);
+        mDisplay.surfaceChanged(holder, format, width, height);
         mOnClientSizeChanged.raise(new SizeChangedEventParameter(width, height));
     }
 
     @Override
     public void surfaceDestroyed(SurfaceHolder holder) {
         // GlDisplay's life time spans to the whole session while GlDesktopView may be created and
         // destroyed for multiple times (say when the phone is rotated). It is important to remove
         // the listeners when the surface is about to be destroyed.
         if (mOnHostSizeChangedListenerKey != null) {
             mDisplay.onHostSizeChanged().remove(mOnHostSizeChangedListenerKey);
         }
         if (mOnCanvasRenderedListenerKey != null) {
             mDisplay.onCanvasRendered().remove(mOnCanvasRenderedListenerKey);
         }
-        mDisplay.surfaceDestroyed();
+        mDisplay.surfaceDestroyed(holder);
     }
 }