[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-kimageshop
Subject: Activation of the node after doing some actions with the image
From: Dmitry Kazakov <dimula73 () gmail ! com>
Date: 2012-01-26 13:05:27
Message-ID: CAEkBSfUL178MVfjnyK5SS6oOBbPJGASTBZ136W6vQk5JcX3dPQ () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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
[Attachment #5 (text/html)]
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>
_______________________________________________
kimageshop mailing list
kimageshop@kde.org
https://mail.kde.org/mailman/listinfo/kimageshop
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic