Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2111643004:
[ash-md] Adjusts animation of the selector after closing an overview item (Closed)
Description[ash-md] Adjusts selector animation after closing an item in overview
BUG=622533
Committed: https://crrev.com/fbec348d355be946e8021ae0c873f97bbf1ad143
Cr-Commit-Position: refs/heads/master@{#403803}
Patch Set 1 #
Total comments: 4
Patch Set 2 : [ash-md] Adjusts animation of the selector after closing an overview item (comments) #
Total comments: 3
Patch Set 3 : [ash-md] Adjusts animation of the selector after closing an overview item (more comments) #
Messages
Total messages: 33 (15 generated)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
varkha@chromium.org changed reviewers: + bruthig@chromium.org
bruthig@, can you please take a look? Thanks!
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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by varkha@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.
https://codereview.chromium.org/2111643004/diff/1/ash/common/wm/overview/wind... File ash/common/wm/overview/window_grid.cc (right): https://codereview.chromium.org/2111643004/diff/1/ash/common/wm/overview/wind... ash/common/wm/overview/window_grid.cc:734: // Animates selector widget to |opacity|. Is this comment needed? https://codereview.chromium.org/2111643004/diff/1/ash/common/wm/overview/wind... File ash/common/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/2111643004/diff/1/ash/common/wm/overview/wind... ash/common/wm/overview/window_selector_item.cc:417: window_selector_->SetSelectorOpacity(0.f); I feel like the actual opacity value of the WindowSelector::selection_widget_ should be an internal detail of the WindowSelector class. Perhaps calling an event callback like WindowSelector::WindowClosed|Closing() that abstracts away the opacity value from the clients would be better.
Thanks! See if you like this better. https://codereview.chromium.org/2111643004/diff/1/ash/common/wm/overview/wind... File ash/common/wm/overview/window_grid.cc (right): https://codereview.chromium.org/2111643004/diff/1/ash/common/wm/overview/wind... ash/common/wm/overview/window_grid.cc:734: // Animates selector widget to |opacity|. On 2016/07/04 17:06:22, bruthig wrote: > Is this comment needed? Done (duplicated for some reason in the header, I guess I was planning first to have this a local anonymous method here and not a class method). https://codereview.chromium.org/2111643004/diff/1/ash/common/wm/overview/wind... File ash/common/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/2111643004/diff/1/ash/common/wm/overview/wind... ash/common/wm/overview/window_selector_item.cc:417: window_selector_->SetSelectorOpacity(0.f); On 2016/07/04 17:06:22, bruthig wrote: > I feel like the actual opacity value of the WindowSelector::selection_widget_ > should be an internal detail of the WindowSelector class. Perhaps calling an > event callback like WindowSelector::WindowClosed|Closing() that abstracts away > the opacity value from the clients would be better. Done.
The CQ bit was checked by varkha@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 https://codereview.chromium.org/2111643004/diff/20001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/2111643004/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:417: window_selector_->HideCurrentSelector(); nit: I still think HideCurrentSelector() would be better named to indicate the "what happened" signal, i.e. WindowClosed, as opposed to the action that is supposed to happen. I don't think the WindowSelectorItem should be the class deciding when to show/hide the selector. Renaming as such would make it a lot more natural to add conditional logic to the method in WindowSelector. Anyway, I feel like this may be very nit picky on my part so I will leave it to your discretion.
https://codereview.chromium.org/2111643004/diff/20001/ash/common/wm/overview/... File ash/common/wm/overview/window_grid.cc (right): https://codereview.chromium.org/2111643004/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/window_grid.cc:890: animation_settings.SetTweenType(gfx::Tween::EASE_IN_OUT); Self review: make the change --md-ash flag-dependent. Done. https://codereview.chromium.org/2111643004/diff/20001/ash/common/wm/overview/... File ash/common/wm/overview/window_selector_item.cc (right): https://codereview.chromium.org/2111643004/diff/20001/ash/common/wm/overview/... ash/common/wm/overview/window_selector_item.cc:417: window_selector_->HideCurrentSelector(); On 2016/07/05 14:27:03, bruthig wrote: > nit: I still think HideCurrentSelector() would be better named to indicate the > "what happened" signal, i.e. WindowClosed, as opposed to the action that is > supposed to happen. I don't think the WindowSelectorItem should be the class > deciding when to show/hide the selector. Renaming as such would make it a lot > more natural to add conditional logic to the method in WindowSelector. > > Anyway, I feel like this may be very nit picky on my part so I will leave it to > your discretion. Fair enough! Done. Also thanks to your insistence I've noticed that I was fading out the selector in any case - even if the item closing was not the one selected. This was masked somewhat by quick fading in and repositioning of the selector but was quite wrong. Thanks!
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by varkha@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...
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by varkha@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 varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2111643004/#ps80001 (title: "[ash-md] Adjusts animation of the selector after closing an overview item (more comments)")
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 #3 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [ash-md] Adjusts selector animation after closing an item in overview BUG=622533 ========== to ========== [ash-md] Adjusts selector animation after closing an item in overview BUG=622533 Committed: https://crrev.com/fbec348d355be946e8021ae0c873f97bbf1ad143 Cr-Commit-Position: refs/heads/master@{#403803} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fbec348d355be946e8021ae0c873f97bbf1ad143 Cr-Commit-Position: refs/heads/master@{#403803} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
