Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2812663002:
exo: Fix cursor scale when crossing displays (Closed)
|
Created:
3 years, 8 months ago by Dominik Laskowski Modified:
3 years, 6 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionexo: Fix cursor scale when crossing displays
While dragging, the cursor was locked, so CursorClient::SetCursor calls
were collapsed into a single call at the end of the drag. When crossing
displays with different densities, the cursor scale was incorrect until
the window was dropped. This CL removes cursor locking to prevent the
OnCursorDisplayChanged update from being postponed.
BUG=631136
BUG=642894
TEST=caroline: Cursor scale is correct while dragging across displays.
Review-Url: https://codereview.chromium.org/2812663002
Cr-Commit-Position: refs/heads/master@{#476375}
Committed: https://chromium.googlesource.com/chromium/src/+/fa6afade49688205eb70ba3329a23c91080cf53a
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 18
Patch Set 3 : Rebase #
Total comments: 3
Patch Set 4 : Rebase #Patch Set 5 : Remove cursor hiding #Patch Set 6 : Rebase #Patch Set 7 : Rebase #Messages
Total messages: 27 (7 generated)
domlaskowski@chromium.org changed reviewers: + oshima@chromium.org, reveman@chromium.org
PTAL. Split out from https://codereview.chromium.org/2780623002/
Just looking at the description: "While dragging, the cursor is locked, so CursorClient::SetCursor calls are collapsed into a single call at the end of the drag." What's the problem with that? Isn't that what we want? "When crossing displays, we need to force a capture to update the scale, and hide the old cursor while the capture is pending." Why? Are we not getting an enter event when the grab from the dragging stops and that results in the cursor being properly updated?
On 2017/04/11 16:58:15, reveman wrote: > What's the problem with that? Isn't that what we want? Yes, but we need to update the scale despite that. On 2017/04/11 16:58:15, reveman wrote: > Why? Are we not getting an enter event when the grab from the dragging stops and > that results in the cursor being properly updated? The dragged window has capture, so focus does not change when the mouse crosses a display boundary.
I don't think I understand the problem enough to evaluate this change. Can you describe in more detail what the problem is? Maybe record a video?
On 2017/04/13 19:46:45, reveman wrote: > I don't think I understand the problem enough to evaluate this change. Can you > describe in more detail what the problem is? Maybe record a video? Say you drag a window across displays with different densities. The cursor should be scaled when it crosses the display boundary, but it's locked so the old bitmap is used until the window is dropped. This CL bypasses the lock.
https://codereview.chromium.org/2812663002/diff/20001/components/exo/wm_helpe... File components/exo/wm_helper_ash.cc (right): https://codereview.chromium.org/2812663002/diff/20001/components/exo/wm_helpe... components/exo/wm_helper_ash.cc:133: ash::Shell* shell = ash::Shell::GetInstance(); nit ::Get() https://codereview.chromium.org/2812663002/diff/20001/components/exo/wm_helpe... components/exo/wm_helper_ash.cc:135: ->SetCursor(cursor, shell->cursor_manager()); NativeCursorManager is to provide platform impls, so I'd like to avoid exposing it to client. Just curious, do we need to lock the cursor?
https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:270: // Capture is asynchronous, so avoid rendering old cursor in the meantime. why shouldn't we keep the old cursor until a new one has arrived? https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:273: // Force the cursor to be updated even if locked. Is "force" only needed for the case when we drag an arc window across display boundaries? It's unfortunate that the current multi-display logic requires these kind of special cases. Would it be possible to generalize this a bit. What if we allow the cursor to always be changed if the currently locked cursor is the current cursor for this pointer? https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:339: // Prevent subsequent requests in the same frame from aborting this capture. why is this needed and correct? https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:389: cursor_client->SetCursor(cursor); can we avoid having this line? "force || cursor_client->IsCursorLocked()" below instead?
https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:270: // Capture is asynchronous, so avoid rendering old cursor in the meantime. On 2017/04/26 23:41:26, reveman wrote: > why shouldn't we keep the old cursor until a new one has arrived? Because the old bitmap may have an incorrect scale or rotation for the new display. https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:273: // Force the cursor to be updated even if locked. On 2017/04/26 23:41:26, reveman wrote: > Is "force" only needed for the case when we drag an arc window across display > boundaries? It's unfortunate that the current multi-display logic requires these > kind of special cases. Would it be possible to generalize this a bit. What if we > allow the cursor to always be changed if the currently locked cursor is the > current cursor for this pointer? Right, and it's only needed because the cursor is locked. In practice, the client doesn't change the cursor while dragging, so we could remove the lock instead. WDYT? https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:339: // Prevent subsequent requests in the same frame from aborting this capture. On 2017/04/26 23:41:26, reveman wrote: > why is this needed and correct? It's a hack to ensure that the lock is bypassed. RequestCopyOfOutput aborts pending requests with the same source ID, so a forced capture may be aborted if the client happens to commit in the same frame. https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:389: cursor_client->SetCursor(cursor); On 2017/04/26 23:41:26, reveman wrote: > can we avoid having this line? "force || cursor_client->IsCursorLocked()" below > instead? The line below updates the current cursor, and this line updates the future unlocked cursor. https://codereview.chromium.org/2812663002/diff/20001/components/exo/wm_helpe... File components/exo/wm_helper_ash.cc (right): https://codereview.chromium.org/2812663002/diff/20001/components/exo/wm_helpe... components/exo/wm_helper_ash.cc:135: ->SetCursor(cursor, shell->cursor_manager()); On 2017/04/26 22:13:39, oshima wrote: > NativeCursorManager is to provide platform impls, so I'd like to avoid exposing > it to client. Just curious, do we need to lock the cursor? Not really. It's mostly for consistency with Ash if the client happens to change the cursor while dragging.
On 2017/04/27 20:27:20, Dominik Laskowski wrote: > https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.cc > File components/exo/pointer.cc (right): > > https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... > components/exo/pointer.cc:270: // Capture is asynchronous, so avoid rendering > old cursor in the meantime. > On 2017/04/26 23:41:26, reveman wrote: > > why shouldn't we keep the old cursor until a new one has arrived? > > Because the old bitmap may have an incorrect scale or rotation for the new > display. > > https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... > components/exo/pointer.cc:273: // Force the cursor to be updated even if locked. > On 2017/04/26 23:41:26, reveman wrote: > > Is "force" only needed for the case when we drag an arc window across display > > boundaries? It's unfortunate that the current multi-display logic requires > these > > kind of special cases. Would it be possible to generalize this a bit. What if > we > > allow the cursor to always be changed if the currently locked cursor is the > > current cursor for this pointer? > > Right, and it's only needed because the cursor is locked. In practice, the > client doesn't change the cursor while dragging, so we could remove the lock > instead. WDYT? > > https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... > components/exo/pointer.cc:339: // Prevent subsequent requests in the same frame > from aborting this capture. > On 2017/04/26 23:41:26, reveman wrote: > > why is this needed and correct? > > It's a hack to ensure that the lock is bypassed. RequestCopyOfOutput aborts > pending requests with the same source ID, so a forced capture may be aborted if > the client happens to commit in the same frame. > > https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... > components/exo/pointer.cc:389: cursor_client->SetCursor(cursor); > On 2017/04/26 23:41:26, reveman wrote: > > can we avoid having this line? "force || cursor_client->IsCursorLocked()" > below > > instead? > > The line below updates the current cursor, and this line updates the future > unlocked cursor. > > https://codereview.chromium.org/2812663002/diff/20001/components/exo/wm_helpe... > File components/exo/wm_helper_ash.cc (right): > > https://codereview.chromium.org/2812663002/diff/20001/components/exo/wm_helpe... > components/exo/wm_helper_ash.cc:135: ->SetCursor(cursor, > shell->cursor_manager()); > On 2017/04/26 22:13:39, oshima wrote: > > NativeCursorManager is to provide platform impls, so I'd like to avoid > exposing > > it to client. Just curious, do we need to lock the cursor? > > Not really. It's mostly for consistency with Ash if the client happens to change > the cursor while dragging. Since we're the client (unlike ash where we have to deal with arbitrary clients), can't we just suppress the cursor update while dragging? It may actually be simpler, although I may be missing some corner case.
https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:270: // Capture is asynchronous, so avoid rendering old cursor in the meantime. On 2017/04/27 at 20:27:19, Dominik Laskowski wrote: > On 2017/04/26 23:41:26, reveman wrote: > > why shouldn't we keep the old cursor until a new one has arrived? > > Because the old bitmap may have an incorrect scale or rotation for the new display. Why is it better to transition to no cursor while waiting for the new one to arrive? Seems better to just do one transition, from old to new size. https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:273: // Force the cursor to be updated even if locked. On 2017/04/27 at 20:27:20, Dominik Laskowski wrote: > On 2017/04/26 23:41:26, reveman wrote: > > Is "force" only needed for the case when we drag an arc window across display > > boundaries? It's unfortunate that the current multi-display logic requires these > > kind of special cases. Would it be possible to generalize this a bit. What if we > > allow the cursor to always be changed if the currently locked cursor is the > > current cursor for this pointer? > > Right, and it's only needed because the cursor is locked. In practice, the client doesn't change the cursor while dragging, so we could remove the lock instead. WDYT? What does removing the lock mean? https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:339: // Prevent subsequent requests in the same frame from aborting this capture. On 2017/04/27 at 20:27:20, Dominik Laskowski wrote: > On 2017/04/26 23:41:26, reveman wrote: > > why is this needed and correct? > > It's a hack to ensure that the lock is bypassed. RequestCopyOfOutput aborts pending requests with the same source ID, so a forced capture may be aborted if the client happens to commit in the same frame. Ok, let's figure out how we can avoid this "force" thing so we don't need this kind of hack. I'm not even sure that showing the perfect cursor during drag is that important unless this multi-display mechanism with client side drags is what we plan on using long term. https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:389: cursor_client->SetCursor(cursor); On 2017/04/27 at 20:27:20, Dominik Laskowski wrote: > On 2017/04/26 23:41:26, reveman wrote: > > can we avoid having this line? "force || cursor_client->IsCursorLocked()" below > > instead? > > The line below updates the current cursor, and this line updates the future unlocked cursor. That's confusing and deserves a comment at least.
https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:270: // Capture is asynchronous, so avoid rendering old cursor in the meantime. On 2017/04/27 21:44:14, reveman wrote: > Why is it better to transition to no cursor while waiting for the new one to > arrive? Seems better to just do one transition, from old to new size. Because the incorrect transform is noticeable, e.g. huge cursor for a frame or two when crossing from high-DPI to low-DPI displays. https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:273: // Force the cursor to be updated even if locked. On 2017/04/27 21:44:14, reveman wrote: > What does removing the lock mean? Removing the LockCursor/UnlockCursor calls, i.e. allowing the client to update the cursor during dragging. https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... components/exo/pointer.cc:339: // Prevent subsequent requests in the same frame from aborting this capture. On 2017/04/27 21:44:14, reveman wrote: > Ok, let's figure out how we can avoid this "force" thing so we don't need this > kind of hack. I'm not even sure that showing the perfect cursor during drag is > that important unless this multi-display mechanism with client side drags is > what we plan on using long term. Yeah, this is a non-issue with server decorations. How about we keep it simple and remove cursor locking for now?
On 2017/04/27 at 22:22:17, domlaskowski wrote: > https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.cc > File components/exo/pointer.cc (right): > > https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... > components/exo/pointer.cc:270: // Capture is asynchronous, so avoid rendering old cursor in the meantime. > On 2017/04/27 21:44:14, reveman wrote: > > Why is it better to transition to no cursor while waiting for the new one to > > arrive? Seems better to just do one transition, from old to new size. > > Because the incorrect transform is noticeable, e.g. huge cursor for a frame or two when crossing from high-DPI to low-DPI displays. > > https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... > components/exo/pointer.cc:273: // Force the cursor to be updated even if locked. > On 2017/04/27 21:44:14, reveman wrote: > > What does removing the lock mean? > > Removing the LockCursor/UnlockCursor calls, i.e. allowing the client to update the cursor during dragging. > > https://codereview.chromium.org/2812663002/diff/20001/components/exo/pointer.... > components/exo/pointer.cc:339: // Prevent subsequent requests in the same frame from aborting this capture. > On 2017/04/27 21:44:14, reveman wrote: > > Ok, let's figure out how we can avoid this "force" thing so we don't need this > > kind of hack. I'm not even sure that showing the perfect cursor during drag is > > that important unless this multi-display mechanism with client side drags is > > what we plan on using long term. > > Yeah, this is a non-issue with server decorations. How about we keep it simple and remove cursor locking for now? Sounds good. Just removing it for client drag, right?
removing lock is fine for now. You can still avoid updating from app side. This is probably necessary to avoid hiding cursor during drag, but we should have better way to handle DSF change with ARC cursor.
Description was changed from ========== exo: Force cursor capture when crossing displays While dragging, the cursor is locked, so CursorClient::SetCursor calls are collapsed into a single call at the end of the drag. When crossing displays, we need to force a capture to update the scale, and hide the old cursor while the capture is pending. BUG=631136 BUG=642894 TEST=caroline: Cursor scale is correct while dragging across displays. ========== to ========== exo: Fix cursor scale when crossing displays While dragging, the cursor was locked, so CursorClient::SetCursor calls were collapsed into a single call at the end of the drag. When crossing displays with different densities, the cursor scale was incorrect until the window was dropped. This CL updates the cursor scale when the mouse crosses a display boundary, and removes cursor locking to prevent the update from being postponed. BUG=631136 BUG=642894 TEST=caroline: Cursor scale is correct while dragging across displays. ==========
PTAL.
lgtm after removing the code that prevents old cursor from being rendering https://codereview.chromium.org/2812663002/diff/40001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2812663002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:270: // Capture is asynchronous, so avoid rendering old cursor in the meantime. I'm still not convinced that we should do this. One cursor transition instead of two seems better imo. I would hate to see the cursor flicker. Much better to show an incorrectly sized cursor for a few ms imo. How about just a simple CaptureCursor() call here for now so we can land this fix and you can then file a bug for the incorrect size problem and use that bug to convince me that showing no cursor for a few ms is better. Is that OK?
lgtm https://codereview.chromium.org/2812663002/diff/40001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2812663002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:270: // Capture is asynchronous, so avoid rendering old cursor in the meantime. On 2017/04/28 21:37:09, reveman wrote: > I'm still not convinced that we should do this. One cursor transition instead of > two seems better imo. I would hate to see the cursor flicker. Much better to > show an incorrectly sized cursor for a few ms imo. How about just a simple > CaptureCursor() call here for now so we can land this fix and you can then file > a bug for the incorrect size problem and use that bug to convince me that > showing no cursor for a few ms is better. Is that OK? Yes, we need a better way to handle custom cursors in general. (high contract, large cursors are others) Let me file a bug(s). I agree that it's probably better to not to reset to none, but i'll leave it to you guys.
https://codereview.chromium.org/2812663002/diff/40001/components/exo/pointer.cc File components/exo/pointer.cc (right): https://codereview.chromium.org/2812663002/diff/40001/components/exo/pointer.... components/exo/pointer.cc:270: // Capture is asynchronous, so avoid rendering old cursor in the meantime. On 2017/05/01 22:18:04, oshima wrote: > On 2017/04/28 21:37:09, reveman wrote: > > I'm still not convinced that we should do this. One cursor transition instead > of > > two seems better imo. I would hate to see the cursor flicker. Much better to > > show an incorrectly sized cursor for a few ms imo. How about just a simple > > CaptureCursor() call here for now so we can land this fix and you can then > file > > a bug for the incorrect size problem and use that bug to convince me that > > showing no cursor for a few ms is better. Is that OK? > > Yes, we need a better way to handle custom cursors in general. > (high contract, large cursors are others) > > Let me file a bug(s). > > I agree that it's probably better to not to reset to none, but i'll leave it to > you guys. > Done, and filed a bug: http://crbug.com/719116
still lgtm
Description was changed from ========== exo: Fix cursor scale when crossing displays While dragging, the cursor was locked, so CursorClient::SetCursor calls were collapsed into a single call at the end of the drag. When crossing displays with different densities, the cursor scale was incorrect until the window was dropped. This CL updates the cursor scale when the mouse crosses a display boundary, and removes cursor locking to prevent the update from being postponed. BUG=631136 BUG=642894 TEST=caroline: Cursor scale is correct while dragging across displays. ========== to ========== exo: Fix cursor scale when crossing displays While dragging, the cursor was locked, so CursorClient::SetCursor calls were collapsed into a single call at the end of the drag. When crossing displays with different densities, the cursor scale was incorrect until the window was dropped. This CL removes cursor locking to prevent the OnCursorDisplayChanged update from being postponed. BUG=631136 BUG=642894 TEST=caroline: Cursor scale is correct while dragging across displays. ==========
The CQ bit was checked by domlaskowski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2812663002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496341111375020,
"parent_rev": "04b63e7cbb8aa3f8f27b0b42f2eaa9e890fed8ac", "commit_rev":
"fa6afade49688205eb70ba3329a23c91080cf53a"}
Message was sent while issue was closed.
Description was changed from ========== exo: Fix cursor scale when crossing displays While dragging, the cursor was locked, so CursorClient::SetCursor calls were collapsed into a single call at the end of the drag. When crossing displays with different densities, the cursor scale was incorrect until the window was dropped. This CL removes cursor locking to prevent the OnCursorDisplayChanged update from being postponed. BUG=631136 BUG=642894 TEST=caroline: Cursor scale is correct while dragging across displays. ========== to ========== exo: Fix cursor scale when crossing displays While dragging, the cursor was locked, so CursorClient::SetCursor calls were collapsed into a single call at the end of the drag. When crossing displays with different densities, the cursor scale was incorrect until the window was dropped. This CL removes cursor locking to prevent the OnCursorDisplayChanged update from being postponed. BUG=631136 BUG=642894 TEST=caroline: Cursor scale is correct while dragging across displays. Review-Url: https://codereview.chromium.org/2812663002 Cr-Commit-Position: refs/heads/master@{#476375} Committed: https://chromium.googlesource.com/chromium/src/+/fa6afade49688205eb70ba3329a2... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/fa6afade49688205eb70ba3329a2... |
