[Spice-devel] [PATCH 3/5] DisplayChannel: start documenting drawable tree

[Jonathon Jongsma]

On Fri, 2017-02-10 at 06:56 -0500, Frediano Ziglio wrote:
> > 
> > 

[snip]

> > +/* Add a @drawable (with a shadow) to the current ring.
> > + * The return value indicates whether the new item should be added
> > to the
> > pipe */
> 
> it's @drawable or @item ?

I guess it should be @item.

> 
> >  static int current_add_with_shadow(DisplayChannel *display, Ring
> > *ring,
> >  Drawable *item)
> >  {

[snip]

> 
> > diff --git a/server/tree.h b/server/tree.h
> > index 35b78ab..e579dd0 100644
> > --- a/server/tree.h
> > +++ b/server/tree.h
> > @@ -43,12 +43,18 @@ struct TreeItem {
> >      RingItem siblings_link;
> >      uint32_t type;
> >      Container *container;
> > +    /* rgn holds the region of the item. As additional items get
> > added to
> > the
> > +     * tree, this region may be modified to represent the portion
> > of the
> > item
> > +     * that is not obscured by other items */
> 
> I'm a bit confused by the "to represent the portion...". Does it mean
> that
> the portion is removed, right? Maybe "this region may be modified to
> exclude
> the portion of the item ..." ?

I think my original comment is accurate but perhaps a little unclear.
If I changed to your wording, I would also have to change "not
obscured" to "obscured".  For example:

"this region may be modified to exclude the portion of the item that is
obscured..."

vs.

"this region may be modified to represent the portion of the item that
is *not* obscured"

They are basically equivalent I think.  I can use your wording though
if you think it's clearer.


> 
> >      QRegion rgn;
> >  };
> >  
> >  /* A region "below" a copy, or the src region of the copy */
> >  struct Shadow {
> >      TreeItem base;
> > +    /* holds the union of all parts of this source region that
> > have been
> > +     * obscured by a drawable item that has been subsequently
> > added to the
> > tree
> > +     */
> 
> Not entirely sure of this.

Yes, it's complicated, but I think the description is accurate.

> 
> There are 2 region of code that modify on_hold:
> - excluding a region from an item with a shadow
>                 region_and(&and_rgn, &shadow->on_hold);
>                 if (!region_is_empty(&and_rgn)) {
>                     region_exclude(&shadow->on_hold, &and_rgn);
>   this remove part of the region

This part of the code is removing that part of the on_hold region
because we just removed that same region from the Shadow itself. In
other words, the on_hold  region must always be a subset of the
Shadow's region. It doesn't make sense to have a region stored in
Shadow's on_hold unless it's actually a part of the Shadow's region. 

> - excluding a region from a shadow
>             region_or(&shadow->on_hold, &and_rgn);
>   this add a region.

This is the part of the region that has been obscured by a new item, so
we add it to on_hold.
> 
> Both in __exclude_region, a function we are not sure what's doing
> and for which purpose.
> 
> >      QRegion on_hold;
> >  };
> >  
> 
> Frediano