Cleaning up the paint API of KoShape

[jaham at gmx.net]

On Sunday 23 January 2011 15:36:15 Inge Wallin wrote:
> There is a big wart on the KoShape API and that is the one for painting.
> Currently we have these two functions:
> 
>  virtual void paint(QPainter &painter, const KoViewConverter &converter) =
> 0; virtual void paintDecorations(QPainter &painter,
>                               const KoViewConverter &converter,
>                               const KoCanvasBase *canvas);
> 
> The biggest problem is the KoViewConverter parameter.  There is no reason
> why this should be necessary. It should be the responsibility of the
> calling code to set up the painter so that the shape itself can just paint
> from (0, 0) to size() and that's it (if we for the moment ignore painting
> outside the shape).

I fully support the idea to clean up the painting API. It would even be 
applicable to how tools are painting, as they do essentially similar things in 
their paint routine.
If I recall correctly I even once started something similar, e.g. applying the 
zoom only once when setting up the painter. But what we still need to consider 
is that we sometimes need to know the zooming factors when painting. For 
instance when painting handles which should have the same size, independent 
from the actual zoom, you need to know the scaling to compensate for when 
painting.
So I think what would be a prerequisite is to closely check all the shapes and 
their painting routines to come up with a list of required data which needs to 
be passed to the painting routine.

> 
> Now we are forced to do things like
> 
>  painter.fillRect(converter.normalToView(QRectF(QPointF(0.0,0.0), size())),
> background());
> 
> This is actually the way that is documented in the apidox.
> 
> The other problem is that having a separate paintDecorations() method can
> potentially introduce inefficiencies. My suggestion would instead be to
> introduce a new parameter to KoShape::paint(): PaintMode.  This could be an
> enum with the values { paintContentsOnly, paintDecorationsOnly, paintAll},
> and maybe in the future more values. I can also imagine it as a bitmap
> with Contents and Decorations as bits. I'm open to suggestions here.
> 
> So the resulting API would become:
> 
>  typedef enum {
>    paintContentsOnly,
>    paintDecorationsOnly,
>    paintAll
>  } ShapePaintMode;
> 
>  virtual void paint(QPainter &painter, ShapePaintMode paintMode) = 0;
> 
>  ...or the alternative definition of ShapePaintMode.

Why not create a class KoShapePaintingContext (just like we have 
KoShapeLoadingContext, KoShapeSavingContext, etc.) ? That object one can be 
passed as the only parameter, providing access to the painter, paint mode and 
other data/methods (canvas, KoToolBase::handlePaintRect, etc) we might need 
(see above).

> 
> If this is ok with the rest of you, I am willing to do the work.  My
> suggestion for the workflow to create as few interruptions as possible
> would be the following. I discussed this with Casper.
> 
> 1. * Rename the current KoShape::paint() as KoShape::paintOld() and mark
> that as deprecated.
>    * Change all the places that call KoShape::paint() to instead call
> paintOld().
>    * At the same time, define a new KoShape::paint() with the new signature
> as defined above and create empty implementations in all shapes.
>    * At the same time mark KoShape::paintDecorations() as deprecated.
>    Step 1 is done in a branch, and should be possible to complete fairly
> quickly, in under a day at least.
> 
> 2. Put the result of step 1 up for review and commit it.
> 
> 3. Work in another branch (or perhaps the same) to implement the new
> paint() methods in all shapes.
> 
> 4. Put this up for review and commit it.
> 
> 5. Fix all the places in the code that call the paintOld() and
> paintDecoration() methods to instead use the new API.  This will probably
> take a bit longer and this can be merged with master also during the work,
> possibly after review(s).
> 
> 6. Remove paintOld() and paintDecorations() from KoShape and all
> implementations from shapes.
> 
> Done.

I am not sure why you would commit the intermediate steps. I think it would be 
better to just let them be reviewed and then carry on with the next step in 
the branch. Merge back to master once all steps are reviewed and completed.

Ciao Jan