Cleaning up the paint API of KoShape

[Thorsten Zachmann]

On Sunday, January 23, 2011 18:02:53 jaham at gmx.net wrote:
> 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);
> > 

not sure if it makes sense to merge paint and paintDecorations as then if a 
shape paints decorations also has to add the test if they need to be painted 
by itself whereas it is now only tested once in the KoShapeManager.

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

It is only partly true. Most shapes don't care about that as they call

applyConversion(painter, converter); 

at the beginning which means that most shapes don't care about the thing you 
wrote above. However e.g. some shapes care as with the above information it is 
possible to reduce the stuff needed for painting. E.g. the picture shape gets 
the exact size and paints it content there without applying additional 
transformation which most likely gives a better result. 
Also I think one reason for passing the KoViewConverter was that shapes can 
decide by themselfs to use different drawing e.g. in very low zoom level. The 
text shape could draw only a line instread of the actual characters if zoomed 
out quite a lot.

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

+1 for me.

For animations in kpresenter something like a KoShapePaintingContext has been 
on my wishlist for quite some time. It would be good if the context could be 
used by all paint methods. That will make it much easier in the future to 
extend it.

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

I agree with Jan here that no  intermediate steps should be merged to master. 
Working on that in a branch is quite easy. 

If you want intermediate steps you could first convert

virtual void paint(QPainter &painter, const KoViewConverter &converter) = 0;

to 

virtual void paint(KoShapePaintingContext &context)

where the current parameters are part of the context.

As that is a finished step in itself it can be merged to master after review. 
And after that the other changes can be done.

Thorsten