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.me...@suse.com <><, Pseudo Engineer, itinerant idiot
_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
