Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
639893003:
Unify MenuButton Pushed state logic (Closed)
DescriptionUnify MenuButton Pushed state logic
CustomButton uses ShouldEnterPushedState to determine visual state changes. This uses IsTriggerableEvent, only transitioning to pushed if an activation will occur.
MenuButton has both an pushed state, showing the menu, as well as triggerable actions which do not show the menu. This change organizes the logic for activating the menu via ShouldEnterPushedState, separating it for menus from IsTriggerableEvent.
TEST=MenuButtonTest.ActivateDropDownOnMouseClick, MenuButtonTest.ActivateDropDownOnGestureTap, MenuButtonTest.DraggableMenuButtonActivatesOnRelease
BUG=398404
Committed: https://crrev.com/48bb4848bf961d8e1b744ac6b4d8275d425279fc
Cr-Commit-Position: refs/heads/master@{#301015}
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 6
Patch Set 4 : Add drag test #Patch Set 5 : Drag Drop Unittest #
Total comments: 22
Patch Set 6 : Update Formatting #
Messages
Total messages: 18 (3 generated)
jonross@chromium.org changed reviewers: + flackr@chromium.org
Hey Rob, Can you take a first look at this change? Centralizing the pushed state activation logic for MenuButtons.
Can you add to this review the change that this simplifies making? i.e. the active state on touchdown https://codereview.chromium.org/639893003/diff/1/ui/views/controls/button/men... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/639893003/diff/1/ui/views/controls/button/men... ui/views/controls/button/menu_button.cc:289: GetDragOperations(mouseev.location()) == ui::DragDropTypes::DRAG_NONE) { nit: I think this would look cleaner if you had EventType active_on = GetDragOperations(mouseev.location()) == ui::DragDropTypes::DRAG_NONE ? ui::ET_MOUSE_PRESSED : ui::ET_MOUSE_RELEASED. Then you can have a single condition which checks for the matching event type and doesn't have to potentially call GetDragOperations twice. https://codereview.chromium.org/639893003/diff/1/ui/views/controls/button/men... File ui/views/controls/button/menu_button.h (right): https://codereview.chromium.org/639893003/diff/1/ui/views/controls/button/men... ui/views/controls/button/menu_button.h:88: // Overridden from MenuButton: Overridden from MenuButton? Isn't this MenuButton?
https://codereview.chromium.org/639893003/diff/1/ui/views/controls/button/men... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/639893003/diff/1/ui/views/controls/button/men... ui/views/controls/button/menu_button.cc:289: GetDragOperations(mouseev.location()) == ui::DragDropTypes::DRAG_NONE) { On 2014/10/14 14:11:06, flackr wrote: > nit: I think this would look cleaner if you had EventType active_on = > GetDragOperations(mouseev.location()) == ui::DragDropTypes::DRAG_NONE ? > ui::ET_MOUSE_PRESSED : ui::ET_MOUSE_RELEASED. > > Then you can have a single condition which checks for the matching event type > and doesn't have to potentially call GetDragOperations twice. Done. https://codereview.chromium.org/639893003/diff/1/ui/views/controls/button/men... File ui/views/controls/button/menu_button.h (right): https://codereview.chromium.org/639893003/diff/1/ui/views/controls/button/men... ui/views/controls/button/menu_button.h:88: // Overridden from MenuButton: On 2014/10/14 14:11:06, flackr wrote: > Overridden from MenuButton? Isn't this MenuButton? Yup, meant CustomButton.
flackr@chromium.org changed reviewers: + sky@chromium.org
lgtm with nit https://codereview.chromium.org/639893003/diff/60001/ui/views/controls/button... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/639893003/diff/60001/ui/views/controls/button... ui/views/controls/button/menu_button.cc:174: if (state() != STATE_DISABLED && ShouldEnterPushedState(event) && Perhaps ShouldEnterPushedState should check for state() != STATE_DISABLED instead of checking at each caller.
Sky could you provide an owners review for MenuButton? I have merged the activation logic into ShouldEnterPressedState. https://codereview.chromium.org/639893003/diff/60001/ui/views/controls/button... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/639893003/diff/60001/ui/views/controls/button... ui/views/controls/button/menu_button.cc:174: if (state() != STATE_DISABLED && ShouldEnterPushedState(event) && On 2014/10/14 19:23:08, flackr wrote: > Perhaps ShouldEnterPushedState should check for state() != STATE_DISABLED > instead of checking at each caller. Done.
How about test coverage too? https://codereview.chromium.org/639893003/diff/80001/ui/views/controls/button... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/639893003/diff/80001/ui/views/controls/button... ui/views/controls/button/menu_button.cc:277: if (state() == STATE_DISABLED) This function shouldn't have to check disabled (just as it doesn't hit test). Call sites should do that (same as CustomButton does now). https://codereview.chromium.org/639893003/diff/80001/ui/views/controls/button... ui/views/controls/button/menu_button.cc:291: if (event.type() == active_on && !InDrag()) I also think InDrag should be checked in the call site too. https://codereview.chromium.org/639893003/diff/80001/ui/views/controls/button... ui/views/controls/button/menu_button.cc:295: if (event.type() == ui::ET_GESTURE_TAP) nit: return event.type() == ET_GESTURE_TAP.
I've added a new test case for MenuButtons can can be dragged. MenuButtonTest.DraggableMenuButtonActivatesOnRelease I've noted in the description two pre-existing tests that verify the behaviour of this change. I need to investigate testing releases during drags further. Currently when performing a mouse drag and release, MenuButton::OnMouseReleased is never called. So the InDrag() logic is not being executed. https://codereview.chromium.org/639893003/diff/80001/ui/views/controls/button... File ui/views/controls/button/menu_button.cc (right): https://codereview.chromium.org/639893003/diff/80001/ui/views/controls/button... ui/views/controls/button/menu_button.cc:277: if (state() == STATE_DISABLED) On 2014/10/14 20:05:06, sky wrote: > This function shouldn't have to check disabled (just as it doesn't hit test). > Call sites should do that (same as CustomButton does now). Done. https://codereview.chromium.org/639893003/diff/80001/ui/views/controls/button... ui/views/controls/button/menu_button.cc:291: if (event.type() == active_on && !InDrag()) On 2014/10/14 20:05:06, sky wrote: > I also think InDrag should be checked in the call site too. Done. https://codereview.chromium.org/639893003/diff/80001/ui/views/controls/button... ui/views/controls/button/menu_button.cc:295: if (event.type() == ui::ET_GESTURE_TAP) On 2014/10/14 20:05:06, sky wrote: > nit: return event.type() == ET_GESTURE_TAP. Done.
Let me know when you've resolved everything and want me to take another look. On Tue, Oct 14, 2014 at 2:42 PM, <jonross@chromium.org> wrote: > I've added a new test case for MenuButtons can can be dragged. > > MenuButtonTest.DraggableMenuButtonActivatesOnRelease > > I've noted in the description two pre-existing tests that verify the > behaviour > of this change. > > I need to investigate testing releases during drags further. Currently when > performing a mouse drag and release, MenuButton::OnMouseReleased is never > called. So the InDrag() logic is not being executed. > > > https://codereview.chromium.org/639893003/diff/80001/ui/views/controls/button... > File ui/views/controls/button/menu_button.cc (right): > > https://codereview.chromium.org/639893003/diff/80001/ui/views/controls/button... > ui/views/controls/button/menu_button.cc:277: if (state() == > STATE_DISABLED) > On 2014/10/14 20:05:06, sky wrote: >> >> This function shouldn't have to check disabled (just as it doesn't hit > > test). >> >> Call sites should do that (same as CustomButton does now). > > > Done. > > https://codereview.chromium.org/639893003/diff/80001/ui/views/controls/button... > ui/views/controls/button/menu_button.cc:291: if (event.type() == > active_on && !InDrag()) > On 2014/10/14 20:05:06, sky wrote: >> >> I also think InDrag should be checked in the call site too. > > > Done. > > https://codereview.chromium.org/639893003/diff/80001/ui/views/controls/button... > ui/views/controls/button/menu_button.cc:295: if (event.type() == > ui::ET_GESTURE_TAP) > On 2014/10/14 20:05:06, sky wrote: >> >> nit: return event.type() == ET_GESTURE_TAP. > > > Done. > > https://codereview.chromium.org/639893003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I've added a test for drag and drop. Could you take another look? Thanks.
https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... File ui/views/controls/button/menu_button_unittest.cc (right): https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:119: : last_source_(NULL), NULL->nullptr https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:144: virtual ~TestDragController() {} no virtual, just override. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:146: virtual void WriteDragDataForView(View* sender, No virtual, just override (ya, style changed). https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:202: TestDragDropClient::TestDragDropClient() : drag_in_progress_(false), run git cl format. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:203: target_(NULL) { nullptr https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:216: return 0; DRAG_NONE https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:364: scoped_ptr<TestMenuButtonListener> menu_button_listener( No need to put in scoped_ptr, create on the stack. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:374: EXPECT_EQ(NULL, menu_button_listener->last_source()); nullptr https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:376: only one newline is needed here. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:379: EXPECT_EQ(Button::STATE_PRESSED, menu_button_listener->last_source_state()); Why does the button stay pressed when the mouse is released? https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:386: scoped_ptr<TestMenuButtonListener> menu_button_listener( same comment about creating on the stack.
https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... File ui/views/controls/button/menu_button_unittest.cc (right): https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:119: : last_source_(NULL), On 2014/10/23 17:25:31, sky wrote: > NULL->nullptr Done. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:144: virtual ~TestDragController() {} On 2014/10/23 17:25:31, sky wrote: > no virtual, just override. Done. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:146: virtual void WriteDragDataForView(View* sender, On 2014/10/23 17:25:31, sky wrote: > No virtual, just override (ya, style changed). Done. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:202: TestDragDropClient::TestDragDropClient() : drag_in_progress_(false), On 2014/10/23 17:25:31, sky wrote: > run git cl format. Done. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:203: target_(NULL) { On 2014/10/23 17:25:31, sky wrote: > nullptr Done. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:216: return 0; On 2014/10/23 17:25:31, sky wrote: > DRAG_NONE Done. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:364: scoped_ptr<TestMenuButtonListener> menu_button_listener( On 2014/10/23 17:25:31, sky wrote: > No need to put in scoped_ptr, create on the stack. Done. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:374: EXPECT_EQ(NULL, menu_button_listener->last_source()); On 2014/10/23 17:25:31, sky wrote: > nullptr Done. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:376: On 2014/10/23 17:25:31, sky wrote: > only one newline is needed here. Done. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:379: EXPECT_EQ(Button::STATE_PRESSED, menu_button_listener->last_source_state()); On 2014/10/23 17:25:31, sky wrote: > Why does the button stay pressed when the mouse is released? When a menu button activates to show the list of actions, it remains in the pressed visual state until an action is taken to dismiss the menu. https://codereview.chromium.org/639893003/diff/120001/ui/views/controls/butto... ui/views/controls/button/menu_button_unittest.cc:386: scoped_ptr<TestMenuButtonListener> menu_button_listener( On 2014/10/23 17:25:31, sky wrote: > same comment about creating on the stack. Done.
LGTM
The CQ bit was checked by jonross@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/639893003/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/48bb4848bf961d8e1b744ac6b4d8275d425279fc Cr-Commit-Position: refs/heads/master@{#301015} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
