[Remoting Android] Create Interfaces for GlDisplay

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

Index: remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java
diff --git a/remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java b/remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java
index af8f6fa82976e20f2ed25d0cdd1ac5663e02b1d2..41728d34fc2d1ca6d26c35cb861b82416cd3cb3c 100644
--- a/remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java
+++ b/remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java
@@ -23,12 +23,11 @@ public class GlDesktopView extends DesktopView implements SurfaceHolder.Callback
 
     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
@@ -37,36 +36,27 @@ public class GlDesktopView extends DesktopView implements SurfaceHolder.Callback
             }
         };
 
-        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
@@ -91,12 +81,12 @@ public class GlDesktopView extends DesktopView implements SurfaceHolder.Callback
                     }
                 });
 
-        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));
     }
 
@@ -111,6 +101,6 @@ public class GlDesktopView extends DesktopView implements SurfaceHolder.Callback
         if (mOnCanvasRenderedListenerKey != null) {
             mDisplay.onCanvasRendered().remove(mOnCanvasRenderedListenerKey);
         }
-        mDisplay.surfaceDestroyed();
+        mDisplay.surfaceDestroyed(holder);
     }
 }