On Thu, Sep 9, 2010 at 7:56 AM, LukasT.dev@gmail.com wrote: > On Thursday 09 September 2010 10:10:13 JL VT wrote: >> There is a function in the test kis_painter_test.cpp, >> KisPainterTest::testSelectionBitBltFixedSelection(), that tests >> whether an arbitrarily bounded source QRect turns into the expected >> destination QRect when blitting. However this test makes no sense >> anymore, since when using the fixed selection as a mask for a brush >> dab, the most sensible way to use it is to have a mask of the same >> size than the QRect to be extracted from the dab, anything else leads >> to either unpredictable or unclear behavior, and it goes against code >> readability. So I gladly followed Dmitry's suggestion of putting an >> ASSERT to check whether someone tries to use bitBltFixedSelection in >> this unwieldy way (a source QRect of different size than the mask). >> This very ASSERT immediately broke the now clearly obsolete unit test. > > There is one problem. The user specify sx, sy and current status of the code > expect that sx and sy are 0,0. I'm not sure if we can leave it this way. > Either fix that for behaviour where sx is in 0 <= sx, sy <= sw,sh or do > something like don't allow to specify them? Good point, I agree that the current behavior is not good. I will either eliminate sx and sy as arguments, or change the ASSERT to assume less. In either case I will explain my rationale for the change. >> Therefore, the changes I propose are: >> 1.- rename bitBltFixedSelection to bitBltWithFixedMask to reflect what >> the function does now (which was in fact what people were using it >> for). > > Mask? bitBltWithFixedSelection? I prefer Mask because it better describes how the function is used in SmudgeOp, DuplicateOp and HatchingOp (they were the only paintops using it last time I checked). First an ugly square full of something is created (A copy of the canvas in SmudgeOp and DuplicateOp, a hatched square in HatchingOp), and then the mask provided by cachedDab() is turned into an alpha8 colorspace transparency mask and applied over the ugly rectangle to make it look like a normal brush. The Mask is a KisFixedPaintDevice. So it makes sense to call it bitBltWithFixedMask. Another option is calling it bitBltWithTransparencyMask, that sounds even more descriptive =) >> 2.- eliminate the obsolete unit test ( >> KisPainterTest::testSelectionBitBltFixedSelection() ). > > I would propose you to write some simple test that test the behaviour so that > we can catch regressions. Makes sense, I'll make sure to create a test to replace the old one. _______________________________________________ kimageshop mailing list kimageshop@kde.org https://mail.kde.org/mailman/listinfo/kimageshop