[Spice-devel] [PATCH] Amend the previous commit to change the "XXX" comments

Jonathon Jongsma jjongsma at redhat.com
Thu Mar 9 20:50:11 UTC 2017


---
Here's a possible patch to be squashed with the exclude_region documentation
patch to avoid using 'XXX' comments as Frediano suggested. I also removed a
couple of things where I didn't think they added much useful information.

 server/display-channel.c | 45 ++++++++++++++++++---------------------------
 1 file changed, 18 insertions(+), 27 deletions(-)

diff --git a/server/display-channel.c b/server/display-channel.c
index 11f8404..2a6dd20 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -591,7 +591,8 @@ static bool current_add_equal(DisplayChannel *display, DrawItem *item, TreeItem
  * and @item may be modified.
  *
  * If there is overlap between @rgn and the @item region, remove the
- * overlapping intersection from both @rgn and the item's region (XXX WHY???)
+ * overlapping intersection from both @rgn and the item's region (NOTE: it's
+ * not clear to me why this is done)
  *
  * 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
@@ -677,13 +678,12 @@ static void __exclude_region(DisplayChannel *display, Ring *ring, TreeItem *item
                      * the @rgn argument */
                     region_or(rgn, &and_rgn);
                     // in flat representation of current, shadow is always his owner next
-                    // XXX: what does that mean?
                     if (!tree_item_contained_by(&shadow->base, *top_ring)) {
                         *top_ring = tree_item_container_items(&shadow->base, ring);
                     }
                 }
             } else {
-                /* XXX: ??? */
+                /* NOTE: It's unclear to me why this code is necessary here */
                 if (frame_candidate) {
                     Drawable *drawable = SPICE_CONTAINEROF(draw, Drawable, tree_item);
                     stream_maintenance(display, frame_candidate, drawable);
@@ -706,7 +706,7 @@ static void __exclude_region(DisplayChannel *display, Ring *ring, TreeItem *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)) {
-                        /* what is top_ring used for??? */
+                        /* NOTE: it's unclear why top_ring is set here */
                         *top_ring = tree_item_container_items(&shadow->base, ring);
                     }
                 }
@@ -737,8 +737,8 @@ 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.
  *
- * XXX: I still don't have a great conceptual understanding of what we're
- * trying to do by calling this function.
+ * NOTE: I still don't have a great conceptual understanding of this function's
+ * intended use.
  *
  * @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.
@@ -954,12 +954,11 @@ static bool current_add(DisplayChannel *display, Ring *ring, Drawable *drawable)
                          * 'on_hold' regions from the shadows of previous
                          * iterations
                          *
-                         * XXX: why do we only only call exclude_region() for
-                         * the previous item if the next item is obscured and
-                         * has a shadow???
+                         * NOTE: 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.
                          */
                         TreeItem *next = sibling;
-                        /* XXX: What is the intent of this call? */
                         exclude_region(display, ring, exclude_base, &exclude_rgn, &next, NULL);
                         if (next != sibling) {
                             /* the @next param is only changed if the given item
@@ -988,11 +987,10 @@ static bool current_add(DisplayChannel *display, Ring *ring, Drawable *drawable)
                 /* advance the loop variable */
                 now = ring_next(ring, now);
                 if (shadow || skip) {
-                    /* XXX: don't understand this exclude_base stuff
-                     * 'now' is currently set to the the item immediately AFTER
-                     * the obscured sibling that we just removed. Why is this
-                     * item used as an 'exclude_base'??
-                     */
+                    /* '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
+                     * 'exclude_base' */
                     exclude_base = now;
                 }
                 continue;
@@ -1002,13 +1000,11 @@ static bool current_add(DisplayChannel *display, Ring *ring, Drawable *drawable)
                 /* the sibling item is opaque and entirely contains the new drawable */
                 Container *container;
 
-                /* XXX: still don't understand this exclude_base stuff. The
-                 * first time through, @exclude_base will be NULL, but
-                 * subsequent loops may set it to something.
-                 * In addition, @exclude_rgn starts out empty, but previous
-                 * iterations of this loop may have added various
-                 * Shadow::on_hold regions to it.
-                 */
+                /* The first time through, @exclude_base will be NULL, but
+                 * subsequent loops may set it to something.  In addition,
+                 * @exclude_rgn starts out empty, but previous iterations of
+                 * this loop may have added various Shadow::on_hold regions to
+                 * it. */
                 if (exclude_base) {
                     exclude_region(display, ring, exclude_base, &exclude_rgn, NULL, NULL);
                     region_clear(&exclude_rgn);
@@ -1060,11 +1056,6 @@ static bool current_add(DisplayChannel *display, Ring *ring, Drawable *drawable)
          * Shadows that were associated with DrawItems that were removed from
          * the tree.  Add the new item's region to that */
         region_or(&exclude_rgn, &item->base.rgn);
-        /* XXX: Why do we need to call exclude_region() here again? I thought
-         * we had just iterated the whole list above to check for region
-         * intersections between this new item and existing items...
-         * NOTE: this is the only place where we call exclude_region() with a
-         * non-NULL frame_candidate argument. */
         exclude_region(display, ring, exclude_base, &exclude_rgn, NULL, drawable);
         stream_trace_update(display, drawable);
         streams_update_visible_region(display, drawable);
-- 
2.9.3



More information about the Spice-devel mailing list