[Spice-devel] [PATCH 06/11] Move some tree item functions to tree.[ch]
Jonathon Jongsma
jjongsma at redhat.com
Mon Nov 16 09:16:58 PST 2015
On Mon, 2015-11-16 at 08:35 -0500, Frediano Ziglio wrote:
> >
> > On Mon, Nov 16, 2015 at 12:06 PM, Frediano Ziglio <fziglio at redhat.com>
> > wrote:
> > > From: Jonathon Jongsma <jjongsma at redhat.com>
> > >
> > > Also rename some functions slightly:
> > > __find_shadow -> tree_item_find_shadow()
> > > __contained_by -> tree_item_contained_by()
> > > ring_of -> tree_item_container_items();
> > > ---
> > > server/red_worker.c | 62
> > > ++++++-----------------------------------------------
> > > server/tree.c | 50 ++++++++++++++++++++++++++++++++++++++++++
> > > server/tree.h | 5 +++++
> > > 3 files changed, 62 insertions(+), 55 deletions(-)
> > >
> > > diff --git a/server/red_worker.c b/server/red_worker.c
> > > index 556963b..4734187 100644
> > > --- a/server/red_worker.c
> > > +++ b/server/red_worker.c
> > > @@ -787,21 +787,6 @@ void display_channel_drawable_unref(DisplayChannel
> > > *display, Drawable *drawable)
> > > display->drawable_count--;
> > > }
> > >
> > > -static inline void remove_shadow(DrawItem *item)
> > > -{
> > > - Shadow *shadow;
> > > -
> > > - if (!item->shadow) {
> > > - return;
> > > - }
> > > - shadow = item->shadow;
> > > - item->shadow = NULL;
> > > - ring_remove(&shadow->base.siblings_link);
> > > - region_destroy(&shadow->base.rgn);
> > > - region_destroy(&shadow->on_hold);
> > > - free(shadow);
> > > -}
> > > -
> > > static void display_stream_trace_add_drawable(DisplayChannel *display,
> > > Drawable *item)
> > > {
> > > ItemTrace *trace;
> > > @@ -843,7 +828,7 @@ static void red_flush_source_surfaces(DisplayChannel
> > > *display, Drawable *drawabl
> > > static inline void current_remove_drawable(DisplayChannel *display,
> > > Drawable *item)
> > > {
> > > display_stream_trace_add_drawable(display, item);
> > > - remove_shadow(&item->tree_item);
> > > + draw_item_remove_shadow(&item->tree_item);
> > > ring_remove(&item->tree_item.base.siblings_link);
> > > ring_remove(&item->list_link);
> > > ring_remove(&item->surface_list_link);
> > > @@ -999,39 +984,6 @@ static void
> > > red_clear_surface_drawables_from_pipes(DisplayChannel *display,
> > > }
> > > }
> > >
> > > -static inline Shadow *__find_shadow(TreeItem *item)
> > > -{
> > > - while (item->type == TREE_ITEM_TYPE_CONTAINER) {
> > > - if (!(item = (TreeItem *)ring_get_tail(&((Container
> > > *)item)->items))) {
> > > - return NULL;
> > > - }
> > > - }
> > > -
> > > - if (item->type != TREE_ITEM_TYPE_DRAWABLE) {
> > > - return NULL;
> > > - }
> > > -
> > > - return ((DrawItem *)item)->shadow;
> > > -}
> > > -
> > > -static inline Ring *ring_of(Ring *ring, TreeItem *item)
> > > -{
> > > - return (item->container) ? &item->container->items : ring;
> > > -}
> > > -
> > > -static inline int __contained_by(TreeItem *item, Ring *ring)
> > > -{
> > > - spice_assert(item && ring);
> > > - do {
> > > - Ring *now = ring_of(ring, item);
> > > - if (now == ring) {
> > > - return TRUE;
> > > - }
> > > - } while ((item = (TreeItem *)item->container));
> > > -
> > > - return FALSE;
> > > -}
> > > -
> > > static void __exclude_region(DisplayChannel *display, Ring *ring,
> > > TreeItem
> > > *item, QRegion *rgn,
> > > Ring **top_ring, Drawable *frame_candidate)
> > > {
> > > @@ -1066,8 +1018,8 @@ static void __exclude_region(DisplayChannel
> > > *display,
> > > Ring *ring, TreeItem *item
> > > region_exclude(&shadow->on_hold, &and_rgn);
> > > region_or(rgn, &and_rgn);
> > > // in flat representation of current, shadow is
> > > always
> > > his owner next
> > > - if (!__contained_by((TreeItem*)shadow, *top_ring)) {
> > > - *top_ring = ring_of(ring, (TreeItem*)shadow);
> > > + if (!tree_item_contained_by((TreeItem*)shadow,
> > > *top_ring)) {
> > > + *top_ring =
> > > tree_item_container_items((TreeItem*)shadow, ring);
> > > }
> > > }
> > > } else {
> > > @@ -1084,10 +1036,10 @@ static void __exclude_region(DisplayChannel
> > > *display, Ring *ring, TreeItem *item
> > > Shadow *shadow;
> > >
> > > region_exclude(rgn, &and_rgn);
> > > - if ((shadow = __find_shadow(item))) {
> > > + if ((shadow = tree_item_find_shadow(item))) {
> > > region_or(rgn, &shadow->on_hold);
> > > - if (!__contained_by((TreeItem*)shadow, *top_ring)) {
> > > - *top_ring = ring_of(ring, (TreeItem*)shadow);
> > > + if (!tree_item_contained_by((TreeItem*)shadow,
> > > *top_ring)) {
> > > + *top_ring =
> > > tree_item_container_items((TreeItem*)shadow, ring);
> > > }
> > > }
> > > }
> > > @@ -2112,7 +2064,7 @@ static int current_add(DisplayChannel *display, Ring
> > > *ring, Drawable *drawable)
> > > Shadow *shadow;
> > > int skip = now == exclude_base;
> > >
> > > - if ((shadow = __find_shadow(sibling))) {
> > > + if ((shadow = tree_item_find_shadow(sibling))) {
> > > if (exclude_base) {
> > > TreeItem *next = sibling;
> > > exclude_region(display, ring, exclude_base,
> > > &exclude_rgn, &next, NULL);
> > > diff --git a/server/tree.c b/server/tree.c
> > > index ad31f09..1daa90c 100644
> > > --- a/server/tree.c
> > > +++ b/server/tree.c
> > > @@ -250,3 +250,53 @@ void container_cleanup(Container *container)
> > > container = next;
> > > }
> > > }
> > > +
> > > +/* FIXME: document weird function: go down containers, and return
> > > drawable->shadow? */
>
> This comment is quite weird. I actually cannot address it.
> Why considering only last items (ring_get_tail) ?
yeah, clearly Marc-Andre couldn't quite understand the function either, which is
probably why he added the comment. Nor can I.
>
> > > +Shadow* tree_item_find_shadow(TreeItem *item)
> > > +{
> > > + while (item->type == TREE_ITEM_TYPE_CONTAINER) {
> > > + if (!(item = (TreeItem *)ring_get_tail(&((Container
> > > *)item)->items))) {
> > > + return NULL;
> > > + }
> > > + }
> > > +
> > > + if (item->type != TREE_ITEM_TYPE_DRAWABLE) {
> > > + return NULL;
> > > + }
> > > +
> > > + return ((DrawItem *)item)->shadow;
> > > +}
> > > +
> > > +Ring *tree_item_container_items(TreeItem *item, Ring *ring)
> > > +{
> > > + return (item->container) ? &item->container->items : ring;
> > > +}
> > > +
> > > +int tree_item_contained_by(TreeItem *item, Ring *ring)
> > > +{
> > > + spice_assert(item && ring);
> > > + do {
> > > + Ring *now = tree_item_container_items(item, ring);
> > > + if (now == ring) {
> > > + return TRUE;
> > > + }
> > > + } while ((item = (TreeItem *)item->container));
> > > +
> > > + return FALSE;
> > > +}
> > > +
> > > +void draw_item_remove_shadow(DrawItem *item)
> > > +{
> > > + Shadow *shadow;
> > > +
> > > + if (!item->shadow) {
> > > + return;
> > > + }
> > > + shadow = item->shadow;
> > > + item->shadow = NULL;
> > > + /* shadow_free? */
>
> I think this comment is suggesting to add the function,
> would be better
>
> /* TODO add and use a shadow_free */
>
> I think there are 3 choices:
> - remove the comment
> - change to a proper TODO
> - do it (add the function and call it)
Yeah, the comment is not entirely unambiguous. But I don't see much point to
creating a separate function for freeing the shadow since it's only used here.
I'd probably just remove the comment.
>
> > > + ring_remove(&shadow->base.siblings_link);
> > > + region_destroy(&shadow->base.rgn);
> > > + region_destroy(&shadow->on_hold);
> > > + free(shadow);
> > > +}
> > > diff --git a/server/tree.h b/server/tree.h
> > > index 01d4ff9..8b9c6ba 100644
> > > --- a/server/tree.h
> > > +++ b/server/tree.h
> > > @@ -80,6 +80,11 @@ static inline int is_opaque_item(TreeItem *item)
> > > }
> > >
> > > void tree_item_dump (TreeItem *item);
> > > +Shadow* tree_item_find_shadow (TreeItem *item);
> > > +int tree_item_contained_by (TreeItem *item, Ring
> > > *ring);
> > > +Ring* tree_item_container_items (TreeItem *item, Ring
> > > *ring);
> > > +
> > > +void draw_item_remove_shadow (DrawItem *item);
> > > Shadow* shadow_new (DrawItem *item,
> > > const
> > > SpicePoint *delta);
> > > Container* container_new (DrawItem *item);
> > > void container_free (Container
> > > *container);
> > > --
> > > 2.4.3
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> > Seems okay, ACK!
> >
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list