[Libreoffice] [PUSHED] Docuview code cleanup

[Matteo Casalin]

Hi Michael,
     please find attached:
* a revised patch, with a new ImplDrawSymbol;
* a picture with the drawings of all symbols, produced by both the old
   and by the new routines. All symbols were drawn with different sides
   of their target rectangle, and for each rectangle size 4 full
   sequences of symbols are drawn:
   - 1st one: target rectangle in red, original symbol
   - 2nd one: target rectangle in green, new symbol
   - 3rd one: original symbol with normal background
   - 4th one: new symbol with normal background
   Please note that the white contour can exceed the symbol rectangle
   "by design", since it's shifted 1 pixel right and 1 pixel down.
   This picture is provided for a simple comparison and not for
   validating the code, which deserves a review and possibly a test on
   the field.
Please find further comments interleaved.

The attached patch is contributed under LGPL3+/MPL1.1 license.

On 11/23/2011 04:02 PM, Michael Meeks wrote:
> 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 :-)

Yeah, using polygons could reduce that code, but I just begun 
contributing and I don't feel comfortable with such a big change, at 
least for now. Besides, I had a quick look at (rendering of) polygons 
and it looks a little too complicated for such small symbols and, if you 
take a look at the circles generated by the original ImplDrawSymbols 
(which made use of polygons), you'll see that the results were not so 
precise. This requires further investigation, anyhow, since also "line" 
routines are quite complex and are called many times for each symbol.

> 	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.

No problem, I see that there's a lot of activity in the repository :)
I'm planning to do some more cleanups in Docuview, I'll post them little 
by little. Is this kind of activities appreciated or would bug-solving 
be better? I'm asking this because this task was chosen by chance, more 
for training than for other reason.
Another question on preferred behaviour for future contributions: should 
I have posted this new patch as a new mail, with an explicit [PATCH] 
header in its subject?

> 	Thanks,
>
> 		Michael.
>

Thanks
Matteo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CmpDrawSymbol.png
Type: image/png
Size: 43740 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20111124/19cfc10a/attachment-0001.png>