StyledLabel: Added AddEmbeddedView method.

StyledLabel: Added AddEmbeddedView method.

Allows arbitrary views to be embedded within the label text.

WORK IN PROGRESS. DO NOT SUBMIT.

BUG=596799

[Matt Giuca, 2016-03-22 08:05:13]

mgiuca@chromium.org changed reviewers:
+ sadrul@chromium.org

[Matt Giuca, 2016-03-22 08:05:13]

Hi Sadrul, what do you think of this in principle? (I haven't polished it up
yet, updated tests, etc.) See bug for rationale.

[sadrul, 2016-03-30 14:28:18]

sadrul@chromium.org changed reviewers:
+ msw@chromium.org, sky@chromium.org

[sadrul, 2016-03-30 14:28:19]

+msw@, sky@

This looks like a nice feature to have, but may be very difficult to implement
correctly. Instead of embedding/inserting views, would it be possible to allow
installing hooks to the text-layout code instead?

[sky, 2016-03-30 16:14:42]

What is the specific use case that is needed?

  -Scott

On Wed, Mar 30, 2016 at 7:28 AM,  <sadrul@chromium.org> wrote:
> +msw@, sky@
>
> This looks like a nice feature to have, but may be very difficult to
> implement
> correctly. Instead of embedding/inserting views, would it be possible to
> allow
> installing hooks to the text-layout code instead?
>
> https://codereview.chromium.org/1819303002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Matt Giuca, 2016-03-31 00:36:17]

On 2016/03/30 16:14:42, sky wrote:
> What is the specific use case that is needed?

See https://crbug.com/596799. The specific case is
https://codereview.chromium.org/1820243002 / https://crbug.com/576531 but that
has the odd distinction of only being needed for 1 milestone, then it gets
deleted. So if this can't be landed in M51 then I don't actually need it (and I
doubt it will). So... no actual use case required as of now.

