Re: [Libreoffice] [PUSHED] Docuview code cleanup

Hi Michael,
    please also note that for SYMBOL_CHECKMARK I kept both normal and 
mirrored versions of the symbol, as in the original routine, although 
the comment

// #106953# never mirror checkmarks

seems to mean that this is not desired. But I don't know if the 
comment is obsolete, misleading (could it mean that the bug was that 
checkmarks was not mirrored, as they should?) or correct (and in the 
latter case implementation is wrong). As a beginner I really don't 
know where to search for such a bug report and, honestly, I'm a bit 
puzzled by bug identifiers (fdo#, i# or whatelse). I did some internet
research about checkmarks in RTL, without results. Unless the original
bug report is found, this issue should/could eventually be clarified 
with some RTL people.

Thanks again
Matteo

--------------------------------------------------
Matteo Casalin <matteo.casalin@poste.it> wrote:
(24/11/2011 02:04)

> 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