[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