Switching shapes and tools: is it broken?

[Jan Hambrecht]

On 21.06.2011 10:47, Dag Andersen wrote:
> Mandag 20 juni 2011 22:37:10 skrev jaham at gmx.net:
>> On Monday 20 June 2011 15:31:28 Dag Andersen wrote:
>>> Mandag 20 juni 2011 15:20:27 skrev du:
>>>> On 20.06.2011 13:02, Dag Andersen wrote:
>>>>> Mandag 20 juni 2011 11:37:13 skrev du:
>>>>>> On 20.06.2011 10:34, Dag Andersen wrote:
>>>>>>> Before the weekend I fixed a crash in ArtisticTextShape and, as Jan
>>>>>>> Hambrect pointed out, I unvittingly removed the functionallity to
>>>>>>> go direct between shapes. So I thought I'd find out how this
>>>>>>> *should* be implemented before I tried to put it back, but I
>>>>>>> can't!
>>>>>>> I can find code that afaics is supposed to do it, but it is never
>>>>>>> executed. It may be that it is just me being confused so I need to
>>>>>>> run this by somebody that is more familiar with the code than I am.
>>>>>>>
>>>>>>> There is two "types" of switches that can occure:
>>>>>>> 1) Switch between shapes with different tools.
>>>>>>> E.g. TextTool ->    ArtisticTextTool.
>>>>>>> 2) Switch between shapes using the same tool.
>>>>>>> E.g. TextTool ->    TextTool
>>>>>>
>>>>>> 3. Switch between shapes using a layer docker like in Karbon or
>>>>>> Stage.
>>>>>
>>>>> Hmm, how is this different from 2?
>>>>
>>>> The tool itself is not involved in changing the shape selection.
>>>>
>>>>>> This was the use case targeted in the artistic text tool. So the
>>>>>> user could just click on another artistic text shape from the layer
>>>>>> docker and have the tool use that for further editing.
>>>>>
>>>>> I opened karbon, added an artistic text shape, added a new layer and
>>>>> added a new artistic text shape to that layer. Is this the case you
>>>>> are thinking of?
>>>>
>>>> No, rather you have two artistic text shapes. While editing the first
>>>> one with the artistic text tool, you click on the second one inside the
>>>> layer docker. The clicked shape gets selected, the first shape gets
>>>> deselected. The tool gets a signal that the shape selection changed and
>>>> checks if the artistic text shape it is currently editing is still
>>>> selected. It notices that it is not selected anymore so it checks if
>>>> any other artistic text shape is selected. Yes there is one, so the
>>>> tool changes its internal state to editing of that newly selected
>>>> shape.
>>>
>>> Yes, just found out what you meant.
>>> I still maintain that the shape should not need to listen to the seletion
>>> changed signal, all switching should be handled by KoToolManager via
>>> activate()/deactivate().
>>> If this is left to the shapes *all* shapes have to implement the same
>>> code...
>>
>> I am not sure that switching tools on each selection change is the right
>> thing to do. That would mean that you are losing the tool state each time
>> the selection changes.
> Well, the tool is only deactivated, not deleted so you can certainly save any
> states.
> But: I think I would expect to get back to the state I was in last time I
> edited a certain *shape*, so imo any states should be saved to the shape.

This is a no-go. Tool state should never be written to a shape. These 
are separate kind of data and should not be mixed.

> Also, in your case, think of this: You have 1 text shape (ts) and 2 artistic
> text shapes (as).
> Now navigating as->as will give different result than as->ts->as.
> As a user I think I'd be confused.

The different result will occur in either case, because activating ts 
would automatically deactivate the artistic text tool and activate the 
default tool. Whereas selecting as2 would make the tool change its state 
but keep it activated (current behaviour) or deactivate the artistict 
text tool and activate it again (your solution).
So yes selecting different shape will cause different results.

>
> I got a bit deeper into the code, and I realized that the primary criteria for
> tool activation is which shapes are selected. Hence, any tool activation
> should be triggered by selectionChanged(). Secondary criteria is if a tool is
> active and if it is the interaction tool or not.
>
> So I put this logic into KoToolManager::selectionChanged(), removed switching
> from currentLayerChanged() and KoToolProxy, and suddenly things started to
> work pretty well.
> I certainly think this is the way to go. If you think you need to be able to
> switch shapes while your tool stays active (use case?), I think we need to add
> a swicthShapes method to the tool, and enhance the switching logic in
> KoToolManager::selectionChanged().

I am not too much attached to the current solution. I just want the use 
case working, where selecting a different artistic text shape while the 
artistic text tool is activated should allow the user to start editing 
the newly selected shape right away. No manually activating the artistic 
text tool again.
So if with your solution the artistic text tool gets first deactivated 
and then activated again when a different artistic text shape is 
selected, this would work too. There is only some minor work to do to 
not clear the last tool state when deactivating the tool in case the 
last selected shape stays selected.

Ciao Jan