Activation of the node after doing some actions with the image

[Dmitry Kazakov]

Hello, all!

I wanted to discuss our node activation strategy a bit, because it creates
some problems now.

Problem:
Some of our tools/actions use direct access to the view to activate a newly
created node. Examples are listed below in [1].

Why this is a problem:
1) It doesn't handle undo/redo. Steps to reproduce: create new layer, undo,
redo -- you will get the lower layer activated, though it is expected that
the new one will be active.
2) [more serious] After making some of the actions asynchronous you cannot
get a pointer to the newly created node, because it'll be created later in
another thread.
3) [even more serious] We can workaround the absence of the pointer to the
new node by wrapping activation in a command, *but* these commands are
executed in the context of the scheduler thread, so accessing the UI
threads will crash Krita. I tried to do this way in kis_tool_move.cc:63 and
it proved it crashes ;). [2]

Possible solutions:

1) [not a good one]
We wrap the activation into a command and declare that the tools and
actions should add this command to the undo stack every time they want to
activate a node. To workaround the crash we do the activation in a
complicated way: we introduce a signal to the image (e.g.
KisImage::sigRequestActivateNode) and it will be transferred to the view by
a queued connection.

Pros: are there any?
Cons:
-- image indirectly depends on the view, that is quite bad
-- [more serious] quite complicated to implement
-- [even more serious] you cannot get currently activated node from the
view being run in the context of the scheduler's thread.

2) [better one]
We may do like Qt's QAbstractItemModel is supposed to work. We do not add
any commands and declare that the tools should not not activate any nodes
directly. All the work is done in the KisNodeManager (or KisNodeModel)
automatically. It means that:

* when a node is added it is automatically activated [we do not have it
currently]
* when a node is removed some neighboring node is activated instead [we do
already have it]
* when a node is moved active node doesn't change
* when a root layer is changed (sigLayersChanged), the topmost node is
activated [we have something like this hardcoded in
KisView2::slotLoadingFinished].

Pros:
+ tools and actions should not think about activation of anything
+ no inter-module dependencies
+ no multithreading problems
+ undo/redo work as expected
Cons:
-- we should consider all the usecases before declaring it (see questions
below)


Conclusion and questions:

So I think the second approach is really better and easier to implement. We
need to fix KisNodeManager (or KisNodeModel) and remove all the direct
calls from the actions. But the questions are:

1) Do all our tools/actions fit into this way of working? I mean, do all of
them activate newly created layers only?
2) Are there any usecases when a tool should activate some other layer, not
a new one?


So what do you think about this problem? Probably, there are some usecases
which I haven't thought about? Or maybe there are some problems possible?



[1] -- List of references to activateNode():

./ui/kis_view2.cpp:474:            nodeManager()->activateNode(node);
./ui/kis_view2.cpp:674:        m_d->nodeManager->activateNode(activeNode);
./ui/flake/kis_shape_controller.cpp:174:
canvas->view()->nodeManager()->activateNode(shapeLayer);
./ui/kis_selection_manager.cc:450:
m_view->nodeManager()->activateNode(layer);
./ui/kis_import_catcher.cc:106:
m_d->view->nodeManager()->activateNode(importedImageLayer.data());
./ui/kis_layer_manager.cc:300:
m_view->nodeManager()->activateNode(layer);
./ui/kis_layer_manager.cc:316:
m_view->nodeManager()->activateNode(layer);
./ui/kis_layer_manager.cc:357:
m_view->nodeManager()->activateNode(layer);
./ui/kis_layer_manager.cc:392:
m_view->nodeManager()->activateNode(layer);
./ui/kis_layer_manager.cc:432:
m_view->nodeManager()->activateNode(adjl);
./ui/kis_layer_manager.cc:484:    m_view->nodeManager()->activateNode(l);
./ui/kis_layer_manager.cc:532:
m_view->nodeManager()->activateNode(dup);
./ui/kis_layer_manager.cc:749:
m_view->nodeManager()->activateNode(newLayer);
./plugins/tools/defaulttools/kis_tool_move.cc:63:
//m_view->nodeManager()->activateNode(m_node);
./plugins/extensions/dockers/defaultdockers/kis_layer_box.cpp:343:
m_nodeManager->activateNode(node);   // NOTE: temporary bug fix - Pentalis

[2] - https://bugs.kde.org/show_bug.cgi?id=290708


-- 
Dmitry Kazakov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kimageshop/attachments/20120126/e04d83bd/attachment.html>