[Xr] Dealing with groups in Xr

[Carl Worth]

On Apr 22, Owen Taylor wrote:
 > I took the liberty of committing a couple of tweaks -

By all means, please feel free. The xrtest module is just a little
playground with a few toys. No bullies or dictators here.

 > I switched one of the temporary surfaces to XrFormatA8, since that
 > was sufficient.

Ah good. That was one of the performance improvements this was to
enable. Glad that is possible.

 > Question I have - don't the temporary surfaces need to be 
 > freed?

Oops. Yes, of course. These are fixed now.

 > I think the new stuff is definitely good in the sense of adding
 > more power and generality.

That's good to hear.

 > I'm not sure that it really makes the code less readable, except
 > for making it longer.

I think in the case of xrknockout, it just got so long that the 3
separate groupings in the code were not easy to see anymore. More on
this below.

 > The only thing I can think of to reduce the verbosity would be
 > move back a bit in the direction you had before by introducing
 > the idea of a "temporary surface" that is part of the state.
[...]
 > Legibility could be improved at the expense of potential
 > efficiency by removing the x,y,width,height arguments
 > from XrStartTemp.

I think I'd like that. Since these are just convenience functions,
they might as well be as convenient as possible.

In which case, we'd have the same functionality that was there with
Push/PopGroup except with the needed improvement that the compositing
of the temporary surface is not tangled up with the end of drawing to
the surface.

Maybe we could go back to the old names as well:

	XrPushGroup, XrPopGroup, XrShowGroup

What think ye?

 >         /* Draw 3 circles to the punch surface, then cut
 >          * that out of the main circle in the overlay
 >          */
 >         if (XrStartTemp (r, XrFormatARGB8, 0, 0, width, height)) {
 >             draw_3circles (r, xc, yc, radius);
 > 
 >             XrEndTemp (r);
 >             XrSetOperator (r, XrOperatorOutReverse);
 >             XrShowTemp (r);
 >         }

BTW, the bracing here helps readability quite a bit. The
error-handling concept in Xr[*] has void functions everywhere. But
discretionary braces could be quite nice in a case like this, even
without the if statement.

-Carl

[*] I don't think I've ever talked much about the error-handling API
in Xr. The idea that Keith and I were working with is that checking
the return status of every function gets messy in the code, and worse,
nobody bothers to do it. So instead, the functions are all void, and
the user can check the error status at any given point with:

	XrStatus
	XrGetStatus(XrState *xrs);

In addition, any error condition in an XrState object makes it just
shut down. That is, calling more Xr functions after an error will
never cause a crash -- the functions just won't do anything.

This way it should be easy to keep code clean by calling a collection
of Xr functions and then just checking once at the end to make sure
everything worked.

As always, comments and suggestions are welcome.