[PATCH 1/2] render/input: set anicursor hooks to control rendering

[Peter Hutterer]

On Wed, Aug 26, 2009 at 05:38:53PM +0300, Vignatti Tiago (Nokia-D/Helsinki) wrote:
> On Wed, Aug 26, 2009 at 03:16:46AM +0200, ext Peter Hutterer wrote:
> > On Tue, Aug 25, 2009 at 05:03:15PM +0300, Tiago Vignatti wrote:
> > > +void
> > > +AnimCursorStart(ScreenPtr pScreen)
> > > +{
> > > +    AnimCurScreenPtr    as;
> > > +
> > > +    as = (AnimCurScreenPtr) xalloc (sizeof (AnimCurScreenRec));
> > > +    if (!as)
> > > +        FatalError("AnimCursor: failed to allocate memory\n");
> > > +
> > > +    /* AnimCursorStart can be stacked without problems. For this we do a
> > > +     * comparision on BlockHandler function, guaranteeing that such function
> > > +     * will not point to ifself. Address function comparision is not the best
> > > +     * way, but eh... */
> > 
> > any code that is "... not the best way, but eh..." usually guarantees that
> > sometime, someone will spend a significant portion of time hating you. at
> > least mark spots with a FIXME or XXX so they are immediately obvious.
> 
> Agreed. A simply counter solve this.
> 
>  
> > > +    if (pScreen->BlockHandler != AnimCurScreenBlockHandler) {
> > > +        Wrap(as, pScreen, CloseScreen, AnimCurCloseScreen);
> > > +        Wrap(as, pScreen, BlockHandler, AnimCurScreenBlockHandler);
> > > +        Wrap(as, pScreen, CursorLimits, AnimCurCursorLimits);
> > > +        Wrap(as, pScreen, DisplayCursor, AnimCurDisplayCursor);
> > > +        Wrap(as, pScreen, SetCursorPosition, AnimCurSetCursorPosition);
> > > +        Wrap(as, pScreen, RealizeCursor, AnimCurRealizeCursor);
> > > +        Wrap(as, pScreen, UnrealizeCursor, AnimCurUnrealizeCursor);
> > > +        Wrap(as, pScreen, RecolorCursor, AnimCurRecolorCursor);
> > > +        SetAnimCurScreen(pScreen,as);
> > > +    }
> > > +}
> > 
> > This seems like overkill. The issue at hand are the Block- and
> > WakeupHandlers, the rest shouldn't have much impact. I see two simpler
> > solutions:
> > - just removing the block handler from the stack so it doesn't get called if
> >   the cursor is invisible.
> 
> that's exactly what I've done. 
> 
> At first it seems overkill to unwrap the functions when the cursor is
> invisible. But this is the only way that I see to remove the handler, given
> that we're already doing a confusing stack of functions there. 

my point was - can we not just unwrap the block handler and leave the others
be?

> > - using Start/Stop to set a flag to skip the loop inside the block handler
> >   and basically make the block handler a noop. I think this would be much
> >   more readable, debuggable and has the same effect.
> 
> I really prefer to just turn off all this rendering functions. It doesn't make
> sense to call them if the cursor is invisible. So I'd go for the first
> solution.

Do you turn off the miSprite rendering functions too then? 

if you don't update the wakeup counter, you only get called when someone
else woke up the server and if then all you're doing is pass through to the
next handler you're not really wasting anything. 
Bonus points are better readability, shareable with the miSprite rendring
stuff and much easier to debug. I always found the wrap-unwrap game very
delicate. I'm biased here, the hours I spent debugging broken wrap/unwraps
fell rather short of being the most exciting time in my life.

Unwrapping might make sense if there aren't any BlockHandlers left and the
whole thing becomes a noop. This is unlikely and as long as the handlers are
called anyway, you might as well go for readability.
 
> > no _X_EXPORTS please. Init and CursorCreate shouldn't be _X_EXPORT either,
> > while we're at it.
> 
> agreed.
 
Cheers,
  Peter