|
|
Description[Remoting Android] Create Interfaces for GlDisplay
This CL adds RenderStub and RenderCallback interface for GlDisplay in
preparation of moving render controlling logic out of DesktopView and
removing GlDesktopView.
BUG=641123
Committed: https://crrev.com/0c198c51b1a159bc0354106588ef54903e4d818c
Cr-Commit-Position: refs/heads/master@{#416415}
Patch Set 1 : [Remoting Android] Create Interfaces for GlDisplay #
Total comments: 20
Patch Set 2 : Reviewer's Feedback #
Total comments: 11
Patch Set 3 : Fix comment: Initializes -> Resets #Patch Set 4 : Merge ToT #Patch Set 5 : Merge ToT again #Messages
Total messages: 35 (9 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Remoting Android] Create Interfaces for GlDisplay ========== to ========== [Remoting Android] Create Interfaces for GlDisplay This CL adds RenderStub and RenderCallback interface for GlDisplay in preparation of moving render controlling logic out of DesktopView. BUG=641123 ==========
yuweih@chromium.org changed reviewers: + joedow@chromium.org, zijiehe@chromium.org
Description was changed from ========== [Remoting Android] Create Interfaces for GlDisplay This CL adds RenderStub and RenderCallback interface for GlDisplay in preparation of moving render controlling logic out of DesktopView. BUG=641123 ========== to ========== [Remoting Android] Create Interfaces for GlDisplay This CL adds RenderStub and RenderCallback interface for GlDisplay in preparation of moving render controlling logic out of DesktopView and the removing GlDesktopView. BUG=641123 ==========
ptal
Description was changed from ========== [Remoting Android] Create Interfaces for GlDisplay This CL adds RenderStub and RenderCallback interface for GlDisplay in preparation of moving render controlling logic out of DesktopView and the removing GlDesktopView. BUG=641123 ========== to ========== [Remoting Android] Create Interfaces for GlDisplay This CL adds RenderStub and RenderCallback interface for GlDisplay in preparation of moving render controlling logic out of DesktopView and removing GlDesktopView. BUG=641123 ==========
FYI this is my plan for refactoring DesktopView: https://docs.google.com/document/d/1mdJNI-CnpoJf6tSCeKphK6Bd4fyqtn3sjA_Nm5EXU...
https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: final class InputFeedbackRadiusMapper { Not sure whether it is good to make this a subclass of RenderStub. Maybe make it a standalone class?
https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderCallback.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderCallback.java:10: public interface RenderCallback { Why separate this out from the Render stub? It seems like this could be exposed by the class implementing the RenderStub interface https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:34: * Informs the view that the cursor has been moved by the TouchInputHandler, which requires I'd remove 'TouchInputHandler' as we could introduce a new class tomorrow that calls into the RenderStub and this comment would be outdated. Same with all of the other comments. https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: final class InputFeedbackRadiusMapper { On 2016/08/29 22:09:42, Yuwei wrote: > Not sure whether it is good to make this a subclass of RenderStub. Maybe make it > a standalone class? I think it is ok here but I'm wondering if it even needs to be exposed by the RenderStub. Could this interface expose a method that receives an InputFeedbackType and a position and internally this class figures out what to draw? https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:45: mScaleFactor = 0; Why not set the default value above on line 41 instead of inside the ctor? https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:57: // SurfaceHolder.Callback overrides. remove, we don't document this in Java. https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:94: // RenderStub overrides. remove
https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:39: getHolder().addCallback(this); It looks like you are still using GlDesktopView for SurfaceHolder.Callback, will you change to use GlDisplay directly in a coming change? https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:28: * Informs the view that its transformation matrix (for rendering the remote desktop bitmap) Maybe view -> stub, ditto. https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: final class InputFeedbackRadiusMapper { On 2016/08/29 22:09:42, Yuwei wrote: > Not sure whether it is good to make this a subclass of RenderStub. Maybe make it > a standalone class? The original design is to use a float diameter to indicate the size of the feedback (GlDisplay.showCursorInputFeedback). Is there any reason to change it the InputFeedbackType? From my opinion, a void showFeedback(float size, PointF pos); may be more useful.
https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:39: getHolder().addCallback(this); On 2016/08/29 22:52:51, Hzj_jie wrote: > It looks like you are still using GlDesktopView for SurfaceHolder.Callback, will > you change to use GlDisplay directly in a coming change? Yep. I plan to completely remove GlDesktopView in future changes. You can refer to this in the design doc. https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderCallback.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderCallback.java:10: public interface RenderCallback { On 2016/08/29 22:35:41, joedow wrote: > Why separate this out from the Render stub? It seems like this could be exposed > by the class implementing the RenderStub interface I've considered having only one interface but 1. Currently multiple DesktopView/TouchInputHandler may be created in single GlDisplay lifetime so we might need to have a special scoped RenderCallback implementation for forwarding events for the lifetime of TouchInputHandler. 2. I think RenderStub and RenderCallback may not share the same functionality and they may potentially be implemented by different classes. #2 is not very important though. If we later resolve #1 (i.e. making TouchInputHandler live as long as GlDisplay lives) then this interface can be a part of RenderStub. https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: final class InputFeedbackRadiusMapper { On 2016/08/29 22:35:41, joedow wrote: > On 2016/08/29 22:09:42, Yuwei wrote: > > Not sure whether it is good to make this a subclass of RenderStub. Maybe make > it > > a standalone class? > > I think it is ok here but I'm wondering if it even needs to be exposed by the > RenderStub. Could this interface expose a method that receives an > InputFeedbackType and a position and internally this class figures out what to > draw? I think there are two issues: * Default function in interface is not allowed until N SDK is deployed. * As Lambros pointed out in previous CL, how the feedback type is rendered is the renderer's implementation detail (say if a renderer implements vibration feedback) so we probably don't want to handle the conversion inside RenderStub.
https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: final class InputFeedbackRadiusMapper { On 2016/08/29 22:52:51, Hzj_jie wrote: > On 2016/08/29 22:09:42, Yuwei wrote: > > Not sure whether it is good to make this a subclass of RenderStub. Maybe make > it > > a standalone class? > > The original design is to use a float diameter to indicate the size of the > feedback (GlDisplay.showCursorInputFeedback). Is there any reason to change it > the InputFeedbackType? From my opinion, a void showFeedback(float size, PointF > pos); may be more useful. I think the question here is whether we want all implementations to show a circle for touch feedback and disallow any other forms of feedback in the future.
PTAL https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderCallback.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderCallback.java:10: public interface RenderCallback { On 2016/08/29 22:35:41, joedow wrote: > Why separate this out from the Render stub? It seems like this could be exposed > by the class implementing the RenderStub interface Put this back into RenderStub. We can later fix the leaking problem inside TouchInputHandler. https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:28: * Informs the view that its transformation matrix (for rendering the remote desktop bitmap) On 2016/08/29 22:52:51, Hzj_jie wrote: > Maybe view -> stub, ditto. Done. https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:34: * Informs the view that the cursor has been moved by the TouchInputHandler, which requires On 2016/08/29 22:35:41, joedow wrote: > I'd remove 'TouchInputHandler' as we could introduce a new class tomorrow that > calls into the RenderStub and this comment would be outdated. Same with all of > the other comments. Done. https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:45: mScaleFactor = 0; On 2016/08/29 22:35:41, joedow wrote: > Why not set the default value above on line 41 instead of inside the ctor? Done. https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:57: // SurfaceHolder.Callback overrides. On 2016/08/29 22:35:41, joedow wrote: > remove, we don't document this in Java. Done.
https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: final class InputFeedbackRadiusMapper { On 2016/08/30 22:08:00, Yuwei wrote: > On 2016/08/29 22:52:51, Hzj_jie wrote: > > On 2016/08/29 22:09:42, Yuwei wrote: > > > Not sure whether it is good to make this a subclass of RenderStub. Maybe > make > > it > > > a standalone class? > > > > The original design is to use a float diameter to indicate the size of the > > feedback (GlDisplay.showCursorInputFeedback). Is there any reason to change it > > the InputFeedbackType? From my opinion, a void showFeedback(float size, PointF > > pos); may be more useful. > > I think the question here is whether we want all implementations to show a > circle for touch feedback and disallow any other forms of feedback in the > future. My understanding of Lambros' point was that if you had an enum which had 'small' and 'large' types then it's possible that a future implementation could interpret these values differently. I think that exposing a method which receives a float param called diameter and a position is just as inflexible (i.e. if we decided to use an audio tone (for example) as feedback, you'd need another method anyway). My point is why should any of the classes other than the renderer care how big the radius is. From an input strategy standpoint, they are defining the type of animation which should be rendered. The renderer knows how big to draw it. Wouldn't it be simpler to say something like: drawAnimation(center_point, SMALL_TOUCH_ANIMATION) and have the renderer figure out size. Later we could do some thing like drawAnimation(center_point, BLUE_ALPAH_SQUARE) and the renderer would take care of it.
https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:44: mDisplay.showInputFeedback(feedbackToShow, pos); 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. https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: Event<SizeChangedEventParameter> onClientSizeChanged(); According to the following implementation, I believe, same as onTouch event, this event should belong to DesktopView, but not GlDisplay or RenderStub. https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:33: private final Event.Raisable<SizeChangedEventParameter> mOnClientSizeChanged = This event is a little bit strange for me. In my opinion, an event is usually owned by the object, where the event fires. So usually client size changed event happens in a View, from it's SurfaceChanged.Callback.surfaceChanged. But you have this event in both DesktopView and GlDisplay.
https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:44: mDisplay.showInputFeedback(feedbackToShow, pos); On 2016/08/30 23:15:03, Hzj_jie wrote: > I believe you will remove this and following four functions in the coming > change. Yep. The whole GlDesktopView class will go away. > 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. 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 :) > 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. Acknowledged. > 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. 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. > 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. Not sure whether I understand this well... Isn't it f(a, b, c) { g.f(a, b, c); } ? > This pattern can also help the tests, i.e. you can fake an event provider > (TouchInputHandler in this case), and let GlDisplay consume it. For testing GlDisplay if I already have access to all its functions then why do I need to fake TouchInputHandler? > What do you think? Feel slightly mixed. I can try to make a prototype and see how that works. > p.s. I have gone through your design doc, but it seems you have not mentioned > this. Yes I just tried to hold off this change since this involves fundamental change of TouchInputHandler and its member. https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: Event<SizeChangedEventParameter> onClientSizeChanged(); On 2016/08/30 23:15:03, Hzj_jie wrote: > According to the following implementation, I believe, same as onTouch event, > this event should belong to DesktopView, but not GlDisplay or RenderStub. In the future I will have surfaceHolder.addCallback(this) in setDesktopView and drop the SurfaceHolder.Callback implementation in DesktopView. onTouch is different since it comes from the View base class. https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java (right): https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/jni/GlDisplay.java:33: private final Event.Raisable<SizeChangedEventParameter> mOnClientSizeChanged = On 2016/08/30 23:15:03, Hzj_jie wrote: > This event is a little bit strange for me. In my opinion, an event is usually > owned by the object, where the event fires. So usually client size changed event > happens in a View, from it's SurfaceChanged.Callback.surfaceChanged. But you > have this event in both DesktopView and GlDisplay. It happens in SurfaceHolder.Callback, not in the view. As mentioned above, DesktopView will no longer implement SurfaceHolder.Callback in the future.
On 2016/08/30 23:08:33, joedow wrote: > https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... > File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): > > https://codereview.chromium.org/2282783003/diff/20001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: final > class InputFeedbackRadiusMapper { > On 2016/08/30 22:08:00, Yuwei wrote: > > On 2016/08/29 22:52:51, Hzj_jie wrote: > > > On 2016/08/29 22:09:42, Yuwei wrote: > > > > Not sure whether it is good to make this a subclass of RenderStub. Maybe > > make > > > it > > > > a standalone class? > > > > > > The original design is to use a float diameter to indicate the size of the > > > feedback (GlDisplay.showCursorInputFeedback). Is there any reason to change > it > > > the InputFeedbackType? From my opinion, a void showFeedback(float size, > PointF > > > pos); may be more useful. > > > > I think the question here is whether we want all implementations to show a > > circle for touch feedback and disallow any other forms of feedback in the > > future. > > My understanding of Lambros' point was that if you had an enum which had 'small' > and 'large' types then it's possible that a future implementation could > interpret these values differently. I think it's user-input based. Something like "long-press in trackpad mode" rather than something like "small circle". I've fixed this in a previous CL. > I think that exposing a method which > receives a float param called diameter and a position is just as inflexible > (i.e. if we decided to use an audio tone (for example) as feedback, you'd need > another method anyway). Yep. That's what I try to avoid. > My point is why should any of the classes other than the renderer care how big > the radius is. From an input strategy standpoint, they are defining the type of > animation which should be rendered. The renderer knows how big to draw it. Yep. That's why I ask whether I should move this class out of RenderStub... > Wouldn't it be simpler to say something like: drawAnimation(center_point, > SMALL_TOUCH_ANIMATION) and have the renderer figure out size. Later we could do > some thing like drawAnimation(center_point, BLUE_ALPAH_SQUARE) and the renderer > would take care of it. Acknowledged. So... Just move this class out of RenderStub then we are done? I think a future implementation may still be interested in reusing this class so it may be better not to be a subclass of GlDisplay...
https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:44: mDisplay.showInputFeedback(feedbackToShow, pos); On 2016/08/31 00:15:56, Yuwei wrote: > On 2016/08/30 23:15:03, Hzj_jie wrote: > > I believe you will remove this and following four functions in the coming > > change. > > Yep. The whole GlDesktopView class will go away. > > > 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. > > 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 :) > > > 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. > > Acknowledged. > > > 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. > > 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. > > > 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. > > Not sure whether I understand this well... Isn't it f(a, b, c) { g.f(a, b, c); } > ? > > > This pattern can also help the tests, i.e. you can fake an event provider > > (TouchInputHandler in this case), and let GlDisplay consume it. > > For testing GlDisplay if I already have access to all its functions then why do > I need to fake TouchInputHandler? > > > What do you think? > > Feel slightly mixed. I can try to make a prototype and see how that works. > > > p.s. I have gone through your design doc, but it seems you have not mentioned > > this. > > Yes I just tried to hold off this change since this involves fundamental change > of TouchInputHandler and its member. 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. https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: Event<SizeChangedEventParameter> onClientSizeChanged(); On 2016/08/31 00:15:56, Yuwei wrote: > On 2016/08/30 23:15:03, Hzj_jie wrote: > > According to the following implementation, I believe, same as onTouch event, > > this event should belong to DesktopView, but not GlDisplay or RenderStub. > > In the future I will have surfaceHolder.addCallback(this) in setDesktopView and > drop the SurfaceHolder.Callback implementation in DesktopView. > > onTouch is different since it comes from the View base class. Any benefit for this change? SurfaceHolder can only be accessed from View.getHolder().
https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:44: mDisplay.showInputFeedback(feedbackToShow, pos); On 2016/08/31 00:31:57, Hzj_jie wrote: > On 2016/08/31 00:15:56, Yuwei wrote: > > On 2016/08/30 23:15:03, Hzj_jie wrote: > > > I believe you will remove this and following four functions in the coming > > > change. > > > > Yep. The whole GlDesktopView class will go away. > > > > > 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. > > > > 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 :) > > > > > 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. > > > > Acknowledged. > > > > > 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. > > > > 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. > > > > > 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. > > > > Not sure whether I understand this well... Isn't it f(a, b, c) { g.f(a, b, c); > } > > ? > > > > > This pattern can also help the tests, i.e. you can fake an event provider > > > (TouchInputHandler in this case), and let GlDisplay consume it. > > > > For testing GlDisplay if I already have access to all its functions then why > do > > I need to fake TouchInputHandler? > > > > > What do you think? > > > > Feel slightly mixed. I can try to make a prototype and see how that works. > > > > > p.s. I have gone through your design doc, but it seems you have not > mentioned > > > this. > > > > Yes I just tried to hold off this change since this involves fundamental > change > > of TouchInputHandler and its member. > > 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. Thanks! It looks like A RenderStub to events adapter? https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: Event<SizeChangedEventParameter> onClientSizeChanged(); On 2016/08/31 00:31:57, Hzj_jie wrote: > On 2016/08/31 00:15:56, Yuwei wrote: > > On 2016/08/30 23:15:03, Hzj_jie wrote: > > > According to the following implementation, I believe, same as onTouch event, > > > this event should belong to DesktopView, but not GlDisplay or RenderStub. > > > > In the future I will have surfaceHolder.addCallback(this) in setDesktopView > and > > drop the SurfaceHolder.Callback implementation in DesktopView. > > > > onTouch is different since it comes from the View base class. > > Any benefit for this change? SurfaceHolder can only be accessed from > View.getHolder(). GlDisplay needs to be notified when the surface is created/changed/destroyed by all means. Also onClientSizeChanged() is triggered by the surface holder callback and onHostSizeChanged() is triggered by the native code. It may look weird if you don't them in the same interface...
LGTM https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: Event<SizeChangedEventParameter> onClientSizeChanged(); On 2016/08/31 00:47:40, Yuwei wrote: > On 2016/08/31 00:31:57, Hzj_jie wrote: > > On 2016/08/31 00:15:56, Yuwei wrote: > > > On 2016/08/30 23:15:03, Hzj_jie wrote: > > > > According to the following implementation, I believe, same as onTouch > event, > > > > this event should belong to DesktopView, but not GlDisplay or RenderStub. > > > > > > In the future I will have surfaceHolder.addCallback(this) in setDesktopView > > and > > > drop the SurfaceHolder.Callback implementation in DesktopView. > > > > > > onTouch is different since it comes from the View base class. > > > > Any benefit for this change? SurfaceHolder can only be accessed from > > View.getHolder(). > > GlDisplay needs to be notified when the surface is created/changed/destroyed by > all means. Also onClientSizeChanged() is triggered by the surface holder > callback and onHostSizeChanged() is triggered by the native code. It may look > weird if you don't them in the same interface... My only concern is SurfaceHolder has a same lifetime as View, listening to a dead object does not really take effect. I do not have a strong opinion here.
On 2016/08/31 00:59:21, Hzj_jie wrote: > LGTM > > https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... > File remoting/android/java/src/org/chromium/chromoting/RenderStub.java (right): > > https://codereview.chromium.org/2282783003/diff/40001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/RenderStub.java:48: > Event<SizeChangedEventParameter> onClientSizeChanged(); > On 2016/08/31 00:47:40, Yuwei wrote: > > On 2016/08/31 00:31:57, Hzj_jie wrote: > > > On 2016/08/31 00:15:56, Yuwei wrote: > > > > On 2016/08/30 23:15:03, Hzj_jie wrote: > > > > > According to the following implementation, I believe, same as onTouch > > event, > > > > > this event should belong to DesktopView, but not GlDisplay or > RenderStub. > > > > > > > > In the future I will have surfaceHolder.addCallback(this) in > setDesktopView > > > and > > > > drop the SurfaceHolder.Callback implementation in DesktopView. > > > > > > > > onTouch is different since it comes from the View base class. > > > > > > Any benefit for this change? SurfaceHolder can only be accessed from > > > View.getHolder(). > > > > GlDisplay needs to be notified when the surface is created/changed/destroyed > by > > all means. Also onClientSizeChanged() is triggered by the surface holder > > callback and onHostSizeChanged() is triggered by the native code. It may look > > weird if you don't them in the same interface... > > My only concern is SurfaceHolder has a same lifetime as View, Yep. Or to be precise the lifetime of the surface is in between the lifetime of the View. > listening to a dead object does not really take effect. GlDisplay will do its clean up in onSurfaceDestroyed() so presumably it won't listen to a dead object? > I do not have a strong opinion here.
I've discussed the refactoring plan with zijiehe@. He is planning to replace TouchInputHandler with a class that has a set of rendering events. When this happens, we can make GlDisplay registers itself to the events of the new class and we can probably remove the RenderStub interface. I think for now it's good to have the RenderStub interface so that I can easily remove GlDesktopView. WDYT+PTAL O_o
On 2016/09/01 01:04:03, Yuwei wrote: > I've discussed the refactoring plan with zijiehe@. > > He is planning to replace TouchInputHandler with a class that has a set of > rendering events. When this happens, we can make GlDisplay registers itself to > the events of the new class and we can probably remove the RenderStub interface. > > I think for now it's good to have the RenderStub interface so that I can easily > remove GlDesktopView. > > WDYT+PTAL O_o I think RenderStub can be kept, though in my opinion, it's more likely to be a DesktopView. The event pattern I suggested is to convert an interface from, 'what you can tell me to do' into 'what I can do by myself'. Use RenderStub as an example, it can change the client / host size, and render canvas, so it will export three events onClientSizeChanged / onHostSizeChanged / onCanvasRendered, but not other functions. This pattern can also apply to the TouchInputHandler, (or whatever we replace it later) i.e. it can notify a feedback, change mouse position, etc. Some other components, say, GlDisplay may respond on some of its events. But TouchInputHandler does not need to know all the components listening to its events. Current interface pattern is not good for some of the logic, as interface implementation and consumer need to know each other. i.e. An interface implementation expects the consumer to execute its functions in an expected order, otherwise it may have unexpected behavior. You may use AnimationJob as an example, the interface requires its consumers to call processAnimation() until it returns false. This seems a red flag to me, i.e. how frequent should I call this function? If I forgot to call it for a long time, what will happen? If I call processAnimation() after it returns false, what will happen?
On 2016/09/01 01:25:09, Hzj_jie wrote: > On 2016/09/01 01:04:03, Yuwei wrote: > > I've discussed the refactoring plan with zijiehe@. > > > > He is planning to replace TouchInputHandler with a class that has a set of > > rendering events. When this happens, we can make GlDisplay registers itself to > > the events of the new class and we can probably remove the RenderStub > interface. > > > > I think for now it's good to have the RenderStub interface so that I can > easily > > remove GlDesktopView. > > > > WDYT+PTAL O_o > > I think RenderStub can be kept, though in my opinion, it's more likely to be a > DesktopView. > > The event pattern I suggested is to convert an interface from, 'what you can > tell me to do' into 'what I can do by myself'. Use RenderStub as an example, it > can change the client / host size, and render canvas, so it will export three > events onClientSizeChanged / onHostSizeChanged / onCanvasRendered, but not other > functions. Yes. But that should become "RenderCallback" rather than a "RenderStub". > This pattern can also apply to the TouchInputHandler, (or whatever we replace it > later) i.e. it can notify a feedback, change mouse position, etc. Some other > components, say, GlDisplay may respond on some of its events. But > TouchInputHandler does not need to know all the components listening to its > events. > > Current interface pattern is not good for some of the logic, as interface > implementation and consumer need to know each other. i.e. An interface > implementation expects the consumer to execute its functions in an expected > order, otherwise it may have unexpected behavior. > > You may use AnimationJob as an example, the interface requires its consumers to > call processAnimation() until it returns false. This seems a red flag to me, > i.e. how frequent should I call this function? If I forgot to call it for a long > time, what will happen? If I call processAnimation() after it returns false, > what will happen?
On 2016/09/01 02:05:04, Yuwei wrote: > On 2016/09/01 01:25:09, Hzj_jie wrote: > > On 2016/09/01 01:04:03, Yuwei wrote: > > > I've discussed the refactoring plan with zijiehe@. > > > > > > He is planning to replace TouchInputHandler with a class that has a set of > > > rendering events. When this happens, we can make GlDisplay registers itself > to > > > the events of the new class and we can probably remove the RenderStub > > interface. > > > > > > I think for now it's good to have the RenderStub interface so that I can > > easily > > > remove GlDesktopView. > > > > > > WDYT+PTAL O_o > > > > I think RenderStub can be kept, though in my opinion, it's more likely to be a > > DesktopView. > > > > The event pattern I suggested is to convert an interface from, 'what you can > > tell me to do' into 'what I can do by myself'. Use RenderStub as an example, > it > > can change the client / host size, and render canvas, so it will export three > > events onClientSizeChanged / onHostSizeChanged / onCanvasRendered, but not > other > > functions. > > Yes. But that should become "RenderCallback" rather than a "RenderStub". > > > This pattern can also apply to the TouchInputHandler, (or whatever we replace > it > > later) i.e. it can notify a feedback, change mouse position, etc. Some other > > components, say, GlDisplay may respond on some of its events. But > > TouchInputHandler does not need to know all the components listening to its > > events. > > > > Current interface pattern is not good for some of the logic, as interface > > implementation and consumer need to know each other. i.e. An interface > > implementation expects the consumer to execute its functions in an expected > > order, otherwise it may have unexpected behavior. > > > > You may use AnimationJob as an example, the interface requires its consumers > to > > call processAnimation() until it returns false. This seems a red flag to me, > > i.e. how frequent should I call this function? If I forgot to call it for a > long > > time, what will happen? If I call processAnimation() after it returns false, > > what will happen? Ping
On 2016/09/02 18:00:03, Yuwei wrote: > On 2016/09/01 02:05:04, Yuwei wrote: > > On 2016/09/01 01:25:09, Hzj_jie wrote: > > > On 2016/09/01 01:04:03, Yuwei wrote: > > > > I've discussed the refactoring plan with zijiehe@. > > > > > > > > He is planning to replace TouchInputHandler with a class that has a set of > > > > rendering events. When this happens, we can make GlDisplay registers > itself > > to > > > > the events of the new class and we can probably remove the RenderStub > > > interface. > > > > > > > > I think for now it's good to have the RenderStub interface so that I can > > > easily > > > > remove GlDesktopView. > > > > > > > > WDYT+PTAL O_o > > > > > > I think RenderStub can be kept, though in my opinion, it's more likely to be > a > > > DesktopView. > > > > > > The event pattern I suggested is to convert an interface from, 'what you can > > > tell me to do' into 'what I can do by myself'. Use RenderStub as an example, > > it > > > can change the client / host size, and render canvas, so it will export > three > > > events onClientSizeChanged / onHostSizeChanged / onCanvasRendered, but not > > other > > > functions. > > > > Yes. But that should become "RenderCallback" rather than a "RenderStub". > > > > > This pattern can also apply to the TouchInputHandler, (or whatever we > replace > > it > > > later) i.e. it can notify a feedback, change mouse position, etc. Some other > > > components, say, GlDisplay may respond on some of its events. But > > > TouchInputHandler does not need to know all the components listening to its > > > events. > > > > > > Current interface pattern is not good for some of the logic, as interface > > > implementation and consumer need to know each other. i.e. An interface > > > implementation expects the consumer to execute its functions in an expected > > > order, otherwise it may have unexpected behavior. > > > > > > You may use AnimationJob as an example, the interface requires its consumers > > to > > > call processAnimation() until it returns false. This seems a red flag to me, > > > i.e. how frequent should I call this function? If I forgot to call it for a > > long > > > time, what will happen? If I call processAnimation() after it returns false, > > > what will happen? > > Ping Sorry, I have not realized this is a question. I think stub is still a good name for this interface. This interface exports events, instead of callbacks.
@joedow Ping O_o?
lgtm
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zijiehe@chromium.org Link to the patchset: https://codereview.chromium.org/2282783003/#ps100001 (title: "Merge ToT again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Create Interfaces for GlDisplay This CL adds RenderStub and RenderCallback interface for GlDisplay in preparation of moving render controlling logic out of DesktopView and removing GlDesktopView. BUG=641123 ========== to ========== [Remoting Android] Create Interfaces for GlDisplay This CL adds RenderStub and RenderCallback interface for GlDisplay in preparation of moving render controlling logic out of DesktopView and removing GlDesktopView. BUG=641123 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Create Interfaces for GlDisplay This CL adds RenderStub and RenderCallback interface for GlDisplay in preparation of moving render controlling logic out of DesktopView and removing GlDesktopView. BUG=641123 ========== to ========== [Remoting Android] Create Interfaces for GlDisplay This CL adds RenderStub and RenderCallback interface for GlDisplay in preparation of moving render controlling logic out of DesktopView and removing GlDesktopView. BUG=641123 Committed: https://crrev.com/0c198c51b1a159bc0354106588ef54903e4d818c Cr-Commit-Position: refs/heads/master@{#416415} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0c198c51b1a159bc0354106588ef54903e4d818c Cr-Commit-Position: refs/heads/master@{#416415} |