Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12543032:
Add views::RichLabel, a class which creates multi-line text with mixed (Closed)
|
Created:
7 years, 9 months ago by Evan Stade Modified:
7 years, 9 months ago CC:
chromium-reviews, Raman Kakilate, tfarina, benquan, dhollowa+watch_chromium.org, ahutter, dbeam+watch-autofill_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd views::RichLabel, a class which creates multi-line text with mixed
links and plain text.
BUG=168704
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188496
Patch Set 1 #
Total comments: 2
Patch Set 2 : other interface #
Total comments: 21
Patch Set 3 : review #
Total comments: 11
Patch Set 4 : more better #Patch Set 5 : overlap #Patch Set 6 : comment #Patch Set 7 : add tests #
Total comments: 43
Patch Set 8 : consts #
Total comments: 4
Patch Set 9 : fix comment + win compile #Patch Set 10 : compile #
Messages
Total messages: 21 (0 generated)
this patch is not finished (for one thing, clicking the links does nothing), but before I continue I'd like to know what you think about this approach. Regarding the interface: would you still prefer that I pass everything as ranges?
I've also added msw to do this as he has thought about how to use RenderText to solve this. https://codereview.chromium.org/12543032/diff/1/ui/views/controls/rich_label.h File ui/views/controls/rich_label.h (right): https://codereview.chromium.org/12543032/diff/1/ui/views/controls/rich_label.... ui/views/controls/rich_label.h:16: RichLabel(const std::vector<string16>& text, I get that you want this interface, but its awkward. I much prefer the following: SetText(const string16&); SetLink(const ui::Range& range, const GURL& url); At some point I want: ApplyStyle(TextStyle style, bool value, const ui::Range& range); but setting the link is a start. If AutofillDialogViews wants an interface like you have here than create a builder interface that lets you create the resulting string16/ vector<std::pair<ui::Range,GURL>> that is easily applied to RichLabel. I would be inclined to name this StyledLabel, but I'm not sold on that.
My rough plan to accomplish this (below) is rather involved. So RichLabel/StyledLabel may be a good [temporary] solution. Here's my rough plan (visualize my hand waving): (1) Have views::Label use RenderText directly. (2) Add Label::[Set|Apply]Style (or just [Set|Apply]Link?). (3) Add Label ranged hit-testing and hover/click handling. (4) Add multi-line formatting to RenderText (nix Label's). https://codereview.chromium.org/12543032/diff/1/ui/views/controls/rich_label.h File ui/views/controls/rich_label.h (right): https://codereview.chromium.org/12543032/diff/1/ui/views/controls/rich_label.... ui/views/controls/rich_label.h:16: RichLabel(const std::vector<string16>& text, On 2013/03/12 13:45:59, sky wrote: > I get that you want this interface, but its awkward. I much prefer the > following: > > SetText(const string16&); > SetLink(const ui::Range& range, const GURL& url); > > At some point I want: > > ApplyStyle(TextStyle style, bool value, const ui::Range& range); > > but setting the link is a start. > > If AutofillDialogViews wants an interface like you have here than create a > builder interface that lets you create the resulting string16/ > vector<std::pair<ui::Range,GURL>> that is easily applied to RichLabel. > > I would be inclined to name this StyledLabel, but I'm not sold on that. Ditto to Scott's points; also see my general CL comment. See RenderText::[Set|Apply]Style for potential inspiration.
updated
Your approach to layout is interesting; it's probably not the cleanest solution, but it's clever and seems like it ought to work (presuming ElideRectangleText respects i18n/BiDi text and the StyledLabel view coordinates are flipped for RTL... I think). https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:24: StyledLabel::StyledLabel(const string16& text, StyledLabelListener* listener) nit: move the 2nd arg to a new line. https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:26: public LinkListener { nit: fits on the line above. https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:28: explicit StyledLabel(const string16& text, StyledLabelListener* listener); Could you take a LinkListener instead and nix StyledLabelListener? https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:32: void SetLink(const ui::Range& range); Shouldn't this let users supply the associated Link*? Or perhaps take a GURL, etc. and return the newly associated Link*? https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:42: struct LinkRange { Try replacing LinkRange, link_ranges_, & link_targets_ w/ gfx::BreakList<Link*>. I think BreakList is well suited to the ranged link-association you need. (otherwise make sure these handle overlapping ranges, etc. well enough) https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:55: int CalculateAndDoLayout(int width, bool dry_run); nit: Maybe GetHeightForWidthAndLayout with s/dry_run/layout/?
https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:24: StyledLabel::StyledLabel(const string16& text, StyledLabelListener* listener) On 2013/03/13 03:22:07, msw wrote: > nit: move the 2nd arg to a new line. Not necessary. You only need put each arg on its own line if the constructor can't fit on the same line. https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:32: link_ranges_.push(LinkRange(range)); Doesn't this need to mark the calculated_size_ invalid? https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:43: CalculateAndDoLayout(GetLocalBounds().width(), false); Honor the border here and in CalculateAndDoLayout. https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:115: view.reset(link); Do links have the same preferred height as labels?
https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:32: link_ranges_.push(LinkRange(range)); On 2013/03/13 14:33:00, sky wrote: > Doesn't this need to mark the calculated_size_ invalid? Done. https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:43: CalculateAndDoLayout(GetLocalBounds().width(), false); On 2013/03/13 14:33:00, sky wrote: > Honor the border here and in CalculateAndDoLayout. Done. https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:115: view.reset(link); On 2013/03/13 14:33:00, sky wrote: > Do links have the same preferred height as labels? nope, fixed by adding a focus border (which shouldn't ever actually show, but does affect the label's insets). https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:26: public LinkListener { On 2013/03/13 03:22:07, msw wrote: > nit: fits on the line above. Done. https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:28: explicit StyledLabel(const string16& text, StyledLabelListener* listener); On 2013/03/13 03:22:07, msw wrote: > Could you take a LinkListener instead and nix StyledLabelListener? no, see below. https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:32: void SetLink(const ui::Range& range); On 2013/03/13 03:22:07, msw wrote: > Shouldn't this let users supply the associated Link*? > Or perhaps take a GURL, etc. and return the newly associated Link*? No. The fact that this class uses a Link view is an implementation detail which I am intentionally hiding. My understanding is that you and Scott want to eventually change the implementation of this class while leaving the interface the same. https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:42: struct LinkRange { On 2013/03/13 03:22:07, msw wrote: > Try replacing LinkRange, link_ranges_, & link_targets_ w/ gfx::BreakList<Link*>. > I think BreakList is well suited to the ranged link-association you need. > (otherwise make sure these handle overlapping ranges, etc. well enough) I tried doing this, but it's not pretty because ui::Range is used as the identifier for the link (see StyledLabelListener) so the type would be gfx::BreakList<ui::Range>, which is confusing and awkward. The current implementation does not attempt to handle overlapping ranges, but it also does not crash. It effectively bumps the second range to right after the first range if there's overlap. https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:55: int CalculateAndDoLayout(int width, bool dry_run); On 2013/03/13 03:22:07, msw wrote: > nit: Maybe GetHeightForWidthAndLayout with s/dry_run/layout/? I prefer the current naming but can change it if you feel strongly.
https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:32: void SetLink(const ui::Range& range); On 2013/03/14 00:30:46, Evan Stade wrote: > On 2013/03/13 03:22:07, msw wrote: > > Shouldn't this let users supply the associated Link*? > > Or perhaps take a GURL, etc. and return the newly associated Link*? > > No. The fact that this class uses a Link view is an implementation detail which > I am intentionally hiding. My understanding is that you and Scott want to > eventually change the implementation of this class while leaving the interface > the same. That is definitely the case, but making this class store the GURL and pass to the listener seems reasonable to me. Otherwise the consumer has to track it which seems a bit tedious. https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:46: int StyledLabel::CalculateAndDoLayout(int width, bool dry_run) { Can you also add some tests for coverage of this? https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:51: if (!dry_run) { Should this be before the early return on 49? https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:63: int x = 0; Doesn't this need to be set to GetInsets().left() here and below (95/106...). https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.h:31: void SetLink(const ui::Range& range); Makybe this should be AddLink since Set implies only one. https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.h:42: LinkRange(const ui::Range& range) : range(range) {} explicit https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.h:71: }; DISALLOW...
I believe this code is assuming Label and Link are the same height, is that true?
will start working on tests. PTAL at implementation changes. On 2013/03/14 15:39:37, sky wrote: > I believe this code is assuming Label and Link are the same height, is that > true? it was more or less ignoring the preferred height of those classes when it comes to positioning child views, and just using the font height. I've changed it to respect the preferred height of a Link instead, and DCHECK that it's the same for all subviews we're creating. I also had to set it to horizontally overlap the subviews to offset the Link and Label borders. https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:32: void SetLink(const ui::Range& range); On 2013/03/14 14:43:58, sky wrote: > On 2013/03/14 00:30:46, Evan Stade wrote: > > On 2013/03/13 03:22:07, msw wrote: > > > Shouldn't this let users supply the associated Link*? > > > Or perhaps take a GURL, etc. and return the newly associated Link*? > > > > No. The fact that this class uses a Link view is an implementation detail > which > > I am intentionally hiding. My understanding is that you and Scott want to > > eventually change the implementation of this class while leaving the interface > > the same. > > That is definitely the case, but making this class store the GURL and pass to > the listener seems reasonable to me. Otherwise the consumer has to track it > which seems a bit tedious. there's not always going to be a GURL associated with a link. Clicking might initiate some non-navigation action in the browser (for example, the bookmark bubble does that). https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:51: if (!dry_run) { On 2013/03/14 14:43:58, sky wrote: > Should this be before the early return on 49? Done. https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:63: int x = 0; On 2013/03/14 14:43:58, sky wrote: > Doesn't this need to be set to GetInsets().left() here and below (95/106...). L132 needs to account for insets, but that's it (fixed). https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.h:31: void SetLink(const ui::Range& range); On 2013/03/14 14:43:58, sky wrote: > Makybe this should be AddLink since Set implies only one. Done. https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.h:42: LinkRange(const ui::Range& range) : range(range) {} On 2013/03/14 14:43:58, sky wrote: > explicit Done. https://codereview.chromium.org/12543032/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.h:71: }; On 2013/03/14 14:43:58, sky wrote: > DISALLOW... Done.
LGTM
added unit test.
SLGTM https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:38: }; DISALLOW...
Bonus points if you add a test or two of link handling. https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12543032/diff/5001/ui/views/controls/styled_l... ui/views/controls/styled_label.h:32: void SetLink(const ui::Range& range); On 2013/03/14 00:30:46, Evan Stade wrote: > On 2013/03/13 03:22:07, msw wrote: > > Shouldn't this let users supply the associated Link*? > > Or perhaps take a GURL, etc. and return the newly associated Link*? > > No. The fact that this class uses a Link view is an implementation detail which > I am intentionally hiding. My understanding is that you and Scott want to > eventually change the implementation of this class while leaving the interface > the same. Hmm, I see your point. I guess tracking links by range is ok. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:18: bool StyledLabel::LinkRange::operator<(const StyledLabel::LinkRange& other) nit: move the arg onto the next line so const isn't on a line by itself. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:34: link_ranges_.push(LinkRange(range)); Should this InvalidateLayout? I don't think it needs to invalidate |calculated_size_|, but I may be wrong. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:39: gfx::Insets focus_border_padding(1, 1, 1, 1); nit: const https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:69: int line_height = CalculateLineHeight(); nit: const https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:90: gfx::Rect chunk_bounds(x, 0, width - x, 2 * line_height); nit: const https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:92: const gfx::Font& font = nit: Remove this Font init and the resource_bundle.h include. Just use the default gfx::Font ctor to get the same value. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:114: size_t position = text_.size() - remaining_string.size(); nit: const https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:146: gfx::Size view_size = view->GetPreferredSize(); nit: const https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:147: DCHECK_EQ(line_height, view_size.height() - 2 * overlap); nit: should line_height remove the focus border inset instead? https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:164: Label label; nit: static https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.h:60: int CalculateLineHeight(); nit: make this a static class function or a file-local static function. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:41: std::string text("This is a test block of text"); nit: const https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:44: gfx::Size label_preferred_size = label.GetPreferredSize(); nit: const https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:50: std::string text("This is a test block of text"); nit: const https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:66: EXPECT_EQ(3, styled()->child_at(0)->bounds().x()); optional nit: compare to styled()->GetInsets()->width()/height() instead of 3? https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:73: std::string text("This is a test block of text."); nit: const https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:86: std::string text("This is a test block of text, "); nit: const https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:87: std::string link_text("and this should be a link"); nit: const https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:105: std::string text("This is a test block of text, "); nit: const
https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:18: bool StyledLabel::LinkRange::operator<(const StyledLabel::LinkRange& other) On 2013/03/14 23:24:30, msw wrote: > nit: move the arg onto the next line so const isn't on a line by itself. is this really a part of our style guide? I see 89 instances of this in chrome's tree. Nevertheless, done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:34: link_ranges_.push(LinkRange(range)); On 2013/03/14 23:24:30, msw wrote: > Should this InvalidateLayout? I don't think it needs to invalidate > |calculated_size_|, but I may be wrong. it does need to do both because adding a link may change wrapping. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:39: gfx::Insets focus_border_padding(1, 1, 1, 1); On 2013/03/14 23:24:30, msw wrote: > nit: const Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:69: int line_height = CalculateLineHeight(); On 2013/03/14 23:24:30, msw wrote: > nit: const Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:90: gfx::Rect chunk_bounds(x, 0, width - x, 2 * line_height); On 2013/03/14 23:24:30, msw wrote: > nit: const Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:92: const gfx::Font& font = On 2013/03/14 23:24:30, msw wrote: > nit: Remove this Font init and the resource_bundle.h include. > Just use the default gfx::Font ctor to get the same value. Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:114: size_t position = text_.size() - remaining_string.size(); On 2013/03/14 23:24:30, msw wrote: > nit: const Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:146: gfx::Size view_size = view->GetPreferredSize(); On 2013/03/14 23:24:30, msw wrote: > nit: const Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:147: DCHECK_EQ(line_height, view_size.height() - 2 * overlap); On 2013/03/14 23:24:30, msw wrote: > nit: should line_height remove the focus border inset instead? not sure I understand the question, but no, the line height already doesn't include focus border. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:164: Label label; On 2013/03/14 23:24:30, msw wrote: > nit: static class type statics are forbidden. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.h:60: int CalculateLineHeight(); On 2013/03/14 23:24:30, msw wrote: > nit: make this a static class function or a file-local static function. Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:38: }; On 2013/03/14 23:08:01, sky wrote: > DISALLOW... I could swear that didn't work last time I tried to do that on a testing::Test class, but it seems to work now. Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:41: std::string text("This is a test block of text"); On 2013/03/14 23:24:30, msw wrote: > nit: const Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:44: gfx::Size label_preferred_size = label.GetPreferredSize(); On 2013/03/14 23:24:30, msw wrote: > nit: const Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:50: std::string text("This is a test block of text"); On 2013/03/14 23:24:30, msw wrote: > nit: const Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:66: EXPECT_EQ(3, styled()->child_at(0)->bounds().x()); On 2013/03/14 23:24:30, msw wrote: > optional nit: compare to styled()->GetInsets()->width()/height() instead of 3? don't want to do that because it might pass if StyledLabel::GetInsets() ignored the border, and Layout also ignored the border. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:73: std::string text("This is a test block of text."); On 2013/03/14 23:24:30, msw wrote: > nit: const Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:86: std::string text("This is a test block of text, "); On 2013/03/14 23:24:30, msw wrote: > nit: const Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:87: std::string link_text("and this should be a link"); On 2013/03/14 23:24:30, msw wrote: > nit: const Done. https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:105: std::string text("This is a test block of text, "); On 2013/03/14 23:24:30, msw wrote: > nit: const Done.
https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:18: bool StyledLabel::LinkRange::operator<(const StyledLabel::LinkRange& other) On 2013/03/15 02:42:27, Evan Stade wrote: > On 2013/03/14 23:24:30, msw wrote: > > nit: move the arg onto the next line so const isn't on a line by itself. > > is this really a part of our style guide? I see 89 instances of this in chrome's > tree. Nevertheless, done. It was in Google's c++ style guide, but was removed in the last iteration: https://code.google.com/p/google-styleguide/source/detail?r=97 . So, you can leave it like you had it.
LGTM with a couple nits; thanks! https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:18: bool StyledLabel::LinkRange::operator<(const StyledLabel::LinkRange& other) On 2013/03/15 03:32:18, sky wrote: > On 2013/03/15 02:42:27, Evan Stade wrote: > > On 2013/03/14 23:24:30, msw wrote: > > > nit: move the arg onto the next line so const isn't on a line by itself. > > > > is this really a part of our style guide? I see 89 instances of this in > chrome's > > tree. Nevertheless, done. > > It was in Google's c++ style guide, but was removed in the last iteration: > https://code.google.com/p/google-styleguide/source/detail?r=97 . So, you can > leave it like you had it. Sorry about that, i'm behind the times! https://codereview.chromium.org/12543032/diff/26001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:147: DCHECK_EQ(line_height, view_size.height() - 2 * overlap); On 2013/03/15 02:42:27, Evan Stade wrote: > On 2013/03/14 23:24:30, msw wrote: > > nit: should line_height remove the focus border inset instead? > > not sure I understand the question, but no, the line height already doesn't > include focus border. Ah, I misread and thought that the lines were stacked with the focus border height built in, but that's not actually the case if I'm now reading it right. https://codereview.chromium.org/12543032/diff/43001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12543032/diff/43001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:20: // a link. nit: comment says link, but code says label. https://codereview.chromium.org/12543032/diff/43001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:22: Label label; nit: static int height = Label().GetPreferredSize().height(); return height; or, static int height = 0; if (height == 0) { <init height> } return height;
https://codereview.chromium.org/12543032/diff/43001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12543032/diff/43001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:20: // a link. On 2013/03/15 08:38:02, msw wrote: > nit: comment says link, but code says label. Done. https://codereview.chromium.org/12543032/diff/43001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:22: Label label; On 2013/03/15 08:38:02, msw wrote: > nit: static int height = Label().GetPreferredSize().height(); return height; > or, static int height = 0; if (height == 0) { <init height> } return height; I specifically was worried that the default height of a label might change over time (e.g. if the user changed default system font size or something). Computing this every time is cheap (especially given that we're creating a bunch of labels anyway, and this just adds one more).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/12543032/57001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/estade@chromium.org/12543032/68002
Message was sent while issue was closed.
Committed patchset #10 manually as r188496 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
