Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2378303002:
Adjust viewport center when in trackpad input mode. (Closed)
DescriptionAdjust viewport center when in trackpad input mode.
This change adjusts the center of the viewport when user is using
trackpad mode. Without this change, large System UI (such as soft input
methods) can obscure the cursor and prevent the user from entering text
in edit controls near the bottom of the screen.
This change addresses this by shifting the cursor position and viewport
up by 50% of the total viewport size, ensuring it is placed near the
mid-point of the viewable area.
BUG=621633
Committed: https://crrev.com/03431887b157c8db6fb738abdae93c7463f4ba3b
Cr-Commit-Position: refs/heads/master@{#423251}
Patch Set 1 #Patch Set 2 : Merging with changes in dependent CL #Patch Set 3 : Merging with ToT #
Total comments: 10
Patch Set 4 : Addressing CR feedback #Patch Set 5 : Removing the float comparison and making the logic for applying the cursor offset a bit cleaner #
Depends on Patchset: Messages
Total messages: 36 (26 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
I'm sending this out before the dependent CL is LGTM'd so I can receive feedback early and push to get this in before branch point (Thursday). PTAL! Thanks, Joe
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 checked by joedow@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Merged changes with ToT, PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2378303002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2378303002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:30: * Represents the actual center of the viewport in image space. This value needs to be a pair It sounds like this is no longer accurate when mCursorOffsetScreenY != 0. https://codereview.chromium.org/2378303002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:138: // Adjust the cursor position to ensure it visible when large System UI (defined as 1/3 or s/it/it is/ https://codereview.chromium.org/2378303002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:143: // Center the cursor within the viewable area (not obscurred by System UI). obscured https://codereview.chromium.org/2378303002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:150: setCursorPosition(mCursorPosition.x, mCursorPosition.y); This feels suspect. Why do you need this call only conditionally? https://codereview.chromium.org/2378303002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:258: // Translate the image so the viewport center is displayed in the middle of the screen. I guess "middle of the screen" is no longer accurate?
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...
Addressed feedback, PTAL! https://codereview.chromium.org/2378303002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2378303002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:30: * Represents the actual center of the viewport in image space. This value needs to be a pair On 2016/10/05 01:35:54, Lambros wrote: > It sounds like this is no longer accurate when mCursorOffsetScreenY != 0. This is still accurate. When we manipulate the viewport and cursor position, this value is consistent and not modified by mCursorOffsetScreenY. https://codereview.chromium.org/2378303002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:138: // Adjust the cursor position to ensure it visible when large System UI (defined as 1/3 or On 2016/10/05 01:35:54, Lambros wrote: > s/it/it is/ Done. https://codereview.chromium.org/2378303002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:143: // Center the cursor within the viewable area (not obscurred by System UI). On 2016/10/05 01:35:54, Lambros wrote: > obscured Done. https://codereview.chromium.org/2378303002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:150: setCursorPosition(mCursorPosition.x, mCursorPosition.y); On 2016/10/05 01:35:54, Lambros wrote: > This feels suspect. Why do you need this call only conditionally? It only affects the case where the cursor offset changes. It's basically an optimization for Touch mode. I'll add a comment. https://codereview.chromium.org/2378303002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:258: // Translate the image so the viewport center is displayed in the middle of the screen. On 2016/10/05 01:35:54, Lambros wrote: > I guess "middle of the screen" is no longer accurate? That's true, this comment should be updated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
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.
lgtm
Thanks!
The CQ bit was checked by joedow@chromium.org
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.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Adjust viewport center when in trackpad input mode. This change adjusts the center of the viewport when user is using trackpad mode. Without this change, large System UI (such as soft input methods) can obscure the cursor and prevent the user from entering text in edit controls near the bottom of the screen. This change addresses this by shifting the cursor position and viewport up by 50% of the total viewport size, ensuring it is placed near the mid-point of the viewable area. BUG=621633 ========== to ========== Adjust viewport center when in trackpad input mode. This change adjusts the center of the viewport when user is using trackpad mode. Without this change, large System UI (such as soft input methods) can obscure the cursor and prevent the user from entering text in edit controls near the bottom of the screen. This change addresses this by shifting the cursor position and viewport up by 50% of the total viewport size, ensuring it is placed near the mid-point of the viewable area. BUG=621633 Committed: https://crrev.com/03431887b157c8db6fb738abdae93c7463f4ba3b Cr-Commit-Position: refs/heads/master@{#423251} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/03431887b157c8db6fb738abdae93c7463f4ba3b Cr-Commit-Position: refs/heads/master@{#423251} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
