Renaming KisPainter::bitBltFixedSelection to bitBltWithFixedMask

[JL VT]

On Thu, Sep 9, 2010 at 7:56 AM, LukasT.dev at gmail.com
<lukast.dev at 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.