Review request: Masks support compositeOps and opacity

[Boudewijn Rempt]

On Friday 23 October 2009, Dmitry Kazakov wrote:
> Hi, all!
> 
> Sorry for not using reviewboard, it doesn't accept git patches and svn
> creates quite messy patches when a file is moved to a different location =)
> 
> Well, here is a patch that allows masks to use different compositeOps and
> opacity. Now a user can choose how a mask will be applied to the layer. It
> makes sense in some circumstances. You can test it with a test file in
> attachment.
> 
> Tetscase:
> 1) If you set mask's composition to "normal", then a selection will be
> darker than a background.
> 2) If you set it to "Alpha darken", then it will dissolve in a background.
> 
> To achieve this i moved colorspace, composition and opacity stuff from
> KisLayer to KisBaseNode, therefore masks can use it too.
> 
> Have i forgotten to move something connected to colorspaces alongside?
> 
> Waiting for your critique! =)
> 

Meh, I see your git-svn version has the same problem as mine: it destroys the 
history of renamed files. 	

-----------

Note: don't replace things like

if(op) return op;
return parent->colorSpace()->compositeOp(COMPOSITE_OVER) 

with

return op ? op : parentNode->colorSpace()->compositeOp(COMPOSITE_OVER);

Whether the one or the other is more readable is purely personal preference, 
and for the compiler it doesn't matter, so respect the decision of the 
original author.

---------

Same for:

if (!parent) {
	return 0;
}

Don't replace that with

if (!parent) return 0;

In principle, I want braces used _everywhere_ because we've had more than 
enough problems with the short form causing bugs when people add (debug) code. 
Just respect what has been written, and don't rewrite it.

----------

About KisBaseNode not knowing about the parent: it has been a conscious 
desicion to separate the the graph part from the base bart. So, if 
setCompositeOpn needs to know about the parent, there is no need for 
duplication, move that code to KisNode from KisLayer and KisMask.

We could also decide that all nodes must have a composite op set, 
COMPOSITE_OVER by default, so they don't go back to their parent anymore.
That might be even simpler.
 
-----------

This is just from eyeballing the source: I haven't been able to run it yet, 
because I get a compile error:

[ 48%] Building CXX object 
krita/image/tests/CMakeFiles/KisBaseNodeTest.dir/kis_base_node_test.o
/home/boud/kde/src/koffice/krita/image/tests/kis_base_node_test.cpp: In member 
function ‘void KisBaseNodeTest::testCreation()’:
/home/boud/kde/src/koffice/krita/image/tests/kis_base_node_test.cpp:42: error: 
cannot allocate an object of abstract type ‘TestNode’
/home/boud/kde/src/koffice/krita/image/tests/kis_base_node_test.cpp:30: note:   
because the following virtual functions are pure within ‘TestNode’:
/home/boud/kde/src/koffice/krita/image/kis_base_node.h:90: note:        
virtual const KoColorSpace* KisBaseNode::colorSpace() const
/home/boud/kde/src/koffice/krita/image/kis_base_node.h:125: note:       
virtual const KoCompositeOp* KisBaseNode::compositeOp() const
/home/boud/kde/src/koffice/krita/image/tests/kis_base_node_test.cpp: In member 
function ‘void KisBaseNodeTest::testContract()’:
/home/boud/kde/src/koffice/krita/image/tests/kis_base_node_test.cpp:54: error: 
cannot allocate an object of abstract type ‘TestNode’
/home/boud/kde/src/koffice/krita/image/tests/kis_base_node_test.cpp:30: note:   
since type ‘TestNode’ has pure virtual functions
/home/boud/kde/src/koffice/krita/image/tests/kis_base_node_test.cpp: In member 
function ‘void KisBaseNodeTest::testProperties()’:
/home/boud/kde/src/koffice/krita/image/tests/kis_base_node_test.cpp:83: error: 
cannot allocate an object of abstract type ‘TestNode’
/home/boud/kde/src/koffice/krita/image/tests/kis_base_node_test.cpp:30: note:   
since type ‘TestNode’ has pure virtual functions
make[2]: *** 
[krita/image/tests/CMakeFiles/KisBaseNodeTest.dir/kis_base_node_test.o] Error 
1
make[1]: *** [krita/image/tests/CMakeFiles/KisBaseNodeTest.dir/all] Error 2
make: *** [all] Error 2


-- 
Boudewijn Rempt | http://www.valdyas.org