Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19382003:
Make dragging outside the app list scroll back to its original position. (Closed)
|
Created:
7 years, 5 months ago by koz (OOO until 15th September) Modified:
7 years, 5 months ago Reviewers:
xiyuan CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina, benwells Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake dragging outside the app list scroll back to its original position.
Currently when dragging an app from the app list outside the bounds of
the app launcher the apps in the launcher continue to re-arrange
themselves even though the cursor may be nowhere near the app launcher,
which looks weird.
This change makes it so that the app launcher animates back to its
original state while the cursor is outside the launcher, and will cause
drops to outside the app launcher to return the dragged app to its
original position, which is consistent with flows such as dragging apps
to the taskbar on Windows / ChromeOS.
BUG=261452
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213339
Patch Set 1 #Patch Set 2 : don't change pages on drag out #
Total comments: 6
Patch Set 3 : respond to comments #
Total comments: 2
Patch Set 4 : add drag buffer #
Total comments: 6
Patch Set 5 : respond to comments #Patch Set 6 : rebase #Patch Set 7 : fix compile #Messages
Total messages: 24 (0 generated)
Should we leave the page as it is when dragging out of the launcher? Say, if a user is attempting to move an app to a different page and accidentally moves out of bound, since we would change to the original page, the user would have to do the page switching again. This could be frustrating.
On 2013/07/18 16:37:10, xiyuan wrote: > Should we leave the page as it is when dragging out of the launcher? Say, if a > user is attempting to move an app to a different page and accidentally moves out > of bound, since we would change to the original page, the user would have to do > the page switching again. This could be frustrating. Yeah, that's a good point. I was aiming for a simple rule here (revert the app launcher to its old state when an app is dragged out), but it's probably far more common for someone who has dragged an app to a different page to want to put it somewhere else in the app launcher.
https://codereview.chromium.org/19382003/diff/4001/ui/app_list/views/apps_gri... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/19382003/diff/4001/ui/app_list/views/apps_gri... ui/app_list/views/apps_grid_view.cc:910: current_page = drag_start_page_; Should we jus return here? Otherwise, this would change to before-drag-start state.
https://codereview.chromium.org/19382003/diff/4001/ui/app_list/views/apps_gri... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/19382003/diff/4001/ui/app_list/views/apps_gri... ui/app_list/views/apps_grid_view.cc:412: MaybeStartPageFlipTimer(last_drag_point_); I wonder if we should also update MaybeStartPageFlipTimer to suppress auto flipping when some conditions met, e.g. user dragged out of bound for too long, or too far etc.
https://codereview.chromium.org/19382003/diff/4001/ui/app_list/views/apps_gri... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/19382003/diff/4001/ui/app_list/views/apps_gri... ui/app_list/views/apps_grid_view.cc:412: MaybeStartPageFlipTimer(last_drag_point_); On 2013/07/19 01:06:33, xiyuan wrote: > I wonder if we should also update MaybeStartPageFlipTimer to suppress auto > flipping when some conditions met, e.g. user dragged out of bound for too long, > or too far etc. Good idea! Done. https://codereview.chromium.org/19382003/diff/4001/ui/app_list/views/apps_gri... ui/app_list/views/apps_grid_view.cc:910: current_page = drag_start_page_; On 2013/07/19 00:59:59, xiyuan wrote: > Should we jus return here? Otherwise, this would change to before-drag-start > state. That's the intent of this change - to have the drag return to the "before drag" state if the user drags the app out, ie: the same state that they will be in after they drop the app outside the launcher. The reason I want that is twofold: - if the user drops an app while the drag is outside the launcher it will (on Windows) revert the app launcher back to its original state anyway, so reverting to that state while the drag is in process would be a better indication of the result of the drag. - currently dragging an app outside the launcher causes the apps inside the launcher to rearrange themselves despite the dragged app being nowhere near them which looks weird We could solve the second problem by simply ignoring drag events that occur outside the app launcher, but that wouldn't help the (somewhat less important) first case. Would you prefer that?
https://codereview.chromium.org/19382003/diff/4001/ui/app_list/views/apps_gri... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/19382003/diff/4001/ui/app_list/views/apps_gri... ui/app_list/views/apps_grid_view.cc:910: current_page = drag_start_page_; On 2013/07/19 04:03:57, koz wrote: > On 2013/07/19 00:59:59, xiyuan wrote: > > Should we jus return here? Otherwise, this would change to before-drag-start > > state. > > That's the intent of this change - to have the drag return to the "before drag" > state if the user drags the app out, ie: the same state that they will be in > after they drop the app outside the launcher. The reason I want that is twofold: > > - if the user drops an app while the drag is outside the launcher it will (on > Windows) revert the app launcher back to its original state anyway, so reverting > to that state while the drag is in process would be a better indication of the > result of the drag. > > - currently dragging an app outside the launcher causes the apps inside the > launcher to rearrange themselves despite the dragged app being nowhere near them > which looks weird > > We could solve the second problem by simply ignoring drag events that occur > outside the app launcher, but that wouldn't help the (somewhat less important) > first case. Would you prefer that? Okay. How about add a threshold for resetting to original state? This way, user does not trigger the reset by accident when trying to flip the page by dragging the item close the edge. And when user drags the item out of the launcher far enough, we take that as user's intent to drop it out of the launcher and reset to the original state. That is, we stop rearranging when the drag is out of the launcher and reset to the original state when the drag is out far enough. What do you think? https://codereview.chromium.org/19382003/diff/12001/ui/app_list/views/apps_gr... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/19382003/diff/12001/ui/app_list/views/apps_gr... ui/app_list/views/apps_grid_view.cc:1033: StopPageFlipTimer(); nit: We probably could move this to be before the "if" since we are doing it in both branches now.
https://codereview.chromium.org/19382003/diff/4001/ui/app_list/views/apps_gri... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/19382003/diff/4001/ui/app_list/views/apps_gri... ui/app_list/views/apps_grid_view.cc:910: current_page = drag_start_page_; On 2013/07/19 04:36:46, xiyuan wrote: > On 2013/07/19 04:03:57, koz wrote: > > On 2013/07/19 00:59:59, xiyuan wrote: > > > Should we jus return here? Otherwise, this would change to before-drag-start > > > state. > > > > That's the intent of this change - to have the drag return to the "before > drag" > > state if the user drags the app out, ie: the same state that they will be in > > after they drop the app outside the launcher. The reason I want that is > twofold: > > > > - if the user drops an app while the drag is outside the launcher it will (on > > Windows) revert the app launcher back to its original state anyway, so > reverting > > to that state while the drag is in process would be a better indication of the > > result of the drag. > > > > - currently dragging an app outside the launcher causes the apps inside the > > launcher to rearrange themselves despite the dragged app being nowhere near > them > > which looks weird > > > > We could solve the second problem by simply ignoring drag events that occur > > outside the app launcher, but that wouldn't help the (somewhat less important) > > first case. Would you prefer that? > > Okay. How about add a threshold for resetting to original state? This way, user > does not trigger the reset by accident when trying to flip the page by dragging > the item close the edge. And when user drags the item out of the launcher far > enough, we take that as user's intent to drop it out of the launcher and reset > to the original state. That is, we stop rearranging when the drag is out of the > launcher and reset to the original state when the drag is out far enough. What > do you think? Yeah, that sounds good. Including turning the page back to where they were originally? https://codereview.chromium.org/19382003/diff/12001/ui/app_list/views/apps_gr... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/19382003/diff/12001/ui/app_list/views/apps_gr... ui/app_list/views/apps_grid_view.cc:1033: StopPageFlipTimer(); On 2013/07/19 04:36:46, xiyuan wrote: > nit: We probably could move this to be before the "if" since we are doing it in > both branches now. Done.
LGTM https://codereview.chromium.org/19382003/diff/18001/ui/app_list/views/apps_gr... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/19382003/diff/18001/ui/app_list/views/apps_gr... ui/app_list/views/apps_grid_view.cc:1082: rect.set_y(rect.y() - kDragBufferPx); nit: think we can replace the above 4 lines with rect.Inset(-kDragBufferPx, -kDragBufferPx, -kDragBufferPx, -kDragBufferPx); https://codereview.chromium.org/19382003/diff/18001/ui/app_list/views/apps_gr... ui/app_list/views/apps_grid_view.cc:1083: return rect.Intersects(gfx::Rect(point, gfx::Size(1, 1))); nit: return rect.Contains(point); https://codereview.chromium.org/19382003/diff/18001/ui/app_list/views/apps_gr... File ui/app_list/views/apps_grid_view.h (right): https://codereview.chromium.org/19382003/diff/18001/ui/app_list/views/apps_gr... ui/app_list/views/apps_grid_view.h:225: bool IsPointWithinDragBuffer(const gfx::Point& point) const; nit: Add a comment for this method.
Thanks, Xiyuan! https://codereview.chromium.org/19382003/diff/18001/ui/app_list/views/apps_gr... File ui/app_list/views/apps_grid_view.cc (right): https://codereview.chromium.org/19382003/diff/18001/ui/app_list/views/apps_gr... ui/app_list/views/apps_grid_view.cc:1082: rect.set_y(rect.y() - kDragBufferPx); On 2013/07/19 16:24:04, xiyuan wrote: > nit: think we can replace the above 4 lines with > rect.Inset(-kDragBufferPx, -kDragBufferPx, -kDragBufferPx, -kDragBufferPx); Done. https://codereview.chromium.org/19382003/diff/18001/ui/app_list/views/apps_gr... ui/app_list/views/apps_grid_view.cc:1083: return rect.Intersects(gfx::Rect(point, gfx::Size(1, 1))); On 2013/07/19 16:24:04, xiyuan wrote: > nit: return rect.Contains(point); Done. https://codereview.chromium.org/19382003/diff/18001/ui/app_list/views/apps_gr... File ui/app_list/views/apps_grid_view.h (right): https://codereview.chromium.org/19382003/diff/18001/ui/app_list/views/apps_gr... ui/app_list/views/apps_grid_view.h:225: bool IsPointWithinDragBuffer(const gfx::Point& point) const; On 2013/07/19 16:24:04, xiyuan wrote: > nit: Add a comment for this method. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/koz@chromium.org/19382003/24001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/koz@chromium.org/19382003/45001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/koz@chromium.org/19382003/62001
Retried try job too often on linux_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/koz@chromium.org/19382003/62001
Retried try job too often on linux_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/koz@chromium.org/19382003/62001
Retried try job too often on linux_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/koz@chromium.org/19382003/62001
Retried try job too often on linux_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/koz@chromium.org/19382003/62001
Message was sent while issue was closed.
Change committed as 213339 |
