Re: There is no need to offset the source rectangle in the reverse direction before scrolling

[Rein Klazes]

On Tue, 13 Dec 2005 23:53:07 +0800, you wrote:
> Changelog:
>    Dmitry Timoshkov <dmitry(a)codeweavers.com>
>    There is no need to offset the source rectangle in the reverse
>    direction before scrolling.
>
> --- cvs/hq/wine/dlls/x11drv/scroll.c	2005-11-22 12:41:07.000000000 +0800
> +++ wine/dlls/x11drv/scroll.c	2005-12-13 23:42:15.000000000 +0800
> /* Then clip again to get the source rectangle that will remain in the
>      * clipping rect */
>     rcSrc = rcClip;
> -    OffsetRect( &rcSrc, -dx, -dy);
>     IntersectRect( &rcSrc, &rcSrc, &rcClip);

In the first place: with this change the IntersectRect call is a no-op 

I doubt  you understand the reason for "to offset the source rectangle
in the reverse direction":

Scrolling should only occur in the clipping rectangle. When you talk
about the *destination* pixels, a clipping with the cliprect is needed.
Here we are talking about the *source* pixels, from destination to
source is indeed reverse. *that* is what is happening here. 

If you are not convinced: the change is wrong and clipping fails. I have
added a test to confirm that. The test also gives a nice visual idea
what goes wrong, just add a Sleep() or two to get the picture.

Changelog:
dlls/x11drv	: scroll.c
dlls/user/tests	: win.c
Restore the previous change to X11DRV_ScrollDC. With a regression test
to confirm it was wrong.

Rein.

There is no need to offset the source rectangle in the reversedirection before scrolling

[Dmitry Timoshkov]

"Rein Klazes" <wijn(a)wanadoo.nl> wrote:
> > /* Then clip again to get the source rectangle that will remain in the
> >      * clipping rect */
> >     rcSrc = rcClip;
> > -    OffsetRect( &rcSrc, -dx, -dy);
> >     IntersectRect( &rcSrc, &rcSrc, &rcClip);
>
> In the first place: with this change the IntersectRect call is a no-op
>
> I doubt  you understand the reason for "to offset the source rectangle
> in the reverse direction":
>
> Scrolling should only occur in the clipping rectangle. When you talk
> about the *destination* pixels, a clipping with the cliprect is needed.
> Here we are talking about the *source* pixels, from destination to
> source is indeed reverse. *that* is what is happening here.

Well, the committed patch 1) does fix a visual glitch in the app I'm working on
and 2) passes all the current tests.

There are a couple of problems with your tests and fixes: it tests
child windows with CS_PARENTDC style set, and it doesn't test the case
when clipping rect is NULL. In my case both cases are not fulfilled and
therefore we see different results.

P.S.
I think that brackets with multiple if/else statements are mandatory to make
it more clear what is going on.

-- 
Dmitry.

There is no need to offset the source rectangle in the reversedirection before scrolling

[Alexandre Julliard]

"Dmitry Timoshkov" <dmitry(a)baikal.ru> writes:
> Well, the committed patch 1) does fix a visual glitch in the app I'm working on
> and 2) passes all the current tests.

The current tests pass with or without your patch, so it doesn't prove
anything. What you need is to come up with a test that replicates what
the app is doing and that fails without your patch.

-- 
Alexandre Julliard
julliard(a)winehq.org

There is no need to offset the source rectangle in the reversedirection before scrolling

[Rein Klazes]

On Wed, 14 Dec 2005 22:42:28 +0800, you wrote:
> Well, the committed patch 1) does fix a visual glitch in the app I'm working on
> and 2) passes all the current tests.

1) You know that doesn't make it right. I see it causing visual glitches
in *three* programs here, girotel,  native hh.exe and wine's built-in
notepad, which does not make it wrong either
2) Indeed, you managed to break functionality that never broke before.
> There are a couple of problems with your tests and fixes

It shows a problem. If you could create a test, or otherwise point to a
way to reproduce your problem I would not be arguing but looking for a
way to fix them all.
> : it tests
> child windows with CS_PARENTDC style set, and it doesn't test the case
> when clipping rect is NULL. In my case both cases are not fulfilled and
> therefore we see different results.

I still feel you do not understand what you 'fixed':

- clipping is plain MSDN specified functionality. Any undocumented
restrictions of the dc to a CS_PARENTDC inhered one is unlikely;
- if the *special* case cliprect==NULL is not handled correctly, then
the correct fix is unlikely to be not to clip in *all* cases.

Rein.