[PATCH 1/2 weston] toytoolkit: Correct improper usage of opaque for button widgets.

[Ander Conselvan de Oliveira]

On 08/07/2012 02:03 PM, Scott Moreau wrote:
>   Hi Ander,
>
> On Tue, Aug 7, 2012 at 2:57 AM, Ander Conselvan de Oliveira
> <conselvan2 at gmail.com <mailto:conselvan2 at gmail.com>> wrote:
>
>     What exactly does this fix? Your commit message doesn't make it that
>     obvious.
>
>
> I assumed the problem was obvious. The definition of the word opaque
> makes it clear. In frame_button_redraw_handler() the code returns (i.e.
> does not draw the buttons) if widget->opaque is set. It should be the
> other way around.
>
>
>     Weston commit 010f98b0 added the opaque field to struct widget to
>     "track and report [...] opaque regions", i.e., the region of a
>     surface whose alpha channel is 1.
>
>
> I don't think this commit is relevant here. What is relevant is the
> meaning of opaque.

My point exactly. The reason I pointed this commit out is because it 
introduced the opaque field with the purpose of tracking the opaque 
region of a window.

The protocol defines the opaque region as "the region of the surface 
that contain opaque content.  The opaque region is an optimization hint 
for the compositor that lets it optimize out redrawing of content behind 
opaque regions.  Setting an opaque region is not required for correct 
behaviour, but marking transparent content as opaque will result in 
repaint artifacts."

So if the opaque field is 0 for a given widget it does *not* mean that 
this widget is hidden or invisible. It only means that the contents 
might contain pixels whose alpha component is different than one. In 
other words, if a surface is opaque its contents obscure whatever is 
below them while otherwise alpha blending is needed.

Later the frame_button code started to use opaque as a visibility field.

(since cgit.fd.o is down at the moment I will post
> some snippets) For widget_set_transparent() you have
>
> widget_set_transparent(struct widget *widget, int transparent)
> {
>      widget->opaque = !transparent;
> }
>
>     It seems to me the frame button code is abusing this field to hide
>     the buttons in fullscreen mode.
>
>
> This may be the case but see window.c~:1300 frame_resize_handler()
>
>
>      if (child->opaque) {
>          widget->window->opaque_region =
>              wl_compositor_create_region(display->compositor);
>          wl_region_add(widget->window->opaque_region,
>                    opaque_margin, opaque_margin,
>                    widget->allocation.width - 2 * opaque_margin,
>                    widget->allocation.height - 2 * opaque_margin);
>      }
>
> it adds the region if opaque is set.
>
>     IMO, either way you define a visibility value based on the opacity
>     value results in improper usage.
>
>
> I did not write this code, I'm just building on top of it while trying
> to improve it.

My point was that since opacity and visibility are not the same thing, 
defining visibility as opaque == 0 or opaque == 1 is equally wrong. Your 
patch doesn't change any behavior, it only changes if we go with the 
former or the latter.

> If you have a better idea, I would be interested to know
> what you think.

One simple solution is to add a visibility field to struct frame_button. 
Or have a general visibility switch on every widget.


Best regards,
Ander