However, it would be useful for other things like the fullscreen bubble
(https://bugs.chromium.org/p/chromium/issues/attachment?aid=219631&inline=1)
which is currently implemented as three labels in a BoxLayout. It would be nicer
if I could just insert the "Esc" box as an embedded view of a styled label. Note
that the BoxLayout trick only works because the text is not multi-line, which is
why I consider it a hack.

> 
>   -Scott
> 
> On Wed, Mar 30, 2016 at 7:28 AM,  <mailto:sadrul@chromium.org> wrote:
> > +msw@, sky@
> >
> > This looks like a nice feature to have, but may be very difficult to
> > implement
> > correctly.

I think the correct implementation is to change StyledLabel to use a BoxLayout
internally; that way I could get the inner labels to be center-aligned
vertically. I think that would also simplify the StyledLabel layout code.

> > Instead of embedding/inserting views, would it be possible to
> > allow
> > installing hooks to the text-layout code instead?

That sounds like a lot of work required on the part of the client. I'd rather
just be able to embed a view within the text.

[sky, 2016-03-31 15:42:10]

Embedding a view seems like a reasonable thing to want to do. That
said, if we don't have a real use now, why add the complexity? I
wouldn't want to expose the implementation details of textlayout to
consumers (meaning boxlayout or any sort of layout). If we add this
functionality I like specifying the views to add and that's all the
consumer need do.

  -Scott

On Wed, Mar 30, 2016 at 5:36 PM,  <mgiuca@chromium.org> wrote:
> On 2016/03/30 16:14:42, sky wrote:
>> What is the specific use case that is needed?
>
> See https://crbug.com/596799. The specific case is
> https://codereview.chromium.org/1820243002 / https://crbug.com/576531 but
> that
> has the odd distinction of only being needed for 1 milestone, then it gets
> deleted. So if this can't be landed in M51 then I don't actually need it
> (and I
> doubt it will). So... no actual use case required as of now.
>
> However, it would be useful for other things like the fullscreen bubble
> (https://bugs.chromium.org/p/chromium/issues/attachment?aid=219631&inline=1)
> which is currently implemented as three labels in a BoxLayout. It would be
> nicer
> if I could just insert the "Esc" box as an embedded view of a styled label.
> Note
> that the BoxLayout trick only works because the text is not multi-line,
> which is
> why I consider it a hack.
>
>>
>> -Scott
>>
>> On Wed, Mar 30, 2016 at 7:28 AM, <mailto:sadrul@chromium.org> wrote:
>> > +msw@, sky@
>> >
>> > This looks like a nice feature to have, but may be very difficult to
>> > implement
>> > correctly.
>
> I think the correct implementation is to change StyledLabel to use a
> BoxLayout
> internally; that way I could get the inner labels to be center-aligned
> vertically. I think that would also simplify the StyledLabel layout code.
>
>> > Instead of embedding/inserting views, would it be possible to
>> > allow
>> > installing hooks to the text-layout code instead?
>
> That sounds like a lot of work required on the part of the client. I'd
> rather
> just be able to embed a view within the text.
>
> https://codereview.chromium.org/1819303002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2016-03-31 16:14:59]

Also, for the record I don't like passing around views (at least in
ui/views code) via scoped_ptr. I say this as the ownership of views
changes depending upon set_client_owned. I realize in your patch you
made StyledLabel also invoke set_client_owned. That is extra confusing
as set_client_owned is intended for users of views, and not something
to change on a client supplied view.

  -Scott

On Thu, Mar 31, 2016 at 8:42 AM, Scott Violet <sky@chromium.org> wrote:
> Embedding a view seems like a reasonable thing to want to do. That
> said, if we don't have a real use now, why add the complexity? I
> wouldn't want to expose the implementation details of textlayout to
> consumers (meaning boxlayout or any sort of layout). If we add this
> functionality I like specifying the views to add and that's all the
> consumer need do.
>
>   -Scott
>
> On Wed, Mar 30, 2016 at 5:36 PM,  <mgiuca@chromium.org> wrote:
>> On 2016/03/30 16:14:42, sky wrote:
>>> What is the specific use case that is needed?
>>
>> See https://crbug.com/596799. The specific case is
>> https://codereview.chromium.org/1820243002 / https://crbug.com/576531 but
>> that
>> has the odd distinction of only being needed for 1 milestone, then it gets
>> deleted. So if this can't be landed in M51 then I don't actually need it
>> (and I
>> doubt it will). So... no actual use case required as of now.
>>
>> However, it would be useful for other things like the fullscreen bubble
>> (https://bugs.chromium.org/p/chromium/issues/attachment?aid=219631&inline=1)
>> which is currently implemented as three labels in a BoxLayout. It would be
>> nicer
>> if I could just insert the "Esc" box as an embedded view of a styled label.
>> Note
>> that the BoxLayout trick only works because the text is not multi-line,
>> which is
>> why I consider it a hack.
>>
>>>
>>> -Scott
>>>
>>> On Wed, Mar 30, 2016 at 7:28 AM, <mailto:sadrul@chromium.org> wrote:
>>> > +msw@, sky@
>>> >
>>> > This looks like a nice feature to have, but may be very difficult to
>>> > implement
>>> > correctly.
>>
>> I think the correct implementation is to change StyledLabel to use a
>> BoxLayout
>> internally; that way I could get the inner labels to be center-aligned
>> vertically. I think that would also simplify the StyledLabel layout code.
>>
>>> > Instead of embedding/inserting views, would it be possible to
>>> > allow
>>> > installing hooks to the text-layout code instead?
>>
>> That sounds like a lot of work required on the part of the client. I'd
>> rather
>> just be able to embed a view within the text.
>>
>> https://codereview.chromium.org/1819303002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Matt Giuca, 2016-03-31 23:31:40]

Description was changed from

==========
StyledLabel: Added AddEmbeddedView method.

Allows arbitrary views to be embedded within the label text.

WORK IN PROGRESS. DO NOT SUBMIT.

BUG=596799
==========

to

==========
StyledLabel: Added AddEmbeddedView method.

Allows arbitrary views to be embedded within the label text.

WORK IN PROGRESS. DO NOT SUBMIT.

BUG=596799
==========

[Matt Giuca, 2016-03-31 23:41:32]

On 2016/03/31 15:42:10, sky wrote:
> Embedding a view seems like a reasonable thing to want to do. That
> said, if we don't have a real use now, why add the complexity?

I did have a real use for it a week ago but now it's gotten too close to the
deadline so I don't think we can use it any more (and as I said, it was only
needed for 1 version).

So I'm just closing this for now.

BUT my concern is that someone will need something like this in the future and
won't be able to find this and will just have to hack it somehow or reimplement
this functionality. My main gripe with the Views system is that there is an
understandable reluctance to add generally useful functionality for a single use
case, and the pattern I see emerging is that all the clients end up hacking
around or implementing their own versions of the general functionality. Here I
saw a pattern that I've needed twice now so I thought I'd try to solve it in a
general way.

> I
> wouldn't want to expose the implementation details of textlayout to
> consumers (meaning boxlayout or any sort of layout). If we add this
> functionality I like specifying the views to add and that's all the
> consumer need do.

SGTM.

> Also, for the record I don't like passing around views (at least in
> ui/views code) via scoped_ptr. I say this as the ownership of views
> changes depending upon set_client_owned. I realize in your patch you
> made StyledLabel also invoke set_client_owned. That is extra confusing
> as set_client_owned is intended for users of views, and not something
> to change on a client supplied view.

