paintopsettings refactoring

[Sven Langkamp]

On Fri, Jan 15, 2010 at 11:43 PM, LukasT.dev at gmail.com <lukast.dev at gmail.com
> wrote:

> On Monday 30 November 2009 08:10:11 Boudewijn Rempt wrote:
> > We had a discussion this Sunday on what to do with the problem that
> paintop
> > settings and their widgets are so very closely tied together. We came to
> >  the following redesign (which I'll also put on the wiki, but I cannot
> >  reach that at the moment):
>
> Is that on the wiki? I could not find it.
>
> > We had an idea toremove the KisPaintOpSettings class and put the paintop
> > options and custom options directly inside the paintop. But I'm not sure
> >  how that works if we look at the flow when selecting a preset and
> starting
> >  to paint:
>
> So we will drop KisPaintOpSettings?
>

I got a bit away from dropping them at the moment. For now the
paintIncremental and paintOutline functionality is still in the settings.
The erase paintop doesn't have both and the setting class isn't much more
than a dummy in case some paintOutline gets added.


> So far I port the paintop like this
>
> I have:
>
> o KisPaintOpOption (e.g. KisSprayShapeOption)
>
> contains GUI elements usually in UI file  + signals in code,
> sometimes some logic like prescaling QImage for the spray paintop according
> particle size
>
> every GUI element is connected to sigSettingChanged() so that the settings
> are
> updated and it also triggers update of the preview of the brush
>
> if you forget to call the signal, the paintop does not know about the
> change
> in GUI because now paintops does not access widgets but settings
>
> o KisPaintOpSettings
>
> contains paintOutlineRect and paintOutline, basiclly it draws the outline
> of
> the brush (paintOutline) and returns the area to be restored on the canvas
> from the previous position of the brush outline (paintOutlineRect)
>
> it used to have also changePaintOpSize method which is called by Shift-drag
> on-canvas size changing but that is moved now to KisPaintOpOptionsWidget
>

I moved changePaintOpSize to the widget because it changed the widgets and
that way it could be easier to share it between the paintops.
All brush based paintops could use the same option widget and could use the
same resize code (still todo)

For paintOutline that would be possible too so that all brush based paintops
would use the same outline drawing code, but that would drag in some
dependency at the moment. After your planned QPainterPath refactoring we
could look at it again.


> Every GUI element value is accessed through code like this
> getInt("Spray/diameter")
>
> My paintops have access just to this KisPaintOpSettings object.
>
> Questions:
> 1. Paintops will have access to the KisPaintOpOption? What is the design
> idea?
>

Originally my plan was to remove the remove that access. For practical
reasons there is still a dependency for the use in paintOutline and
changePaintOpSize, as it's the best way to access the widgets from the tool.
It wouldn't be needed in paintOutline if it reads the setting, but it turned
that it too slow in case you change the paintop size on canvas so it's still
reading from the widget directly. For the properties itself the widgets
shouldn't be used.


> Problems:
> * It is very frustrating job to port my paintops as I have many options in
> GUI. It is easy to make mistake because of the code like
> getInt("Spray/diaemter") compiles fine, but it has mistake (it suppose to
> be
> diameter). Is there any nice way to fprint the KisPaintOpSettings?
>

I know it's a bit frustrating with all those settings, but for the presets
to work everythings must be serializable.
The settings have a method dump() that outputs all properties. To avoid
mistakes you can either define some strings and use these instead of
hardcoding the strings or write a unittest that check the roundtrip of all
properties.


> * I wanted to return QImage in the settings object because spray uses
> QImage.But there isn't method getQImage("Spray/imageBrush") or something
> like
> this.
>

You can still put the QImage as a member in the sprayop settings.
Everythings that isn't a property won't be saved automatically, so you have
to do the saving code yourself. For a QImage the filename of the source file
could be added as property additionally to the QImage.

Another possibility would be to use a pattern from the pattern server
instead of QImage. That way you only have to store the id of the pattern and
the paintop could fetch it from the server.

There is also the possibility that the image can't be loaded from disk when
the preset is loaded, because it might be removed or something like that.
For that I added a method canPaint() to KisPaintOp that can check if the
settings are read correctly and block painting in case e.g. the image is
missing.


> I looked at the KisBrushOp code and it has access to class KisBrushOption.
> I
> will do that somehow similar, is that ok? PaintOp will have access to
> Options?
>

It's ok for use in paintOutline for now. In almost all other cases it
shouldn't be used.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kimageshop/attachments/20100116/d59949a8/attachment.htm