[cairo] Use _cairo_rectangle_intersect in more places for intersection checking

[Carl Worth]

On Sun, 28 Oct 2007 04:36:01 +0000, "=?ISO-8859-1?Q?BJ=F6rn_Lindqvist?=" wrote:
> Alright. Here are the two patches again with all indentation redone to
> tabs. Note that it will still look inconsistent in some places when
> viewing the patch. This is not my fault, the first tab character is
> always only 7 space long when printed in a diff.

Well, there is the fact that we do the first-level of indentation with
4 spaces, so those don't hide the extra diff-added space like an
initial tab. But that won't actually result in anything that looks
obviously inconsistent in the diff output. (It won't lead to a column
that wanders left and right for example.)

I do appreciate you submitting your code to improve cairo. And I don't
want to discourage you from doing that, but there are still a few
things to fix in this submission.

The patches as submitted are now doing several different things that
should be separated into separate commits:

	1. Changing existing indentation

	2. Adding documentation

	3. Refactoring a repeated calculation into a shared place

Those first two changes don't affect the correctness of the
implementation, but the third one does. So it would be much easier to
review it if it were separated.

Also, you're submitting plain diffs which means that before anyone can
merge this into cairo, they will have to fabricate commit messages for
each commit. If you instead make git commits yourself then you can
submit patches with commit messages attached, saving the reviewer that
additional work, (and preventing the reviewer from missing something
essential in the description).

Finally, writing your own commit messages in advance provides you a
huge benefit as far as keeping separate changes separate. If you find
yourself making a list like the above or saying things like "also" in
the commit message, then those are clues to you that things need to be
separated.

If any of the above is unclear, (how to make git commits, how to use
git-format-patch to prepare them for submission, how to split existing
commits into multiple pieces), please ask and we'd be more than happy
to help you with this process.

We do want cairo to improve, and we appreciate your help.

-Carl

PS. A couple of notes follow on indentation, (which I really wouldn't
be mentioning here except that it was already raised as a topic in
this thread).

> +    if (interest) {
> +	if (!_cairo_rectangle_intersect (&read_rect, interest)) {

This is an example of a case you could have been referring to when you
said the patch would have inconsistent indentation that is not your
fault. It is true that in the original diff, this indentation looks
like it advances only 3 columns instead of 4. This is directly caused
by cairo's 4-column indent scheme, (and could be avoided by using a
tab-only indent scheme like the Linux kernel does for example). But I
don't mind this at all since it doesn't stand out noticeably when
reviewing the diff.

Notice that in my reply here, the problem has gotten worse, since my
email composer has added two additional characters so the apparent
indentation is now only 1 column. And that does look quite nasty, I
admit.

>      if (rect_out)
> -    {
> -	rect_out->x = x1;
> -	rect_out->y = y1;
> -	rect_out->width = width;
> -	rect_out->height = height;
> -    }
> +        *rect_out = read_rect;

Here you are introducing a new line of code that is indented with
spaces instead of a tab, and this one is noticeable, (the code being
removed is clearly indented differently than the code being added), so
this should be fixed.

> -    if (image_rect) {
> -	image_rect->x = x1;
> -	image_rect->y = y1;
> -	image_rect->width = x2 - x1;
> -	image_rect->height = y2 - y1;
> -    }
> +    if (image_rect)
> +        *image_rect = read_rect;

Same here.

>  	imagerep = xcb_get_image_reply(surface->dpy,
>  				    xcb_get_image(surface->dpy, XCB_IMAGE_FORMAT_Z_PIXMAP,
> -						surface->drawable,
> -						x1, y1,
> -						x2 - x1, y2 - y1,
> -						AllPlanes), &error);
> +						   surface->drawable,
> +						   read_rect.x,
> +						   read_rect.y,
> +						   read_rect.width,
> +						   read_rect.height,
> +						   AllPlanes), &error);

This one also stood out to me since it has that same pattern of code
being removed at one indentation level and code being added at
another. This case is different though as your code is not introducing
incorrect indentation, but is actually correcting the alignment of
some code that was originally poorly formatted.

I made a point above about separating commits with logically
independent pieces. To be honest though, if, when making a small
change you notice an indentation problem or some missing
documentation, then that's generally not a bad thing to just fix on
the spot. So I wouldn't really object to a single commit for this
change here. (But the general rule of splitting commits _is_ still
important.)

Also, the line of code with "xcb_get_image(" is still incorrectly
indented here. That would be a fix that's outside the lines of code
that you have to touch for your change here, but it would be nice to
fix it since you're in the area. So it might still be worth a separate
commit for reindentation to fix junk like that.

The other reason you might want to keep the re-indentation and
documentation as separate patches is to be better able to show off
with the final diffstat of the functional patch. We *love* commits
that successfully remove more lines of code than they add, so it's
often worth a little extra effort to make that happen.

> -			 surface->depth,
> -			 pixmap,
> -			 surface->drawable,
> -			 x2 - x1, y2 - y1);
> +			   surface->depth,
> +			   pixmap,
> +			   surface->drawable,
> +			   read_rect.width, read_rect.height);

As above. This is bad alignment in the original code.

> -		     x1, y1, 0, 0, x2 - x1, y2 - y1);
> +			read_rect.x, read_rect.y,
> +			0, 0,
> +			read_rect.width, read_rect.height);

Clearly cairo-xcb-surface.c is a total mess with regard to alignment
of these multi-line function calls. It's definitely worth a separate
commit first to fix this up since presumably there are many similar
problems that your work doesn't happen to hit.

I could do that easily with a quick "M-x indent-region" and commit,
but I'll hold off since I don't want to make things more difficult for
you by introducing conflicts.

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.cairographics.org/archives/cairo/attachments/20071029/34e0a3b2/attachment.pgp