Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12906002:
Add ability to defined ranges with different styles in StyledLabel (Closed)
|
Created:
7 years, 9 months ago by tbarzic Modified:
2 years, 5 months ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd ability to defined ranges with different styles in StyledLabel
In adition to having linkified ranges, support ranges with different font styles
and tooltips.
BUG=175070
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191857
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #
Total comments: 8
Patch Set 6 : . #
Total comments: 8
Patch Set 7 : . #Patch Set 8 : . #
Total comments: 20
Patch Set 9 : . #Patch Set 10 : . #
Total comments: 3
Patch Set 11 : rebase + win fix #Patch Set 12 : . #
Messages
Total messages: 21 (0 generated)
Hey Can you take a look at this?
https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:48: result->SetFont(result->font().DeriveFont(0, style_info.font_style)); setting a different font means that the bounds calculated for the label with ui::ElideRectangleText may be incorrect. https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:165: // This happens when only whitespace is left. it doesn't seem like this should be what happens if only whitespace is left. It seems to imply we may not respect trailing whitespace. If we want to not respect trailing whitespace, the cleanest solution would be to trim it when first setting the text.
https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:48: result->SetFont(result->font().DeriveFont(0, style_info.font_style)); On 2013/03/18 23:20:50, Evan Stade wrote: > setting a different font means that the bounds calculated for the label with > ui::ElideRectangleText may be incorrect. good point.. is it safe to assume that the font height will remain the same after DeriveFont(0, new_style)? If so the new version should work fine. https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:165: // This happens when only whitespace is left. On 2013/03/18 23:20:50, Evan Stade wrote: > it doesn't seem like this should be what happens if only whitespace is left. It > seems to imply we may not respect trailing whitespace. If we want to not respect > trailing whitespace, the cleanest solution would be to trim it when first > setting the text. yeah we could do that, the downside is trimming the text when it's set could make the label setup a bit messier (e.g. to properly handle the following case: label = StyledLabel(text); label->SetLink(Range(2, text.length());
https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:48: result->SetFont(result->font().DeriveFont(0, style_info.font_style)); On 2013/03/19 01:07:34, tbarzic wrote: > On 2013/03/18 23:20:50, Evan Stade wrote: > > setting a different font means that the bounds calculated for the label with > > ui::ElideRectangleText may be incorrect. > > good point.. is it safe to assume that the font height will remain the same > after DeriveFont(0, new_style)? If so the new version should work fine. height and width would both need to stay the same (width for wrapping). I don't think bolding text keeps the same width. There may be styles that change the height, I dunno.
https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:48: result->SetFont(result->font().DeriveFont(0, style_info.font_style)); On 2013/03/19 04:49:29, Evan Stade wrote: > On 2013/03/19 01:07:34, tbarzic wrote: > > On 2013/03/18 23:20:50, Evan Stade wrote: > > > setting a different font means that the bounds calculated for the label with > > > ui::ElideRectangleText may be incorrect. > > > > good point.. is it safe to assume that the font height will remain the same > > after DeriveFont(0, new_style)? If so the new version should work fine. > > height and width would both need to stay the same (width for wrapping). I don't > think bolding text keeps the same width. There may be styles that change the > height, I dunno. Yes, I understand that width is important for wrapping, but patchset 6 should handle that... the wrapping is calculated with the font style of the next text chunk that should be laid out. I guess the words that contain different font styles could still get broken at the style breaks, but I'm not sure how much we care about that use case at this point. ps. supporting only underlined style is enough for my needs, so I'd be happy to limit possible font styles to NORMAL and UNDERLINED (both of which we already have embedded in the label)
any updates on this?
I think estade is on vacation til Monday. On Fri, Mar 22, 2013 at 10:24 AM, <tbarzic@chromium.org> wrote: > any updates on this? > > https://codereview.chromium.**org/12906002/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:165: // This happens when only whitespace is left. On 2013/03/19 01:07:34, tbarzic wrote: > On 2013/03/18 23:20:50, Evan Stade wrote: > > it doesn't seem like this should be what happens if only whitespace is left. > It > > seems to imply we may not respect trailing whitespace. If we want to not > respect > > trailing whitespace, the cleanest solution would be to trim it when first > > setting the text. > > yeah we could do that, the downside is trimming the text when it's set could > make the label setup a bit messier (e.g. to properly handle the following case: > label = StyledLabel(text); > label->SetLink(Range(2, text.length()); but you still don't handle that correctly, because instead of styling the trailing whitespace, you lop it off (sometimes). So either handle it correctly or don't handle it at all, such that the above code might be considered illegal if |text| has trailing whitespace. https://codereview.chromium.org/12906002/diff/12001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12906002/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.h:33: int font_style; docs for these members (esp. the first one) https://codereview.chromium.org/12906002/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.h:59: const RangeStyleInfo& style_info) nit: indent https://codereview.chromium.org/12906002/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.h:86: // A mapping from a link control to the range it corresponds to in |text_|. nit: a link? https://codereview.chromium.org/12906002/diff/12001/ui/views/controls/styled_... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/12906002/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:206: } please add a test for bolding and how it affects layout.
https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12906002/diff/4002/ui/views/controls/styled_l... ui/views/controls/styled_label.cc:165: // This happens when only whitespace is left. On 2013/03/25 22:30:11, Evan Stade wrote: > On 2013/03/19 01:07:34, tbarzic wrote: > > On 2013/03/18 23:20:50, Evan Stade wrote: > > > it doesn't seem like this should be what happens if only whitespace is left. > > It > > > seems to imply we may not respect trailing whitespace. If we want to not > > respect > > > trailing whitespace, the cleanest solution would be to trim it when first > > > setting the text. > > > > yeah we could do that, the downside is trimming the text when it's set could > > make the label setup a bit messier (e.g. to properly handle the following > case: > > label = StyledLabel(text); > > label->SetLink(Range(2, text.length()); > > but you still don't handle that correctly, because instead of styling the > trailing whitespace, you lop it off (sometimes). So either handle it correctly > or don't handle it at all, such that the above code might be considered illegal > if |text| has trailing whitespace. ok then, added trimming in the constructor. note that the algorithm doesn't respect leading whitespace either, so trimming both ways (or should we not trim whitespace at the first line beginning) https://codereview.chromium.org/12906002/diff/12001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12906002/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.h:33: int font_style; On 2013/03/25 22:30:11, Evan Stade wrote: > docs for these members (esp. the first one) Done. https://codereview.chromium.org/12906002/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.h:59: const RangeStyleInfo& style_info) On 2013/03/25 22:30:11, Evan Stade wrote: > nit: indent Done. https://codereview.chromium.org/12906002/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label.h:86: // A mapping from a link control to the range it corresponds to in |text_|. On 2013/03/25 22:30:11, Evan Stade wrote: > nit: a link? Done. https://codereview.chromium.org/12906002/diff/12001/ui/views/controls/styled_... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/12906002/diff/12001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:206: } On 2013/03/25 22:30:11, Evan Stade wrote: > please add a test for bolding and how it affects layout. Done.
lgtm https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:194: // but does with normal font style. can you have the test make sure the second assertion is true (that with the normal font, it does fit on one line)? https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:214: // The second bold part should start in a new line. nit: on a new line.
https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:72: TrimWhitespace(text, TRIM_ALL, &text_); Is there a reason to do this? I ask as presumably any styles added are going to be calculated from the text passed in. If TrimWhitespace removed leading whitespace then all those calculations would be off. https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:82: RangeStyleInfo style_info; Can't this be implemented in terms of AddStyleRange now? https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:144: std::priority_queue<StyleRange> style_ranges = style_ranges_; Why do we copy here? style_ranges_ isn't going to change during this, right? https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.h:41: bool linkify; How about is_link. https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.h:48: // Marks the given range within |text_| as a link. |range| should be contained should->must https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.h:53: // |range| should be contained in |text_|. should->must https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:59: ASSERT_EQ(Label::kViewClassName, styled()->child_at(0)->GetClassName()); Don't you need an assert on on child_count() (same comment for other tests).
https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:72: TrimWhitespace(text, TRIM_ALL, &text_); On 2013/03/28 16:02:09, sky wrote: > Is there a reason to do this? I ask as presumably any styles added are going to > be calculated from the text passed in. If TrimWhitespace removed leading > whitespace then all those calculations would be off. that's true, I guess you should only trim trailing and let the Trim in CalculateAndDoLayout handle leading. https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:144: std::priority_queue<StyleRange> style_ranges = style_ranges_; On 2013/03/28 16:02:09, sky wrote: > Why do we copy here? style_ranges_ isn't going to change during this, right? no, but |style_ranges| is
https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:72: TrimWhitespace(text, TRIM_ALL, &text_); On 2013/03/28 17:03:07, Evan Stade wrote: > On 2013/03/28 16:02:09, sky wrote: > > Is there a reason to do this? I ask as presumably any styles added are going > to > > be calculated from the text passed in. If TrimWhitespace removed leading > > whitespace then all those calculations would be off. > > that's true, I guess you should only trim trailing and let the Trim in > CalculateAndDoLayout handle leading. yeah, I had similar thought last night but haven't had chance to update the code. the StyledLabel now respects leading whitespace, provided that the first line would not be empty (in which case leading whitespace is ignored). https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:82: RangeStyleInfo style_info; On 2013/03/28 16:02:09, sky wrote: > Can't this be implemented in terms of AddStyleRange now? Done. (Added static RangeStyleInfo::CreateForLink (the link default style values differ from the label's so I think this would be useful)) https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:144: std::priority_queue<StyleRange> style_ranges = style_ranges_; On 2013/03/28 17:03:07, Evan Stade wrote: > On 2013/03/28 16:02:09, sky wrote: > > Why do we copy here? style_ranges_ isn't going to change during this, right? > > no, but |style_ranges| is as Evan said, |style_range| gets emptied. if you're too concerned about this, I could work something out so we don't have to copy all the ranges (e.g. using set and just iteration over it) https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... File ui/views/controls/styled_label.h (right): https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.h:41: bool linkify; On 2013/03/28 16:02:09, sky wrote: > How about is_link. Done. https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.h:48: // Marks the given range within |text_| as a link. |range| should be contained On 2013/03/28 16:02:09, sky wrote: > should->must Done. https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label.h:53: // |range| should be contained in |text_|. On 2013/03/28 16:02:09, sky wrote: > should->must Done. https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... File ui/views/controls/styled_label_unittest.cc (right): https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:59: ASSERT_EQ(Label::kViewClassName, styled()->child_at(0)->GetClassName()); On 2013/03/28 16:02:09, sky wrote: > Don't you need an assert on on child_count() (same comment for other tests). yeah, I forgot to add them in these two. https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:194: // but does with normal font style. On 2013/03/27 22:25:11, Evan Stade wrote: > can you have the test make sure the second assertion is true (that with the > normal font, it does fit on one line)? Done. https://codereview.chromium.org/12906002/diff/27001/ui/views/controls/styled_... ui/views/controls/styled_label_unittest.cc:214: // The second bold part should start in a new line. On 2013/03/27 22:25:11, Evan Stade wrote: > nit: on a new line. Done.
LGTM
https://codereview.chromium.org/12906002/diff/39001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12906002/diff/39001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:145: if (x == 0 && line > 0) I don't think you need to make this change. |position| is calculated in such a way as to compensate for trimming leading whitespace no matter what line you're on.
https://codereview.chromium.org/12906002/diff/39001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12906002/diff/39001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:145: if (x == 0 && line > 0) On 2013/03/29 19:35:48, Evan Stade wrote: > I don't think you need to make this change. |position| is calculated in such a > way as to compensate for trimming leading whitespace no matter what line you're > on. yeah, but this way we actually respect leading whitespace.
https://codereview.chromium.org/12906002/diff/39001/ui/views/controls/styled_... File ui/views/controls/styled_label.cc (right): https://codereview.chromium.org/12906002/diff/39001/ui/views/controls/styled_... ui/views/controls/styled_label.cc:145: if (x == 0 && line > 0) On 2013/03/29 20:00:57, tbarzic wrote: > On 2013/03/29 19:35:48, Evan Stade wrote: > > I don't think you need to make this change. |position| is calculated in such a > > way as to compensate for trimming leading whitespace no matter what line > you're > > on. > > yeah, but this way we actually respect leading whitespace. ok, lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tbarzic@chromium.org/12906002/47001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... 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/tbarzic@chromium.org/12906002/58003
Message was sent while issue was closed.
Change committed as 191857 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