That is why I passed it as a scoped_ptr; then the StyledLabel becomes the
undisputed single owner of the view and is allowed to do things like call
set_client_owned --- StyledLabel is now the client, NOT whoever passed it in to
AddEmbeddedView. I need to call set_client_owned on the view because every time
StyledLabel re-layouts, it deletes all of its children and starts again. I need
to make sure that all of the Labels that were created by the StyledLabel get
deleted, but the embedded views stay alive and owned by the StyledLabel's vector
of scoped_ptrs.

[sky, 2016-04-01 01:58:54]

On Thu, Mar 31, 2016 at 4:41 PM,  <mgiuca@chromium.org> wrote:
> On 2016/03/31 15:42:10, sky wrote:
>> Embedding a view seems like a reasonable thing to want to do. That
>> said, if we don't have a real use now, why add the complexity?
>
> I did have a real use for it a week ago but now it's gotten too close to the
> deadline so I don't think we can use it any more (and as I said, it was only
> needed for 1 version).
>
> So I'm just closing this for now.
>
> BUT my concern is that someone will need something like this in the future
> and
> won't be able to find this and will just have to hack it somehow or
> reimplement
> this functionality. My main gripe with the Views system is that there is an
> understandable reluctance to add generally useful functionality for a single
> use
> case, and the pattern I see emerging is that all the clients end up hacking
> around or implementing their own versions of the general functionality. Here
> I
> saw a pattern that I've needed twice now so I thought I'd try to solve it in
> a
> general way.

I don't think there is a one size fits all for every scenario. In
chrome code we've generally tried to remove unused features as they
add complexity. If we need a feature though, I think it should be
implemented in the most ideal locations. Some times that is client
code, sometimes that is near the main code.

>> I
>> wouldn't want to expose the implementation details of textlayout to
>> consumers (meaning boxlayout or any sort of layout). If we add this
>> functionality I like specifying the views to add and that's all the
>> consumer need do.
>
> SGTM.
>
>> Also, for the record I don't like passing around views (at least in
>> ui/views code) via scoped_ptr. I say this as the ownership of views
>> changes depending upon set_client_owned. I realize in your patch you
>> made StyledLabel also invoke set_client_owned. That is extra confusing
>> as set_client_owned is intended for users of views, and not something
>> to change on a client supplied view.
>
> That is why I passed it as a scoped_ptr; then the StyledLabel becomes the
> undisputed single owner of the view and is allowed to do things like call
> set_client_owned --- StyledLabel is now the client, NOT whoever passed it in
> to
> AddEmbeddedView. I need to call set_client_owned on the view because every
> time
> StyledLabel re-layouts, it deletes all of its children and starts again. I
> need
> to make sure that all of the Labels that were created by the StyledLabel get
> deleted, but the embedded views stay alive and owned by the StyledLabel's
> vector
> of scoped_ptrs.

Again, set_client_owned is intended for clients and not something that
views should use internally (in most cases). I don't think scoped_ptr
is a good match with set_client_owned.

  -Scott

> https://codereview.chromium.org/1819303002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.