Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2050813002:
MacViews: support backgrounds for selected rows (Closed)
|
Created:
4 years, 6 months ago by Elly Fong-Jones Modified:
4 years, 4 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: support backgrounds for selected rows
This change adds support to TreeView for drawing a background on the entire
selected row instead of just behind the label of the selected row, controlled
by PlatformStyle::kTreeViewSelectsEntireRow.
Incidentally, this CL fixes the views_unittests build with gn so that this CL can be tested.
This CL is heavily inspired by https://codereview.chromium.org/1364423002/.
BUG=605589
Committed: https://crrev.com/df230152824bb04c6a7d520ecc77e24b9c02eaa1
Cr-Commit-Position: refs/heads/master@{#413755}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Refactor TreeView, fix views_unittests build #
Total comments: 14
Patch Set 3 : excise depth #
Total comments: 10
Patch Set 4 : GetForegroundBoundsForNode :( #
Total comments: 8
Patch Set 5 : fixes #Patch Set 6 : rebase #Patch Set 7 : rebase again #Patch Set 8 : placate msvc #
Messages
Total messages: 40 (19 generated)
Description was changed from ========== MacViews: support backgrounds for selected rows This change adds support to TreeView for drawing a background on the entire selected row instead of just behind the label of the selected row, controlled by PlatformStyle::kTreeViewSelectsEntireRow. This CL is heavily inspired by https://codereview.chromium.org/1364423002/. BUG=605589 ========== to ========== MacViews: support backgrounds for selected rows This change adds support to TreeView for drawing a background on the entire selected row instead of just behind the label of the selected row, controlled by PlatformStyle::kTreeViewSelectsEntireRow. This CL is heavily inspired by https://codereview.chromium.org/1364423002/. BUG=605589 ==========
ellyjones@chromium.org changed reviewers: + tapted@chromium.org
tapted: ptal? :)
https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... File ui/views/controls/tree/tree_view.cc (right): https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:648: gfx::Rect bounds(GetBoundsForNodeImpl(node, row, depth)); This should use GetBackgroundBoundsForNode() too, otherwise clicking on the row (but not on the _text_) won't change the row selection. But of course we will need to pass in `row` and `depth` so it does the right thing (or figure out some other nice solution). There's another call in TreeView::ShowContextMenu() which should probably be the entire row as well, but I haven't checked a Cocoa TreeView to see if (a) it can have context menus and (b) whether they are invoked outside the text region. In fact, I think the *only* place we still want to use GetBoundsForNode[Impl] is in TreeView::LayoutEditor() - and we might want to tweak that as well. https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:773: const SkColor bg_color = GetNativeTheme()->GetSystemColor( bg_color -> selected_row_bg_color https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:774: text_background_color_id(HasFocus())); I think HasFocus() -> HasFocus() || editing_ Otherwise when editing the field editor gets focus, which makes the background goes grey. Cocoa retains the background color for this case https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:818: // Paint the text. Should this still be inside `if ((!editing_ || selected_node_ != node)`? (i.e. regardless of kTreeViewSelectsEntireRow). Otherwise, we're painting text "under" the Textfield control being used for editing. Something neater than nesting/repeating the `if` statements might be, here, // No need to paint the text on the row being edited. if (editing_ && selected_node == node) return; In fact, this can go up on line 801, so that the `if` statement on line 809 only has to check `if (!PlatformStyle::kTreeViewSelectsEntireRow)` https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:880: gfx::Rect row_bounds(GetBoundsForNode(node)); This rect we want is much simpler - e.g. |depth| is ignored completely, and we don't need to mirror anything. I think we should just repeat the y/height calculations from GetBoundsForNodeImpl
Description was changed from ========== MacViews: support backgrounds for selected rows This change adds support to TreeView for drawing a background on the entire selected row instead of just behind the label of the selected row, controlled by PlatformStyle::kTreeViewSelectsEntireRow. This CL is heavily inspired by https://codereview.chromium.org/1364423002/. BUG=605589 ========== to ========== MacViews: support backgrounds for selected rows This change adds support to TreeView for drawing a background on the entire selected row instead of just behind the label of the selected row, controlled by PlatformStyle::kTreeViewSelectsEntireRow. Incidentally, this CL fixes the views_unittests build with gn so that this CL can be tested. This CL is heavily inspired by https://codereview.chromium.org/1364423002/. BUG=605589 ==========
tapted: ptal? :) https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... File ui/views/controls/tree/tree_view.cc (right): https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:648: gfx::Rect bounds(GetBoundsForNodeImpl(node, row, depth)); On 2016/06/09 07:13:37, tapted wrote: > This should use GetBackgroundBoundsForNode() too, otherwise clicking on the row > (but not on the _text_) won't change the row selection. But of course we will > need to pass in `row` and `depth` so it does the right thing (or figure out some > other nice solution). > > There's another call in TreeView::ShowContextMenu() which should probably be the > entire row as well, but I haven't checked a Cocoa TreeView to see if (a) it can > have context menus and (b) whether they are invoked outside the text region. > > In fact, I think the *only* place we still want to use GetBoundsForNode[Impl] is > in TreeView::LayoutEditor() - and we might want to tweak that as well. I reworked this function and ShowContextMenu to be more explicit that they are doing hit-testing, by adding NodeAtPoint(), and added HitsNodeArrow() to test for arrow hits. This function is now much easier to follow, I think. https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:773: const SkColor bg_color = GetNativeTheme()->GetSystemColor( On 2016/06/09 07:13:37, tapted wrote: > bg_color -> selected_row_bg_color Done. https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:774: text_background_color_id(HasFocus())); On 2016/06/09 07:13:37, tapted wrote: > I think HasFocus() -> HasFocus() || editing_ > > Otherwise when editing the field editor gets focus, which makes the background > goes grey. Cocoa retains the background color for this case Done. https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:818: // Paint the text. On 2016/06/09 07:13:37, tapted wrote: > Should this still be inside `if ((!editing_ || selected_node_ != node)`? (i.e. > regardless of kTreeViewSelectsEntireRow). Otherwise, we're painting text "under" > the Textfield control being used for editing. Something neater than > nesting/repeating the `if` statements might be, here, > > // No need to paint the text on the row being edited. > if (editing_ && selected_node == node) > return; > > In fact, this can go up on line 801, so that the `if` statement on line 809 only > has to check `if (!PlatformStyle::kTreeViewSelectsEntireRow)` Done. https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:880: gfx::Rect row_bounds(GetBoundsForNode(node)); On 2016/06/09 07:13:38, tapted wrote: > This rect we want is much simpler - e.g. |depth| is ignored completely, and we > don't need to mirror anything. I think we should just repeat the y/height > calculations from GetBoundsForNodeImpl The reason this uses GetBoundsForNode() and then adjusts is so that the background isn't the entire row on builds where kTreeViewSelectsEntireRow is false, since the background is used to decide what needs to be repainted in SchedulePaintForNode().
Sorry I didn't manage to get to this on Friday - I like the refactor! But there might be a subtle regression in HitsNodeArrow() https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... File ui/views/controls/tree/tree_view.cc (right): https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:648: gfx::Rect bounds(GetBoundsForNodeImpl(node, row, depth)); On 2016/06/09 20:24:37, Elly Jones wrote: > On 2016/06/09 07:13:37, tapted wrote: > > This should use GetBackgroundBoundsForNode() too, otherwise clicking on the > row > > (but not on the _text_) won't change the row selection. But of course we will > > need to pass in `row` and `depth` so it does the right thing (or figure out > some > > other nice solution). > > > > There's another call in TreeView::ShowContextMenu() which should probably be > the > > entire row as well, but I haven't checked a Cocoa TreeView to see if (a) it > can > > have context menus and (b) whether they are invoked outside the text region. > > > > In fact, I think the *only* place we still want to use GetBoundsForNode[Impl] > is > > in TreeView::LayoutEditor() - and we might want to tweak that as well. > > I reworked this function and ShowContextMenu to be more explicit that they are > doing hit-testing, by adding NodeAtPoint(), and added HitsNodeArrow() to test > for arrow hits. This function is now much easier to follow, I think. Agree - nice refactoring! https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:880: gfx::Rect row_bounds(GetBoundsForNode(node)); On 2016/06/09 20:24:37, Elly Jones wrote: > On 2016/06/09 07:13:38, tapted wrote: > > This rect we want is much simpler - e.g. |depth| is ignored completely, and we > > don't need to mirror anything. I think we should just repeat the y/height > > calculations from GetBoundsForNodeImpl > > The reason this uses GetBoundsForNode() and then adjusts is so that the > background isn't the entire row on builds where kTreeViewSelectsEntireRow is > false, since the background is used to decide what needs to be repainted in > SchedulePaintForNode(). Yeah - I think it would look like if (!PlatformStyle::kTreeViewSelectsEntireRow) return GetTextBoundsForNode(node); int row, ignored_depth; row = GetRowForInternalNode(node, &ignored_depth); return gfx::Rect(bounds.x(), row * row_height_ + kVerticalInset, bounds().width(), row_height_); https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... File ui/views/controls/tree/tree_view.cc (right): https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:75: return gesture.details().tap_count() == 2; I think we can skip the `auto` and do return event.AsGestureEvent().details().tap_count() == 2; https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:76: } else { nit: "no else after return" - last item at https://www.chromium.org/developers/coding-style#TOC-Code-formatting https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:405: if (!NodeAtPoint(local_point)) I think it would improve NodeAtPoint if it required a non-null depth - then we just end up with an extra line here like `int ignored_depth;` rather than the extra stuff in NodeAtPoint https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:765: PaintExpandControl(canvas, bounds, node->is_expanded()); We probably need to convert this to a vector icon so we can control the color it's painted, but that's good for a follow-up. https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:873: gfx::Rect TreeView::GetBoundsForNode(InternalNode* node) { perhaps we should rename these to GetTextBoundsForNode[Impl]? https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:1047: return false; nit: blank line after https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:1050: gfx::Rect arrow_bounds(bounds.x() + arrow_dx, bounds.y(), kArrowRegionSize, I don't think we can use bounds.x() here -- GetBackgroundBoundsForNode() returns different a different x value for kTreeViewSelectsEntireRow, but the arrow should still be in the same place, regardless of kTreeViewSelectsEntireRow (it seems ok on mac, so maybe this has regressed on other platforms) It's possible the fix is just `bounds.x()` -> 'bounds().x()` but that's a bit too subtle. - We should rename the |bounds| local variable to row_bounds. Or, perhaps best, just use int row, depth; row = GetRowForInternalNode(node, &depth); int arrow_dx = .. gfx::Rect arrow_bounds(/* x calc */, row * row_height_ + kVerticalInset, kArrowRegionSize, row_height_); i.e. don't pass in |depth| to the function -- GetBackgroundBoundsForNode is currently calculating it anyway.
tapted: ptal? :) https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... File ui/views/controls/tree/tree_view.cc (right): https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:75: return gesture.details().tap_count() == 2; On 2016/06/13 07:03:47, tapted wrote: > I think we can skip the `auto` and do > > return event.AsGestureEvent().details().tap_count() == 2; Done. https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:76: } else { On 2016/06/13 07:03:48, tapted wrote: > nit: "no else after return" - last item at > https://www.chromium.org/developers/coding-style#TOC-Code-formatting Done. https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:405: if (!NodeAtPoint(local_point)) On 2016/06/13 07:03:47, tapted wrote: > I think it would improve NodeAtPoint if it required a non-null depth - then we > just end up with an extra line here like `int ignored_depth;` rather than the > extra stuff in NodeAtPoint Done. https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:765: PaintExpandControl(canvas, bounds, node->is_expanded()); On 2016/06/13 07:03:47, tapted wrote: > We probably need to convert this to a vector icon so we can control the color > it's painted, but that's good for a follow-up. Acknowledged. https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:873: gfx::Rect TreeView::GetBoundsForNode(InternalNode* node) { On 2016/06/13 07:03:47, tapted wrote: > perhaps we should rename these to GetTextBoundsForNode[Impl]? Done. https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:1047: return false; On 2016/06/13 07:03:48, tapted wrote: > nit: blank line after Done. https://codereview.chromium.org/2050813002/diff/20001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:1050: gfx::Rect arrow_bounds(bounds.x() + arrow_dx, bounds.y(), kArrowRegionSize, On 2016/06/13 07:03:48, tapted wrote: > I don't think we can use bounds.x() here -- GetBackgroundBoundsForNode() returns > different a different x value for kTreeViewSelectsEntireRow, but the arrow > should still be in the same place, regardless of kTreeViewSelectsEntireRow > > (it seems ok on mac, so maybe this has regressed on other platforms) > > It's possible the fix is just `bounds.x()` -> 'bounds().x()` but that's a bit > too subtle. - We should rename the |bounds| local variable to row_bounds. > > Or, perhaps best, just use > > int row, depth; > row = GetRowForInternalNode(node, &depth); > int arrow_dx = .. > gfx::Rect arrow_bounds(/* x calc */, row * row_height_ + kVerticalInset, > kArrowRegionSize, row_height_); > > > i.e. don't pass in |depth| to the function -- GetBackgroundBoundsForNode is > currently calculating it anyway. Done.
basically lgtm, but there's a dangling thread from an earlier patchset (I guess I could be convinced either way). https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... File ui/views/controls/tree/tree_view.cc (right): https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:880: gfx::Rect row_bounds(GetBoundsForNode(node)); On 2016/06/13 07:03:47, tapted wrote: > On 2016/06/09 20:24:37, Elly Jones wrote: > > On 2016/06/09 07:13:38, tapted wrote: > > > This rect we want is much simpler - e.g. |depth| is ignored completely, and > we > > > don't need to mirror anything. I think we should just repeat the y/height > > > calculations from GetBoundsForNodeImpl > > > > The reason this uses GetBoundsForNode() and then adjusts is so that the > > background isn't the entire row on builds where kTreeViewSelectsEntireRow is > > false, since the background is used to decide what needs to be repainted in > > SchedulePaintForNode(). > > Yeah - I think it would look like > > > if (!PlatformStyle::kTreeViewSelectsEntireRow) > return GetTextBoundsForNode(node); > > int row, ignored_depth; > row = GetRowForInternalNode(node, &ignored_depth); > return gfx::Rect(bounds.x(), row * row_height_ + kVerticalInset, > bounds().width(), row_height_); ping? any thoughts on this? https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... File ui/views/controls/tree/tree_view.cc (right): https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:787: gfx::Rect text_bounds(bounds.x() + text_offset_, bounds.y(), gah - I didn't notice this. That confuses things. i.e. having `GetTextBoundsForNodeImpl` not include |text_offset_|. Perhaps if we state in the comment for `GetTextBoundsForNodeImpl` that it includes |text_offset_|, and here call |text_bounds| -> |drawn_text_bounds| or something. Otherwise we need to come up with a better name for GetTextBoundsForNodeImpl, but everything I can think of makes things more confusing. https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... File ui/views/controls/tree/tree_view.h (right): https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.h:300: // Returns the bounds for a node. nit: update comment https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.h:303: // Implementation of GetBoundsForNode. Separated out as some callers already nit: update comment https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.h:337: // This function returns the InternalNode (if any) lying under |point|. If no nit: remove "This function" / "this function". https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.h:341: // This function returns whether |point| is in the bounds of |node|'s nit: Returns whether..
ellyjones@chromium.org changed reviewers: + sky@chromium.org
sky: ptal? https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... File ui/views/controls/tree/tree_view.cc (right): https://codereview.chromium.org/2050813002/diff/1/ui/views/controls/tree/tree... ui/views/controls/tree/tree_view.cc:880: gfx::Rect row_bounds(GetBoundsForNode(node)); On 2016/06/15 03:17:49, tapted wrote: > On 2016/06/13 07:03:47, tapted wrote: > > On 2016/06/09 20:24:37, Elly Jones wrote: > > > On 2016/06/09 07:13:38, tapted wrote: > > > > This rect we want is much simpler - e.g. |depth| is ignored completely, > and > > we > > > > don't need to mirror anything. I think we should just repeat the y/height > > > > calculations from GetBoundsForNodeImpl > > > > > > The reason this uses GetBoundsForNode() and then adjusts is so that the > > > background isn't the entire row on builds where kTreeViewSelectsEntireRow is > > > false, since the background is used to decide what needs to be repainted in > > > SchedulePaintForNode(). > > > > Yeah - I think it would look like > > > > > > if (!PlatformStyle::kTreeViewSelectsEntireRow) > > return GetTextBoundsForNode(node); > > > > int row, ignored_depth; > > row = GetRowForInternalNode(node, &ignored_depth); > > return gfx::Rect(bounds.x(), row * row_height_ + kVerticalInset, > > bounds().width(), row_height_); > > ping? any thoughts on this? Oop, sorry, missed it. Done. https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... File ui/views/controls/tree/tree_view.cc (right): https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:787: gfx::Rect text_bounds(bounds.x() + text_offset_, bounds.y(), On 2016/06/15 03:17:49, tapted wrote: > gah - I didn't notice this. That confuses things. i.e. having > `GetTextBoundsForNodeImpl` not include |text_offset_|. Perhaps if we state in > the comment for `GetTextBoundsForNodeImpl` that it includes |text_offset_|, and > here call |text_bounds| -> |drawn_text_bounds| or something. > > Otherwise we need to come up with a better name for GetTextBoundsForNodeImpl, > but everything I can think of makes things more confusing. Argh, I didn't spot that either. I introduced GetForegroundBoundsForNode, converted most uses of GetTextBounds over to GetForegroundBounds, then made this use GetTextBounds directly, and everything seems to work okay. https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... File ui/views/controls/tree/tree_view.h (right): https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.h:300: // Returns the bounds for a node. On 2016/06/15 03:17:50, tapted wrote: > nit: update comment Done. https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.h:303: // Implementation of GetBoundsForNode. Separated out as some callers already On 2016/06/15 03:17:50, tapted wrote: > nit: update comment Done. https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.h:337: // This function returns the InternalNode (if any) lying under |point|. If no On 2016/06/15 03:17:50, tapted wrote: > nit: remove "This function" / "this function". Done. https://codereview.chromium.org/2050813002/diff/40001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.h:341: // This function returns whether |point| is in the bounds of |node|'s On 2016/06/15 03:17:50, tapted wrote: > nit: Returns whether.. Done.
https://codereview.chromium.org/2050813002/diff/60001/ui/views/controls/tree/... File ui/views/controls/tree/tree_view.cc (right): https://codereview.chromium.org/2050813002/diff/60001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:1041: int row = GetRowForInternalNode(node, &depth); Can this use GetForegroundBoundsForNode? It would be nice if PaintExpandControl and this used a common function. https://codereview.chromium.org/2050813002/diff/60001/ui/views/controls/tree/... File ui/views/controls/tree/tree_view.h (right): https://codereview.chromium.org/2050813002/diff/60001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.h:345: InternalNode* NodeAtPoint(const gfx::Point& point); GetNodeAtPoint and put above GetNodeByRow. Also make it clear this uses the foreground bounds. https://codereview.chromium.org/2050813002/diff/60001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.h:348: bool HitsNodeArrow(InternalNode* node, const gfx::Point& point); nit: arrow refers to a specific rendering and isn't necessarily the same. How about IsPointInExpandControl? https://codereview.chromium.org/2050813002/diff/60001/ui/views/style/platform... File ui/views/style/platform_style.h (right): https://codereview.chromium.org/2050813002/diff/60001/ui/views/style/platform... ui/views/style/platform_style.h:47: static const bool kTreeViewSelectsEntireRow; nit: I think kTreeViewSelectionPaintsEntireRow makes it clearer what this controls.
sky: ptal? :) https://codereview.chromium.org/2050813002/diff/60001/ui/views/controls/tree/... File ui/views/controls/tree/tree_view.cc (right): https://codereview.chromium.org/2050813002/diff/60001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.cc:1041: int row = GetRowForInternalNode(node, &depth); On 2016/06/16 17:34:39, sky wrote: > Can this use GetForegroundBoundsForNode? It would be nice if PaintExpandControl > and this used a common function. I don't think so. This function needs to use row and depth, both of which GetForegroundBoundsForNode deliberately abstracts away. https://codereview.chromium.org/2050813002/diff/60001/ui/views/controls/tree/... File ui/views/controls/tree/tree_view.h (right): https://codereview.chromium.org/2050813002/diff/60001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.h:345: InternalNode* NodeAtPoint(const gfx::Point& point); On 2016/06/16 17:34:39, sky wrote: > GetNodeAtPoint and put above GetNodeByRow. Also make it clear this uses the > foreground bounds. Done. https://codereview.chromium.org/2050813002/diff/60001/ui/views/controls/tree/... ui/views/controls/tree/tree_view.h:348: bool HitsNodeArrow(InternalNode* node, const gfx::Point& point); On 2016/06/16 17:34:39, sky wrote: > nit: arrow refers to a specific rendering and isn't necessarily the same. How > about IsPointInExpandControl? Done. https://codereview.chromium.org/2050813002/diff/60001/ui/views/style/platform... File ui/views/style/platform_style.h (right): https://codereview.chromium.org/2050813002/diff/60001/ui/views/style/platform... ui/views/style/platform_style.h:47: static const bool kTreeViewSelectsEntireRow; On 2016/06/16 17:34:39, sky wrote: > nit: I think kTreeViewSelectionPaintsEntireRow makes it clearer what this > controls. Done.
LGTM
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2050813002/#ps80001 (title: "fixes")
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
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...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2050813002/#ps100001 (title: "rebase")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2050813002/#ps120001 (title: "rebase again")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2050813002/#ps140001 (title: "placate msvc")
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
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ellyjones@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.
Description was changed from ========== MacViews: support backgrounds for selected rows This change adds support to TreeView for drawing a background on the entire selected row instead of just behind the label of the selected row, controlled by PlatformStyle::kTreeViewSelectsEntireRow. Incidentally, this CL fixes the views_unittests build with gn so that this CL can be tested. This CL is heavily inspired by https://codereview.chromium.org/1364423002/. BUG=605589 ========== to ========== MacViews: support backgrounds for selected rows This change adds support to TreeView for drawing a background on the entire selected row instead of just behind the label of the selected row, controlled by PlatformStyle::kTreeViewSelectsEntireRow. Incidentally, this CL fixes the views_unittests build with gn so that this CL can be tested. This CL is heavily inspired by https://codereview.chromium.org/1364423002/. BUG=605589 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: support backgrounds for selected rows This change adds support to TreeView for drawing a background on the entire selected row instead of just behind the label of the selected row, controlled by PlatformStyle::kTreeViewSelectsEntireRow. Incidentally, this CL fixes the views_unittests build with gn so that this CL can be tested. This CL is heavily inspired by https://codereview.chromium.org/1364423002/. BUG=605589 ========== to ========== MacViews: support backgrounds for selected rows This change adds support to TreeView for drawing a background on the entire selected row instead of just behind the label of the selected row, controlled by PlatformStyle::kTreeViewSelectsEntireRow. Incidentally, this CL fixes the views_unittests build with gn so that this CL can be tested. This CL is heavily inspired by https://codereview.chromium.org/1364423002/. BUG=605589 Committed: https://crrev.com/df230152824bb04c6a7d520ecc77e24b9c02eaa1 Cr-Commit-Position: refs/heads/master@{#413755} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/df230152824bb04c6a7d520ecc77e24b9c02eaa1 Cr-Commit-Position: refs/heads/master@{#413755} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
