[Spice-devel] [PATCH] fixup! DisplayChannel: document exclude_region() functions

[Frediano Ziglio]

> 
> Potential changes to amend with previous commit per suggestions by Uri,
> Frediano.
> ---
>  server/display-channel.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 4ee842a..5f4caf9 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -592,7 +592,7 @@ static bool current_add_equal(DisplayChannel *display,
> DrawItem *item, TreeItem
>   *
>   * If there is overlap between @rgn and the @item region, remove the
>   * overlapping intersection from both @rgn and the item's region (NOTE: it's
> - * not clear to me why this is done)
> + * not clear to me why this is done - jjongsma)
>   *
>   * However, if the item is a DrawItem that has a shadow, we add an
>   additional
>   * region to @rgn: the intersection of the shadow item's region with @rgn
>   when
> @@ -626,8 +626,8 @@ static bool current_add_equal(DisplayChannel *display,
> DrawItem *item, TreeItem
>   * @ring: a fallback toplevel ring???
>   * @item: the tree item to exclude from @rgn
>   * @rgn: the region to exclude
> - * @top_ring: ?
> - * @frame_candidate: ?
> + * @top_ring: ???
> + * @frame_candidate: ???
>   */
>  static void __exclude_region(DisplayChannel *display, Ring *ring, TreeItem
>  *item, QRegion *rgn,
>                               Ring **top_ring, Drawable *frame_candidate)
> @@ -683,7 +683,7 @@ static void __exclude_region(DisplayChannel *display,
> Ring *ring, TreeItem *item
>                      }
>                  }
>              } else {
> -                /* NOTE: It's unclear to me why this code is necessary here
> */
> +                /* TODO: document the purpose of this code */
>                  if (frame_candidate) {
>                      Drawable *drawable = SPICE_CONTAINEROF(draw, Drawable,
>                      tree_item);
>                      stream_maintenance(display, frame_candidate, drawable);
> @@ -700,13 +700,14 @@ static void __exclude_region(DisplayChannel *display,
> Ring *ring, TreeItem *item
>                  Shadow *shadow;
>  
>                  /* exclude the intersection from the 'rgn' argument as well,
> -                 * but only if the item is now empty... WHY??? */
> +                 * but only if the item is now empty.
> +                 * TODO: explain why this is necessary */
>                  region_exclude(rgn, &and_rgn);
>                  if ((shadow = tree_item_find_shadow(item))) {
>                      /* add the shadow's on_hold region back to the 'rgn'
>                      argument */
>                      region_or(rgn, &shadow->on_hold);
>                      if (!tree_item_contained_by(&shadow->base, *top_ring)) {
> -                        /* NOTE: it's unclear why top_ring is set here */
> +                        /* TODO: document why top_ring is set here */
>                          *top_ring = tree_item_container_items(&shadow->base,
>                          ring);
>                      }
>                  }
> @@ -737,8 +738,7 @@ static void __exclude_region(DisplayChannel *display,
> Ring *ring, TreeItem *item
>   * Any items that have an empty region as a result of the __exclude_region()
>   * call are removed from the tree.
>   *
> - * NOTE: I still don't have a great conceptual understanding of this
> function's
> - * intended use.
> + * TODO: What is the intended use of this function?
>   *
>   * @ring: every time this function is called, @ring is a Surface's 'current'
>   *      ring, or to the ring of children of a container within that ring.
> @@ -751,7 +751,7 @@ static void __exclude_region(DisplayChannel *display,
> Ring *ring, TreeItem *item
>   *      next item after iteration is complete (which may be different than
>   the
>   *      passed value if that item was removed from the tree
>   * @frame_candidate: usually callers pass NULL, sometimes it's the drawable
> - *      that's being added to the 'current' ring. What is its purpose?
> + *      that's being added to the 'current' ring. TODO: What is its purpose?
>   */
>  static void exclude_region(DisplayChannel *display, Ring *ring, RingItem
>  *ring_item,
>                             QRegion *rgn, TreeItem **last, Drawable
>                             *frame_candidate)
> @@ -954,9 +954,9 @@ static bool current_add(DisplayChannel *display, Ring
> *ring, Drawable *drawable)
>                           * 'on_hold' regions from the shadows of previous
>                           * iterations
>                           *
> -                         * NOTE: it's unclear to me why we only only call
> +                         * TODO: it's unclear to me why we only only call
>                           * exclude_region() for the previous item if the
>                           next
> -                         * item is obscured and has a shadow.
> +                         * item is obscured and has a shadow. -jjongsma
>                           */
>                          TreeItem *next = sibling;
>                          exclude_region(display, ring, exclude_base,
>                          &exclude_rgn, &next, NULL);
> @@ -989,7 +989,7 @@ static bool current_add(DisplayChannel *display, Ring
> *ring, Drawable *drawable)
>                  if (shadow || skip) {
>                      /* 'now' is currently set to the the item immediately
>                      AFTER
>                       * the obscured sibling that we just removed.
> -                     * NOTE: it's unclear to me Why this item is used as an
> +                     * TODO: document why this item is used as an
>                       * 'exclude_base' */
>                      exclude_base = now;
>                  }

Ack with this change

Frediano