Cleaning up the paint API of KoShape

[Inge Wallin]

I'll reply to both Jan and Casper here instead of doing two separate mails.

On Sunday, January 23, 2011 20:20:36 C. Boemann wrote:
> 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.

Ok, I'll try to do that, but I would appreciate some help here. On the other 
hand, if we have the KoShapePaintContext, we could easily add info that we 
forgot to begin with.

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

Good idea.  I'll do that.

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

I don't think this is a good idea, though.  The paint method need to know what 
to do but not necessarily why.  The problem with giving reasons is that we may 
come up with more reasons in the future, and then we would have to change all 
the paint methods in all the shapes to cover that.


> > > [Deleted the proposed sequence of steps]

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

Mainly to minimize the impact on those that have unsaved changes in other 
branches. The smaller the changes at every point, the smaller the impact.

> 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

I still think it's the correct way of thinking.  And it would also let those 
that have external shapes work in parallel. I know we haven't frozen the API 
officially but why not ease the blow if we can?