Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2372663002:
Separating cursor and viewport calculations in the desktop canvas (Closed)
DescriptionSeparating cursor and viewport calculations in the desktop canvas
This change simplifies the code which handles positioning and
determining where the center of the viewport on the DesktopActivity
should be. There is a small benefit now, but it makes deaing with
additional image padding much simpler (allowing the canvas to be pulled
from underneath System UI elements) in a future CL.
BUG=621633
Committed: https://crrev.com/0eb9ad0afea07802c3c426001df2c5a0b1946e3d
Cr-Commit-Position: refs/heads/master@{#421365}
Patch Set 1 #Patch Set 2 : Merging with ToT and doing some pre-review clean-up #
Total comments: 16
Patch Set 3 : Updating viewport bound calcs and addressing other feedback #
Total comments: 5
Patch Set 4 : Addressing feedback #
Dependent Patchsets: Messages
Total messages: 30 (20 generated)
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
joedow@chromium.org changed reviewers: + lambroslambrou@chromium.org
PTAL!
https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:55: public void setViewportCenter(float newX, float newY) { This should probably be called setCursorAndViewportPositions(). It sets both mCursorPosition and mViewportPosition to the given point. Then it constrains both values to lie within their allowed regions. The doc for this public method should make it clear that the viewport is not actually moved (the matrix is not transformed). This breaks the claimed attribute of mViewportPosition being the actual center of the viewport. https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:71: public void moveViewportCenter(float deltaX, float deltaY) { Clarify this does not actually move the viewport - the matrix is not changed. Maybe name it offsetViewportAndSetCursorPosition()? And reserve the word 'move' for methods that transform the matrix? (see moveViewportToPoint() which does transform the matrix, but leaves mViewportPosition untouched) https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:88: public PointF moveViewportWithCursor(float deltaX, float deltaY) { Maybe offsetCursorAndSetViewportPosition() ? https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:132: // The goal of the code below is to position the viewport as close to the desired center The phrase 'desired center' is how you've described mCursorPosition. However, the code below uses mViewportPosition. https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:136: moveViewportToPoint(mViewportPosition); This call modifies the transformation matrix so that mViewportPosition is the actual center of the viewport. This implies that previously it was not, which implies that this method acts to restore a consistent internal state. https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:137: translateImageToFitScreen(); But this call will modify the matrix *without* updating mViewportPosition, breaking the internal state again. Except that it actually doesn't (?) modify the matrix, because mViewportPosition is constrained to some bounds that make this a no-op. https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:221: private RectF getViewportBounds() { IIUC, this is precisely the set of points for which translateImageToFitScreen() would be a no-op if the viewport were already centered on the point. I think this fact is crucial to understanding where you're going with this CL :) I think you're headed for a place where translateImageToFitScreen() is always a no-op so it can be removed completely? https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:279: mRenderData.transform.postTranslate(xAdjust, yAdjust); By actually transforming the matrix here, you break the claimed attribute of mViewportPosition being the center of the viewport. But as noted elsewhere, I don't think the matrix actually gets transformed here - the adjustments will end up being zero always.
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL! https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:55: public void setViewportCenter(float newX, float newY) { On 2016/09/27 02:16:37, Lambros wrote: > This should probably be called setCursorAndViewportPositions(). > It sets both mCursorPosition and mViewportPosition to the given point. > Then it constrains both values to lie within their allowed regions. > > The doc for this public method should make it clear that the viewport is > not actually moved (the matrix is not transformed). This breaks the claimed > attribute of mViewportPosition being the actual center of the viewport. Updating the name is fine, but I'd prefer to name it based on the intention of the caller (in this case they are setting the cursor position which also adjusts the viewport.) I didn't change the calling pattern in my original CL (i.e. callers always called repositionImage() after calling this method), but the second comment no longer applies as I've made additional changes. https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:71: public void moveViewportCenter(float deltaX, float deltaY) { On 2016/09/27 02:16:37, Lambros wrote: > Clarify this does not actually move the viewport - the matrix is not changed. > Maybe name it offsetViewportAndSetCursorPosition()? > And reserve the word 'move' for methods that transform the matrix? > (see moveViewportToPoint() which does transform the matrix, but leaves > mViewportPosition untouched) Acknowledged. https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:88: public PointF moveViewportWithCursor(float deltaX, float deltaY) { On 2016/09/27 02:16:36, Lambros wrote: > Maybe offsetCursorAndSetViewportPosition() ? Renamed based on intention of the caller. https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:132: // The goal of the code below is to position the viewport as close to the desired center On 2016/09/27 02:16:36, Lambros wrote: > The phrase 'desired center' is how you've described mCursorPosition. However, > the code below uses mViewportPosition. Acknowledged. https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:136: moveViewportToPoint(mViewportPosition); On 2016/09/27 02:16:37, Lambros wrote: > This call modifies the transformation matrix so that mViewportPosition is the > actual center of the viewport. This implies that previously it was not, which > implies that this > method acts to restore a consistent internal state. Acknowledged. https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:137: translateImageToFitScreen(); On 2016/09/27 02:16:37, Lambros wrote: > But this call will modify the matrix *without* updating mViewportPosition, > breaking the internal state again. > > Except that it actually doesn't (?) modify the matrix, because mViewportPosition > is > constrained to some bounds that make this a no-op. Acknowledged. https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:221: private RectF getViewportBounds() { On 2016/09/27 02:16:37, Lambros wrote: > IIUC, this is precisely the set of points for which translateImageToFitScreen() > would be a no-op if the viewport were already centered on the point. > > I think this fact is crucial to understanding where you're going with this CL :) > I think you're headed for a place where translateImageToFitScreen() is always a > no-op so it can be removed completely? I was moving to a place where multiple transforms weren't needed as the coordinates would never be messed up. I think the diffing tool made the change seem much larger than it actually was but I've done a bit more work just to get rid of the need for the additional transformations in this CL. https://codereview.chromium.org/2372663002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:279: mRenderData.transform.postTranslate(xAdjust, yAdjust); On 2016/09/27 02:16:37, Lambros wrote: > By actually transforming the matrix here, you break the claimed attribute of > mViewportPosition being the center of the viewport. But as noted elsewhere, I > don't > think the matrix actually gets transformed here - the adjustments will end up > being zero always. Acknowledged.
lgtm, thanks! https://codereview.chromium.org/2372663002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2372663002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:249: float screenCenterX = (screenVectors[0] / 2.0f); nit: () not needed.
yuweih@chromium.org changed reviewers: + yuweih@chromium.org
https://codereview.chromium.org/2372663002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2372663002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:81: mCursorPosition.set(mViewportPosition); Just curious that why do we need to move the cursor position here? I think it makes sense to move the viewport center when the cursor position is moved so that the viewport can follow the cursor but the other way around might look a little bit weird to me... Constraining the old cursor position to the viewport bound makes sense though.
Description was changed from ========== Separating cursor and viewport calculations in the desktop canvas This change simplifies the code which handles positioning and determining where the center of the viewport on the DesktopActivity should be. There is a small benefit now, but it makes deaing with additional image padding much simpler (allowing the canvas to be pulled from underneath System UI elements) in a future CL. BUG=621633 ========== to ========== Separating cursor and viewport calculations in the desktop canvas This change simplifies the code which handles positioning and determining where the center of the viewport on the DesktopActivity should be. There is a small benefit now, but it makes deaing with additional image padding much simpler (allowing the canvas to be pulled from underneath System UI elements) in a future CL. BUG=621633 ==========
yuweih@chromium.org changed reviewers: - yuweih@chromium.org
Thanks! https://codereview.chromium.org/2372663002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2372663002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:81: mCursorPosition.set(mViewportPosition); On 2016/09/27 22:01:55, Yuwei wrote: > Just curious that why do we need to move the cursor position here? I think it > makes sense to move the viewport center when the cursor position is moved so > that the viewport can follow the cursor but the other way around might look a > little bit weird to me... Constraining the old cursor position to the viewport > bound makes sense though. I wanted to keep the Cursor and Viewport in sync to demonstrate the relationship between the two in code. You are right that it isn't strictly necessary. https://codereview.chromium.org/2372663002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:249: float screenCenterX = (screenVectors[0] / 2.0f); On 2016/09/27 21:32:55, Lambros wrote: > nit: () not needed. Done.
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org Link to the patchset: https://codereview.chromium.org/2372663002/#ps60001 (title: "Addressing feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2372663002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2372663002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:81: mCursorPosition.set(mViewportPosition); On 2016/09/27 22:23:37, joedow wrote: > On 2016/09/27 22:01:55, Yuwei wrote: > > Just curious that why do we need to move the cursor position here? I think it > > makes sense to move the viewport center when the cursor position is moved so > > that the viewport can follow the cursor but the other way around might look a > > little bit weird to me... Constraining the old cursor position to the viewport > > bound makes sense though. > > I wanted to keep the Cursor and Viewport in sync to demonstrate the relationship > between the two in code. You are right that it isn't strictly necessary. Acknowledged.
Message was sent while issue was closed.
Description was changed from ========== Separating cursor and viewport calculations in the desktop canvas This change simplifies the code which handles positioning and determining where the center of the viewport on the DesktopActivity should be. There is a small benefit now, but it makes deaing with additional image padding much simpler (allowing the canvas to be pulled from underneath System UI elements) in a future CL. BUG=621633 ========== to ========== Separating cursor and viewport calculations in the desktop canvas This change simplifies the code which handles positioning and determining where the center of the viewport on the DesktopActivity should be. There is a small benefit now, but it makes deaing with additional image padding much simpler (allowing the canvas to be pulled from underneath System UI elements) in a future CL. BUG=621633 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Separating cursor and viewport calculations in the desktop canvas This change simplifies the code which handles positioning and determining where the center of the viewport on the DesktopActivity should be. There is a small benefit now, but it makes deaing with additional image padding much simpler (allowing the canvas to be pulled from underneath System UI elements) in a future CL. BUG=621633 ========== to ========== Separating cursor and viewport calculations in the desktop canvas This change simplifies the code which handles positioning and determining where the center of the viewport on the DesktopActivity should be. There is a small benefit now, but it makes deaing with additional image padding much simpler (allowing the canvas to be pulled from underneath System UI elements) in a future CL. BUG=621633 Committed: https://crrev.com/0eb9ad0afea07802c3c426001df2c5a0b1946e3d Cr-Commit-Position: refs/heads/master@{#421365} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0eb9ad0afea07802c3c426001df2c5a0b1946e3d Cr-Commit-Position: refs/heads/master@{#421365} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
