New feature for review

[LukasT.dev@gmail.com]

On Monday 02 November 2009 13:44:07 Cyrille Berger wrote:
> On Monday 02 November 2009, LukasT.dev at gmail.com wrote:
> > On Monday 02 November 2009 09:22:52 Cyrille Berger wrote:
> > > On Sunday 01 November 2009, LukasT.dev at gmail.com wrote:
> > > > Basically I want to be able to change the size of the brush quickly.
> > > > So I implemented the wishlist [2] a bit and now we have two
> > > > possibilities. The old patch is not applicable at the trunk anymore
> > > > :/ I hoped you to review little faster ;( .
> > > >
> > > > Here is the new patch [1] and now when you press Shift and drag with
> > > > mouse left/right the spray brush changes it's size. Change the cursor
> > > > mode to _brush outline_ to see changes.
> > >
> > > About the patch itself, I think it would be better done using
> > > KisPaintOpSettings::mousePressEvent (and probably adding the related
> > > mouseReleaseEvent/mouseMoveEvent).
> >
> > Why?
> >
> > Boud said he wanted something more abstract. So I decided not to bring
> >  those events to paintop and I created the method. This way e.g. wheel
> > can be configured to change brush size. Although this is not good example
> > as it is obsolete now.
> >
> > I think the code should be in freehand as it is more logical for me. It's
> > Freehand business how it manages the events. My feeling of good design...
> 
> The problem is that in your patch nothing is abstracted. It introduces a
> "changeBrushSize", and if I remember well the design of the paint op, we
> wanted to hide the idea of "brush", and I think your patch go one step
>  back. You are right that it gives some flexibility to freehand tool on
>  what input affect the brush size, yet the freehand tool is not supposed to
>  know about brush. But then if we have other on canvas setting we want to
>  make we would have to extend KisPaintOpSettings to support more of those
>  settings (actually we already have a similar setting that allow to set the
>  offset of the duplicate op).

So let's rename it to changePaintopSize and you hide the idea of the "brush" 
:)

Freehand now controls the brush outline. You can't turn on and off the outline 
in the paintop settings. That's another reason I would put it in the freehand 
tool.

> So on one hand we might not want to have the KisPaintOpSettings to know
>  about events (this would go in the direction of separating UI of
>  painting), and we probably do not want to bloat the KisPaintOpSettings
>  with settings that are specific to a set of paintops.

The biggest problem I see is the conversion of the various measures like 
pixels in documents vs pixels in widget + zoom + points + another 
complication. If this is in KisPaintOpSettings , it drags flake in and that's 
something boud was trying to avoid (there is bug report about it)

My feature plan for 2.2 also contains brush outlines.
I would love to see paintops to work just in document measures and the tool 
should handle the conversions. Current brush outline code breaks this. Maybe 
we should talk this in Oslo and design something more abstract.

> So maybe what we need is a new class "Editor", that would be triggered by
> paint tools when the user press shift, that would countains a set of
>  hanles, be able to draw an overlay (not so good for UI seperation though),
>  and list actions (that can be assigned to keyboard).
> With the handles you could change the size of the brush, set the
> position/offset for the duplicate op. Overlay would fix the visualisation
>  issue for when the user do not use the outline. And finaly action could
>  countains stuff like "increase brush size" that could be mapped to a
>  shortcut.
> 
> Just an idea :)
> 

At least I would like to see this patch in 2.2 if there is nobody working on 
something better. But we also want to stabilize our API so it can contradict.

boud, your opinion?