Hello, all!<br><br>I wanted to discuss our node activation strategy a bit, because it creates some problems now.<br><br>Problem:<br>Some of our tools/actions use direct access to the view to activate a newly created node. Examples are listed below in [1].<br>
<br>Why this is a problem:<br>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.<br>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.<br>
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]<br>
<br>Possible solutions:<br><br>1) [not a good one]<br>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. <br>
<br>Pros: are there any?<br>Cons:<br>-- image indirectly depends on the view, that is quite bad<br>-- [more serious] quite complicated to implement<br>-- [even more serious] you cannot get currently activated node from the view being run in the context of the scheduler's thread.<br>
<br>2) [better one]<br>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:<br>
<br>* when a node is added it is automatically activated [we do not have it currently]<br>* when a node is removed some neighboring node is activated instead [we do already have it]<br>* when a node is moved active node doesn't change<br>
* when a root layer is changed (sigLayersChanged), the topmost node is activated [we have something like this hardcoded in KisView2::slotLoadingFinished].<br><br>Pros:<br>+ tools and actions should not think about activation of anything<br>
+ no inter-module dependencies<br>+ no multithreading problems<br>+ undo/redo work as expected<br>Cons:<br>-- we should consider all the usecases before declaring it (see questions below)<br><br><br>Conclusion and questions:<br>
<br>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:<br><br>1) Do all our tools/actions fit into this way of working? I mean, do all of them activate newly created layers only? <br>
2) Are there any usecases when a tool should activate some other layer, not a new one?<br><br><br>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?<br>
<br><br><br>[1] -- List of references to activateNode():<br><br>./ui/kis_view2.cpp:474:            nodeManager()->activateNode(node);<br>./ui/kis_view2.cpp:674:        m_d->nodeManager->activateNode(activeNode);<br>
./ui/flake/kis_shape_controller.cpp:174:            canvas->view()->nodeManager()->activateNode(shapeLayer);<br>./ui/kis_selection_manager.cc:450:        m_view->nodeManager()->activateNode(layer);<br>./ui/kis_import_catcher.cc:106:            m_d->view->nodeManager()->activateNode(importedImageLayer.data());<br>
./ui/kis_layer_manager.cc:300:            m_view->nodeManager()->activateNode(layer);<br>./ui/kis_layer_manager.cc:316:            m_view->nodeManager()->activateNode(layer);<br>./ui/kis_layer_manager.cc:357:            m_view->nodeManager()->activateNode(layer);<br>
./ui/kis_layer_manager.cc:392:            m_view->nodeManager()->activateNode(layer);<br>./ui/kis_layer_manager.cc:432:        m_view->nodeManager()->activateNode(adjl);<br>./ui/kis_layer_manager.cc:484:    m_view->nodeManager()->activateNode(l);<br>
./ui/kis_layer_manager.cc:532:        m_view->nodeManager()->activateNode(dup);<br>./ui/kis_layer_manager.cc:749:        m_view->nodeManager()->activateNode(newLayer);<br>./plugins/tools/defaulttools/kis_tool_move.cc:63:        //m_view->nodeManager()->activateNode(m_node);<br>
./plugins/extensions/dockers/defaultdockers/kis_layer_box.cpp:343:        m_nodeManager->activateNode(node);   // NOTE: temporary bug fix - Pentalis<br clear="all"><br>[2] - <a href="https://bugs.kde.org/show_bug.cgi?id=290708">https://bugs.kde.org/show_bug.cgi?id=290708</a><br>
<br><br>-- <br>Dmitry Kazakov<br>