[Remoting Android] Placeholder for DesktopView

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

 // Copyright 2013 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.annotation.SuppressLint;
 import android.content.DialogInterface;
 import android.content.Intent;
 import android.content.pm.ApplicationInfo;
 import android.content.pm.PackageManager;
 import android.content.pm.PackageManager.NameNotFoundException;
 import android.os.Build;
 import android.os.Bundle;
 import android.os.Handler;
 import android.support.v7.app.ActionBar.OnMenuVisibilityListener;
 import android.support.v7.app.AlertDialog;
 import android.support.v7.app.AppCompatActivity;
 import android.support.v7.widget.Toolbar;
 import android.view.KeyEvent;
 import android.view.Menu;
 import android.view.MenuItem;
 import android.view.MotionEvent;
 import android.view.View;
 import android.view.View.OnLayoutChangeListener;
 import android.view.View.OnTouchListener;
+import android.view.ViewGroup;
 import android.view.inputmethod.InputMethodManager;
 
 import org.chromium.chromoting.cardboard.DesktopActivity;
 import org.chromium.chromoting.help.HelpContext;
 import org.chromium.chromoting.help.HelpSingleton;
 import org.chromium.chromoting.jni.Client;
 
 import java.util.List;
 
 /**
@@ skipping 60 matching lines @@
     public void onCreate(Bundle savedInstanceState) {
         super.onCreate(savedInstanceState);
         setContentView(R.layout.desktop);
 
         mClient = Client.getInstance();
         mInjector = new InputEventSender(mClient);
 
         mToolbar = (Toolbar) findViewById(R.id.toolbar);
         setSupportActionBar(mToolbar);
 
-        DesktopView remoteHostDesktop = (DesktopView) findViewById(R.id.desktop_view);
+        DesktopViewInterface remoteHostDesktop =
+                mClient.getDisplay().createDesktopView(getApplicationContext());

[Lambros, 2016/07/08 02:13:41]

When creating Views, it's important to use the activity context, not the app
context (I expect you'll see some crashes). Pass 'this' here instead.

When the Activity goes away, after onDestroy(), the View will no longer be
valid. If JNI or anything else can call into the View and modify any UI, you'll
have to add something to onDestroy() to stop that from happening.

[Yuwei, 2016/07/08 19:58:56]

Done
I think that's fine. Both desktop views override SurfaceDestroyed() and do
cleanup there.

+        remoteHostDesktop.setLayoutParams(new ViewGroup.LayoutParams(
+                ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.MATCH_PARENT));
+        ((ViewGroup) findViewById(R.id.desktop_view_placeholder)).addView((View) remoteHostDesktop);

[Lambros, 2016/07/08 02:13:41]

The cast to (View) is ugly, maybe indicates a design weakness?

It seems like you want to force any DesktopViewInterface implementations to be
View-derived objects? Maybe an abstract View-derived class would be a better
choice than a pure interface?

Alternatively, have the View object and DesktopViewInterface impl as separate
objects, where one is a member of the other?

I don't feel too strongly if you have reasons for wanting this design.

[Yuwei, 2016/07/08 19:58:56]

Actually I have considered making it an abstract class extending View but the
problem is both desktop views extend from SurfaceView and Java doesn't allow
multi-class inheritance. It will work if I extend the desktop view "interface"
from SurfaceView but I'm not sure whether this is a flexible design...

Maybe just make it extend from SurfaceView for now? It's unlikely we will have
the third rendering implementation in foreseeable future...

[Lambros, 2016/07/08 22:02:36]

I think this is fine. As long as you are rendering in another thread, you are
going to need a SurfaceView anyway.

If you like, you could make your own SurfaceView-derived class, and have any
differences in implementation delegated to a separate object which can be
created/switched at run-time. If you had

class MySurfaceView extends SurfaceView {
public MySurfaceView(Context context, AttributeSet attributes) {...}
}

You could add <org.chromium.chromoting.MySurfaceView>.. directly into your
layout XML without needing any placeholder View.

[Yuwei, 2016/07/08 23:07:54]

This sounds like a better approach, but also requires a lot of refactorizations
O_o

This may sound a little bit hacky. I'm not sure whether a surface that had been
used to draw Java canvas can be reused for rendering OpenGL buffers when
switching impl. at run-time...

I don't have strong opinion about this. Maybe check in this CL and consider this
approach later?

[Yuwei, 2016/07/08 23:35:53]

CC zijiehe@. There is a discussion about making DesktopView a pure drawing
canvas and pulling out the drawing logic. Do you have any opinion about this
change or is it something already in your refactoring plan?

         remoteHostDesktop.init(this, mClient);
         mSwitchToCardboardDesktopActivity = false;
 
         getSupportActionBar().setDisplayShowTitleEnabled(false);
         getSupportActionBar().setDisplayHomeAsUpEnabled(true);
 
         // For this Activity, the home button in the action bar acts as a Disconnect button, so
         // set the description for accessibility/screen readers.
         getSupportActionBar().setHomeActionContentDescription(R.string.disconnect_myself_button);
 
@@ skipping 38 matching lines @@
         } else {
             remoteHostDesktop.setFitsSystemWindows(true);
         }
     }
 
     @Override
     protected void onStart() {
         super.onStart();
         mActivityLifecycleListener.onActivityStarted(this);
         mClient.enableVideoChannel(true);
-        DesktopView desktopView = (DesktopView) findViewById(R.id.desktop_view);

[Yuwei, 2016/07/08 00:30:00]

This logic is unnecessary since attachRedrawCallback() is already called inside
DesktopView.surfaceCreated(). Removed this since there is no more
R.id.desktop_view.

-        desktopView.attachRedrawCallback();
         mClient.getCapabilityManager().addListener(this);
     }
 
     @Override
     protected void onPause() {
         if (isFinishing()) mActivityLifecycleListener.onActivityPaused(this);
         super.onPause();
         if (!mSwitchToCardboardDesktopActivity) {
             mClient.enableVideoChannel(false);
         }
@@ skipping 429 matching lines @@
     @Override
     public boolean dispatchKeyEvent(KeyEvent event) {
         if (event.getKeyCode() == KeyEvent.KEYCODE_BACK) {
             mClient.destroy();
             return super.dispatchKeyEvent(event);
         }
 
         return mInjector.sendKeyEvent(event);
     }
 }