Cleaning up the paint API of KoShape

[C. Boemann]

On Sunday 23 January 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);
> > 
> > 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).
+1
and a forprint boolean in there, rather than say only pain decorations. Let's 
just give the paint method the info it needs to paint what it want without 
actually telling it what to paint.

> > 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
> complete
Hmm I'me actually changing my mind and thinking it would be okay to do it all 
and then merge. But the original idea was to get the dependent work out there 
as soon as possible. Thinking about that though, I see how that wouldn't reall 
give an advantage anyway