Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2441723004:
[Android][Client] Updating the calculations used for image overpanning (Closed)
DescriptionUpdating the calculations used for image overpanning
This CL simplifies the calculations used for panning the remote desktop
image out from under the System UI. It also constrains the cursor to
the image coordinates vs. the previous method which did not.
The old approach I used was to allow overpanning the remote desktop
image by adjusting the boundaries of the image. This worked reasonably
well but had one side effect for cursor mode as it meant the cursor
arrow could be rendered outside of the actual image dimensions.
Functionally this was fine as it was still constrained on the remote
system but visually it was not optimal.
The new approach is to use accurate boundaries for the cursor and
viewport but instead accumulate an offset value (mViewportOffset) when
the user tries to 'overpan' the image past its boundaries. The effect
is that the cursor is still constrained to the image but we can move the
image further in either dimension to ensure they can interact with it.
Once the System UI disappears, we run a per-frame animation to reduce
the offset and reposition/render the remote image. The effect is that
the image slides back into view quickly which is much cleaner than the
snapping effect we would have if we allowed Android to reposition the
view.
BUG=653309
Committed: https://crrev.com/95ccb26c84a537f696b374097f7050d472fba289
Cr-Commit-Position: refs/heads/master@{#427462}
Patch Set 1 #Patch Set 2 : Pre-CR cleanup #
Total comments: 18
Patch Set 3 : Addressing CR Feedback #
Total comments: 8
Patch Set 4 : Addressing feedback #
Total comments: 8
Patch Set 5 : Addressing feedback and merging with ToT #Messages
Total messages: 40 (28 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.
Description was changed from ========== The new style... BUG=653309 ========== to ========== Updating the calculations used for image overpanning This CL simplifies the calculations used for panning the remote desktop image out from under the System UI. It also constrains the cursor to the image coordinates vs. the previous method which did not. The old approach I used was to allow overpanning the remote desktop image by adjusting the boundaries of the image. This worked reasonably well but had one side effect for cursor mode as it meant the cursor arrow could be rendered outside of the actual image dimensions. Functionally this was fine as it was still constrained on the remote system but visually it was not optimal. The new approach is to use accurate boundaries for the cursor and viewport but instead accumulate an offset value (mViewportOffset) when the user tries to 'overpan' the image past its boundaries. The effect is that the cursor is still constrained to the image but we can move the image further in either dimension to ensure they can interact with it. Once the System UI disappears, we run a per-frame animation to reduce the offset and reposition/render the remote image. The effect is that the image slides back into view quickly which is much cleaner than the snapping effect we would have if we allowed Android to reposition the view. BUG=653309 ==========
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...
Description was changed from ========== Updating the calculations used for image overpanning This CL simplifies the calculations used for panning the remote desktop image out from under the System UI. It also constrains the cursor to the image coordinates vs. the previous method which did not. The old approach I used was to allow overpanning the remote desktop image by adjusting the boundaries of the image. This worked reasonably well but had one side effect for cursor mode as it meant the cursor arrow could be rendered outside of the actual image dimensions. Functionally this was fine as it was still constrained on the remote system but visually it was not optimal. The new approach is to use accurate boundaries for the cursor and viewport but instead accumulate an offset value (mViewportOffset) when the user tries to 'overpan' the image past its boundaries. The effect is that the cursor is still constrained to the image but we can move the image further in either dimension to ensure they can interact with it. Once the System UI disappears, we run a per-frame animation to reduce the offset and reposition/render the remote image. The effect is that the image slides back into view quickly which is much cleaner than the snapping effect we would have if we allowed Android to reposition the view. BUG=653309 ========== to ========== Updating the calculations used for image overpanning This CL simplifies the calculations used for panning the remote desktop image out from under the System UI. It also constrains the cursor to the image coordinates vs. the previous method which did not. The old approach I used was to allow overpanning the remote desktop image by adjusting the boundaries of the image. This worked reasonably well but had one side effect for cursor mode as it meant the cursor arrow could be rendered outside of the actual image dimensions. Functionally this was fine as it was still constrained on the remote system but visually it was not optimal. The new approach is to use accurate boundaries for the cursor and viewport but instead accumulate an offset value (mViewportOffset) when the user tries to 'overpan' the image past its boundaries. The effect is that the cursor is still constrained to the image but we can move the image further in either dimension to ensure they can interact with it. Once the System UI disappears, we run a per-frame animation to reduce the offset and reposition/render the remote image. The effect is that the image slides back into view quickly which is much cleaner than the snapping effect we would have if we allowed Android to reposition the view. BUG=653309 ==========
joedow@chromium.org changed reviewers: + lambroslambrou@chromium.org, yuweih@chromium.org
joedow@chromium.org changed required reviewers: + lambroslambrou@chromium.org
PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:16: private static final float EPSILON = 0.0001f; Does this value need to be different from MINIMUM_OFFSET_DISTANCE? If they are different, you will end up starting the animation whenever length() lies between these values, and it will run for just one frame. https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:25: private static final float MINIMUM_OFFSET_DISTANCE = 1; This is in screen coordinates? https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:120: if (mFrameRenderedCallback != null) { Please add a comment, or move this to a method called stopOffsetReductionAnimation(). https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:232: adjustedScreenHeight = mSystemUiScreenSize.bottom; I don't follow this. For example, if the system keyboard were 1 pixel thick (silly, but bear with me), then mSystemUiScreenSize.bottom would be 1, and the other values would be 0. This code would set adjustedScreenHeight equal to 1. Then if the system keyboard disappeared to 0 pixels, the isEmpty() above would be true, then the 'else' block would execute, and adjustedScreenHeight would suddenly jump from 1 to mRenderDate.screenHeight. https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:330: new RectF(Math.min(mViewportOffset.x, 0.0f), Math.min(mViewportOffset.y, 0.0f), Would new RectF().union(mViewportOffset.x, mViewportOffset.y) work here? https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:361: return new RectF(mSystemUiScreenSize); Just 'new RectF()', no need to pass in an empty RectF. https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:364: // letterBox padding represents the space added to the image to center it on the screen. nit: Begin with capital letter, or enclose with '' or ||. https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:383: // We only want to change adjust the viewport offset when System UI is present. Remove 'change' or 'adjust'.
https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:230: if (mAdjustViewportSizeForSystemUi && !mSystemUiScreenSize.isEmpty()) { You're calling isEmpty() on something that isn't a true rectangle. According to the doc, isEmpty() returns true whenever left >= right or top >= bottom, which won't do what you want if the values are offsets.
PTAL! https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:16: private static final float EPSILON = 0.0001f; On 2016/10/24 22:20:33, Lambros wrote: > Does this value need to be different from MINIMUM_OFFSET_DISTANCE? > If they are different, you will end up starting the animation whenever length() > lies between these values, and it will run for just one frame. This is used for floating point comparisons (specifically whether the offset values are 0). This should be different as we should not start an animation if the offset is 0 but the offset could become a value < 1 while the animation is running and we want to terminate it at that point. https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:25: private static final float MINIMUM_OFFSET_DISTANCE = 1; On 2016/10/24 22:20:33, Lambros wrote: > This is in screen coordinates? It is in the same coordinates as mViewportOffset. I've updated the comment and name a bit, but it is essentially the terminating condition for the offset reduction animation. https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:120: if (mFrameRenderedCallback != null) { On 2016/10/24 22:20:33, Lambros wrote: > Please add a comment, or move this to a method called > stopOffsetReductionAnimation(). Done! https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:230: if (mAdjustViewportSizeForSystemUi && !mSystemUiScreenSize.isEmpty()) { On 2016/10/24 22:37:27, Lambros wrote: > You're calling isEmpty() on something that isn't a true rectangle. > According to the doc, isEmpty() returns true whenever left >= right or top >= > bottom, which won't do what you want if the values are offsets. It is an actual Rect (updated based on previous CR feedback from you). I've updated the comments and variable name to reflect that. https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:232: adjustedScreenHeight = mSystemUiScreenSize.bottom; On 2016/10/24 22:20:32, Lambros wrote: > I don't follow this. For example, if the system keyboard were 1 pixel thick > (silly, but bear with me), then mSystemUiScreenSize.bottom would be 1, and the > other values would be 0. This code would set adjustedScreenHeight equal to 1. > > Then if the system keyboard disappeared to 0 pixels, the isEmpty() above would > be true, then the 'else' block would execute, and adjustedScreenHeight would > suddenly jump from 1 to mRenderDate.screenHeight. Since this is a rect, given an example screen height of 800px and a 1px think keyboard, the adjusted screen height would be 799px. https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:330: new RectF(Math.min(mViewportOffset.x, 0.0f), Math.min(mViewportOffset.y, 0.0f), On 2016/10/24 22:20:33, Lambros wrote: > Would > new RectF().union(mViewportOffset.x, mViewportOffset.y) > work here? It does, union returns void though so I can't chain the method call and assignment. https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:361: return new RectF(mSystemUiScreenSize); On 2016/10/24 22:20:33, Lambros wrote: > Just 'new RectF()', no need to pass in an empty RectF. Good point, I think this is left over from a previous change. https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:364: // letterBox padding represents the space added to the image to center it on the screen. On 2016/10/24 22:20:32, Lambros wrote: > nit: Begin with capital letter, or enclose with '' or ||. Done. https://codereview.chromium.org/2441723004/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:383: // We only want to change adjust the viewport offset when System UI is present. On 2016/10/24 22:20:33, Lambros wrote: > Remove 'change' or 'adjust'. Done.
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.
Some drive-by comments... Didn't carefully read how the transformation works so please wait for Lambros' magic word to commit. https://codereview.chromium.org/2441723004/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2441723004/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:37: private PointF mCursorPosition = new PointF(); Maybe not for this CL. If this is the desired center of the viewport, could it be better to call it mDesiredCenter? I think it is slightly weird to introduce the idea of cursor position when changing viewport. https://codereview.chromium.org/2441723004/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:409: mViewportOffset.set(mViewportOffset.x * VIEWPORT_OFFSET_REDUCTION_FACTOR, The timing implicitly depends on the fact that the renderer (mostly. maybe try the animation while playing YouTube video?) renders at 60Hz. I think it will be safer to calculate the offset using time as parameter. BTW have you tried Android's stock interpolators (e.g. DecelerateInterpolator)? https://codereview.chromium.org/2441723004/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:434: mRenderStub.onCanvasRendered().remove(mFrameRenderedCallback); This line of code probably doesn't work. addSelfRemovable wraps the callback with SelfRemovableParameterRunnable and adds it to the set so removing the raw callback probably doesn't work. Setting mFrameRenderedCallback to null should terminate the animation. Null-checking it at the beginning of the callback can stop the animation in time.
The CQ bit was checked by joedow@chromium.org to run a CQ dry run
PTAL! https://codereview.chromium.org/2441723004/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2441723004/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:37: private PointF mCursorPosition = new PointF(); On 2016/10/25 08:25:26, Yuwei wrote: > Maybe not for this CL. If this is the desired center of the viewport, could it > be better to call it mDesiredCenter? I think it is slightly weird to introduce > the idea of cursor position when changing viewport. Lambros and I have talked about this a bit already, I'm fine changing the name in this CL. https://codereview.chromium.org/2441723004/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:409: mViewportOffset.set(mViewportOffset.x * VIEWPORT_OFFSET_REDUCTION_FACTOR, On 2016/10/25 08:25:26, Yuwei wrote: > The timing implicitly depends on the fact that the renderer (mostly. maybe try > the animation while playing YouTube video?) renders at 60Hz. I think it will be > safer to calculate the offset using time as parameter. > > BTW have you tried Android's stock interpolators (e.g. DecelerateInterpolator)? I didn't realize the renderer wasn't running at a consistent framerate, timing based animation SGTM. https://codereview.chromium.org/2441723004/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:434: mRenderStub.onCanvasRendered().remove(mFrameRenderedCallback); On 2016/10/25 08:25:26, Yuwei wrote: > This line of code probably doesn't work. addSelfRemovable wraps the callback > with SelfRemovableParameterRunnable and adds it to the set so removing the raw > callback probably doesn't work. > > Setting mFrameRenderedCallback to null should terminate the animation. > Null-checking it at the beginning of the callback can stop the animation in > time. Thanks for pointing that out, I can see the intention behind this event type but it feels like I should still be able to remove it manually (i.e. outside of the runnable). oh well.
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.
LGTM for my comments. Thanks! https://codereview.chromium.org/2441723004/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2441723004/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:409: mViewportOffset.set(mViewportOffset.x * VIEWPORT_OFFSET_REDUCTION_FACTOR, On 2016/10/25 16:49:50, joedow wrote: > On 2016/10/25 08:25:26, Yuwei wrote: > > The timing implicitly depends on the fact that the renderer (mostly. maybe try > > the animation while playing YouTube video?) renders at 60Hz. I think it will > be > > safer to calculate the offset using time as parameter. > > > > BTW have you tried Android's stock interpolators (e.g. > DecelerateInterpolator)? > > I didn't realize the renderer wasn't running at a consistent framerate, timing > based animation SGTM. 60Hz is more like the highest possible framerate of the renderer. It will drop if the display thread gets blocked (say if uploading desktop image takes too long). https://codereview.chromium.org/2441723004/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:434: mRenderStub.onCanvasRendered().remove(mFrameRenderedCallback); On 2016/10/25 16:49:50, joedow wrote: > On 2016/10/25 08:25:26, Yuwei wrote: > > This line of code probably doesn't work. addSelfRemovable wraps the callback > > with SelfRemovableParameterRunnable and adds it to the set so removing the raw > > callback probably doesn't work. > > > > Setting mFrameRenderedCallback to null should terminate the animation. > > Null-checking it at the beginning of the callback can stop the animation in > > time. > > Thanks for pointing that out, I can see the intention behind this event type but > it feels like I should still be able to remove it manually (i.e. outside of the > runnable). oh well. Agreed. We may want to fix the self removable at some point. We can do something like returning the SelfRemovableRunnable in addSelfRemovable() or playing with the hash and equals functions of the runnable.
lgtm https://codereview.chromium.org/2441723004/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2441723004/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:237: * Returns the center position of the viewport (in screen coordinates) including adjustments. Clarify 'adjustments' by referring to {@link #adjustViewportForSystemUi()} https://codereview.chromium.org/2441723004/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:239: private PointF getViewportCenter() { I think this name is misleading, because this is in screen coordinates whereas moveViewportCenter() is in desktop image coordinates. Maybe call it getViewportScreenCenter() or getAdjustedScreenCenter()? https://codereview.chromium.org/2441723004/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:359: // LetterBox padding represents the space added to the image to center it on the screen. nit: lower-case B. https://codereview.chromium.org/2441723004/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:399: if (mFrameRenderedCallback != null || Math.abs(mViewportOffset.length()) < EPSILON) { Math.abs() is unnecessary - length() is never negative.
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...
Thanks! https://codereview.chromium.org/2441723004/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2441723004/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:237: * Returns the center position of the viewport (in screen coordinates) including adjustments. On 2016/10/25 19:26:31, Lambros wrote: > Clarify 'adjustments' by referring to {@link #adjustViewportForSystemUi()} Done. https://codereview.chromium.org/2441723004/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:239: private PointF getViewportCenter() { On 2016/10/25 19:26:31, Lambros wrote: > I think this name is misleading, because this is in screen coordinates whereas > moveViewportCenter() is in desktop image coordinates. Maybe call it > getViewportScreenCenter() or getAdjustedScreenCenter()? Done. https://codereview.chromium.org/2441723004/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:359: // LetterBox padding represents the space added to the image to center it on the screen. On 2016/10/25 19:26:31, Lambros wrote: > nit: lower-case B. Done. https://codereview.chromium.org/2441723004/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:399: if (mFrameRenderedCallback != null || Math.abs(mViewportOffset.length()) < EPSILON) { On 2016/10/25 19:26:31, Lambros wrote: > Math.abs() is unnecessary - length() is never negative. Good point!
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
The patchset sent to the CQ was uploaded after l-g-t-m from yuweih@chromium.org, lambroslambrou@chromium.org Link to the patchset: https://codereview.chromium.org/2441723004/#ps80001 (title: "Addressing feedback and merging with ToT")
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 ========== Updating the calculations used for image overpanning This CL simplifies the calculations used for panning the remote desktop image out from under the System UI. It also constrains the cursor to the image coordinates vs. the previous method which did not. The old approach I used was to allow overpanning the remote desktop image by adjusting the boundaries of the image. This worked reasonably well but had one side effect for cursor mode as it meant the cursor arrow could be rendered outside of the actual image dimensions. Functionally this was fine as it was still constrained on the remote system but visually it was not optimal. The new approach is to use accurate boundaries for the cursor and viewport but instead accumulate an offset value (mViewportOffset) when the user tries to 'overpan' the image past its boundaries. The effect is that the cursor is still constrained to the image but we can move the image further in either dimension to ensure they can interact with it. Once the System UI disappears, we run a per-frame animation to reduce the offset and reposition/render the remote image. The effect is that the image slides back into view quickly which is much cleaner than the snapping effect we would have if we allowed Android to reposition the view. BUG=653309 ========== to ========== Updating the calculations used for image overpanning This CL simplifies the calculations used for panning the remote desktop image out from under the System UI. It also constrains the cursor to the image coordinates vs. the previous method which did not. The old approach I used was to allow overpanning the remote desktop image by adjusting the boundaries of the image. This worked reasonably well but had one side effect for cursor mode as it meant the cursor arrow could be rendered outside of the actual image dimensions. Functionally this was fine as it was still constrained on the remote system but visually it was not optimal. The new approach is to use accurate boundaries for the cursor and viewport but instead accumulate an offset value (mViewportOffset) when the user tries to 'overpan' the image past its boundaries. The effect is that the cursor is still constrained to the image but we can move the image further in either dimension to ensure they can interact with it. Once the System UI disappears, we run a per-frame animation to reduce the offset and reposition/render the remote image. The effect is that the image slides back into view quickly which is much cleaner than the snapping effect we would have if we allowed Android to reposition the view. BUG=653309 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Updating the calculations used for image overpanning This CL simplifies the calculations used for panning the remote desktop image out from under the System UI. It also constrains the cursor to the image coordinates vs. the previous method which did not. The old approach I used was to allow overpanning the remote desktop image by adjusting the boundaries of the image. This worked reasonably well but had one side effect for cursor mode as it meant the cursor arrow could be rendered outside of the actual image dimensions. Functionally this was fine as it was still constrained on the remote system but visually it was not optimal. The new approach is to use accurate boundaries for the cursor and viewport but instead accumulate an offset value (mViewportOffset) when the user tries to 'overpan' the image past its boundaries. The effect is that the cursor is still constrained to the image but we can move the image further in either dimension to ensure they can interact with it. Once the System UI disappears, we run a per-frame animation to reduce the offset and reposition/render the remote image. The effect is that the image slides back into view quickly which is much cleaner than the snapping effect we would have if we allowed Android to reposition the view. BUG=653309 ========== to ========== Updating the calculations used for image overpanning This CL simplifies the calculations used for panning the remote desktop image out from under the System UI. It also constrains the cursor to the image coordinates vs. the previous method which did not. The old approach I used was to allow overpanning the remote desktop image by adjusting the boundaries of the image. This worked reasonably well but had one side effect for cursor mode as it meant the cursor arrow could be rendered outside of the actual image dimensions. Functionally this was fine as it was still constrained on the remote system but visually it was not optimal. The new approach is to use accurate boundaries for the cursor and viewport but instead accumulate an offset value (mViewportOffset) when the user tries to 'overpan' the image past its boundaries. The effect is that the cursor is still constrained to the image but we can move the image further in either dimension to ensure they can interact with it. Once the System UI disappears, we run a per-frame animation to reduce the offset and reposition/render the remote image. The effect is that the image slides back into view quickly which is much cleaner than the snapping effect we would have if we allowed Android to reposition the view. BUG=653309 Committed: https://crrev.com/95ccb26c84a537f696b374097f7050d472fba289 Cr-Commit-Position: refs/heads/master@{#427462} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/95ccb26c84a537f696b374097f7050d472fba289 Cr-Commit-Position: refs/heads/master@{#427462} |
