Re: [Libreoffice] [PUSHED] Docuview code cleanup

Hi Michael,
you were faster than me: I've been playing with that code lately, 
rewriting all of the ImplDrawSymbol routine and was planning to do the
last verifications/cleanups this evening and send it today or tomorrow
with also a picture including all of the old symbols and the "new" 
ones, in both enabled and disabled states and also with "non gray" 
rectangle background to verify that they fit the desired area.
The new patch will also clean many of those local variables and avoid 
calling the bitmap/polygon functions for drawing a circle.
Sorry for this delay, but the codebase is huge, my laptop slow and I'm
a little bit rusted with c/c++. But I took the chance to also do some 
basic dbg sessions, I hope I'll be faster next time.
I was planning to have a look to easyhacks for future contributions, 
but if you think it's worthy I can focus on docuview some more :)
Hope to send you the new patch (with picture) ASAP.
Thanks
Matteo

--------------------------------------------------
Michael Meeks <michael.meeks@suse.com> wrote:
(23/11/2011 16:02)

> Hi Matteo,
>
> On Sun, 2011-11-13 at 19:54 +0100, Matteo Casalin wrote:
> >      my name's Matteo and this is my first contribution [attempt] to 
> > this wonderful piece of work, besides "spreading the word".
>
>       Cool - welcome ! and I'm sorry it took so long to get to reviewing this
> properly.
>
> > The attached patch does a little code cleanup in Docuview::DrawSymbol 
> > function and its helper, reducing local variables and calls to "real" 
> > draw functions.
>
>       :-)
>
> > Please note that:
> > * the results of reworked code was not fully tested, since I really
> >    don't know were all of those symbols are drawn, but those that I was
> >    able to verify look OK to me;
>
>       Great. We see some 'symbols' drawn on buttons often next/previous
> buttons that are hidden in various places. Personally I'd prefer to have
> alpha transparent, themed bitmaps for all of them but ... ;-)
>
> > * There are still other cleanups that can be done in that code, but I
> >    would like to have some feedback before working on them. For example,
> >    this patch could include too many changes.
>
>       So, I -think- (and I've inverted some of the senses here) that:
>
> -    if ( nMin & 0x01 )
> -        nMin--;
> ...
> -            if ( !(nMin & 0x01) )
>
>       Should be replaced by:
>
> +    const bool bMinSideIsOdd = nMin & 1;
> ..
> +            if ( bMinSideIsOdd )
>
>       Rather than !bMinSideIsOdd, since the nMin-- alters the state ;-) yet
> another reason why this unclear & unhelful code needs cleaning up
> IMHO :-)
>
>       This code is quite amazing ;-)
>
>             pDev->DrawPixel( Point( nCenterX, nTop ) );
>             for ( long i = 1; i <= n2; ++i )
>             {
>                 nTop++;
>                 pDev->DrawRect( Rectangle (Point( nCenterX-i, nTop ),
>                                 Point( nCenterX+i, nTop ) ) );
>             }
>
>       As a way to draw a triangle for an up-arrow is really quite amazing ...
> Particularly when cut/pasted as the down arrow as well. I'd love to see
> that stuff made common and replaced with pDev->DrawPolygon or similar
> instead :-) cf. tools/inc/tools/gen.hxx and vcl/inc/vcl/outdev.hxx. We
> should be able to use Polygon::Rotate() to evaporate lots of this code I
> hope, possibly we could even set anti-aliasing transiently to get a
> nicer rendered result too :-)
>
>       Anyhow - apart from changing the polarity of the bMinSideIsOdd later in
> the code, I've pushed it as is; something so broken deserves all the
> fixing it can get ASAP :-)
>
>       Sorry again for the delay; any chance you'd be interested in making
> that function fully sane ? :-) it'd be much appreciated.
>
>       Thanks,
>
>               Michael.
>
> -- 
> michael.meeks@suse.com  <><, Pseudo Engineer, itinerant idiot