Need help on couple of patches

[Igor Tarasov]

Hello!

I've written 2 patches, and I need advice on both of them.

First one is here:
http://www.winehq.org/pipermail/wine-patches/2009-February/069463.html

It's a single-line patch that adds forwarding notification that was
omitted in wine code. This fix looks pretty simple, but it included
lots of reading/testing/debugging, and here is what I came with. And
as I told there it fixes one wine bugzilla bug.

But as I see, it was not comitted, so I am asking for advice: what
could I do in order to make it go through?

Another patch is here: http://bugs.winehq.org/show_bug.cgi?id=12701

The patch adresses 3 problems that all together fix that bug. My
question there was if I should send it in one patch or in several. But
it seems that no one have noticed that post.

Any help and advice is appreciated. Thanks.

[Jérôme Gardou]

Igor Tarasov a écrit :
> Hello!
>
> I've written 2 patches, and I need advice on both of them.
>
> First one is here:
> http://www.winehq.org/pipermail/wine-patches/2009-February/069463.html
>
> It's a single-line patch that adds forwarding notification that was
> omitted in wine code. This fix looks pretty simple, but it included
> lots of reading/testing/debugging, and here is what I came with. And
> as I told there it fixes one wine bugzilla bug.
>
> But as I see, it was not comitted, so I am asking for advice: what
> could I do in order to make it go through?
>
> Another patch is here: http://bugs.winehq.org/show_bug.cgi?id=12701
>
> The patch adresses 3 problems that all together fix that bug. My
> question there was if I should send it in one patch or in several. But
> it seems that no one have noticed that post.
>
> Any help and advice is appreciated. Thanks.

I can't answer your first question, but for the second one : wine 
developers policy is "one patch per fix". So you should send three patches.

[Juan Lang]

Hi Igor,
> It's a single-line patch that adds forwarding notification that was
> omitted in wine code. This fix looks pretty simple, but it included
> lots of reading/testing/debugging, and here is what I came with. And
> as I told there it fixes one wine bugzilla bug.

A regression test would go a long way toward proving your fix is correct.
> The patch adresses 3 problems that all together fix that bug. My
> question there was if I should send it in one patch or in several. But
> it seems that no one have noticed that post.

Several patches would definitely improve this one's chance of success.

Getting your first patch committed is usually the toughest.  Good
luck, and thanks for helping Wine,
--Juan

[Igor Tarasov]

Hi Juan,
Thanks for quick reply.
> A regression test would go a long way toward proving your fix is correct.

There are two problems:

1. I am not that good in C and I have never written any regression tests in C.

2. I just cant think of any regression test for that patch. It should
test if the notification is really being forwarded?

And does that mean that tests for the second patch are also required?

P.S:
> Getting your first patch committed is usually the toughest.  Good
> luck, and thanks for helping Wine,

That's not my first patch ;) I just not too sure of what should I do.

[Juan Lang]

Hi Igor,
> I just cant think of any regression test for that patch. It should
> test if the notification is really being forwarded?

Yes, that's what I mean.  You're right, that might be hard, since you
have to simulate a mouse click.
> And does that mean that tests for the second patch are also required?

Required?  Not necessarily, but they always help.  Obviously correct
patches tend to get applied even without regression tests, but even
they would benefit from them.
--Juan

[Igor Tarasov]

> > I just cant think of any regression test for that patch. It should
> > test if the notification is really being forwarded?
>
> Yes, that's what I mean.  You're right, that might be hard, since you
> have to simulate a mouse click.

Now I don't get it. Looked through listview tests and there are not
even one mouse-related test. And my patch could be tested by just
sending HDN_ITEMCLICKW to listview control. And capturing the response
notifications.

But the code is:

static LRESULT LISTVIEW_HeaderNotification(LISTVIEW_INFO *infoPtr,
const NMHEADERW *lpnmh)
{
    ...
    switch (lpnmh->hdr.code)
    {
        ...
        case HDN_ITEMCLICKW:
        case HDN_ITEMCLICKA:
        {
            ...
            notify_forward_header(infoPtr, lpnmh);
        }
        break;

Is that really should be tested for being executed, by simulating
mouse click? Or I just don't understand something?
> > And does that mean that tests for the second patch are also required?
>
> Required?  Not necessarily, but they always help.  Obviously correct
> patches tend to get applied even without regression tests, but even
> they would benefit from them.

Okay, gonna try.

[Dmitry Timoshkov]

"Igor Tarasov"  wrote:
> Now I don't get it. Looked through listview tests and there are not
> even one mouse-related test. And my patch could be tested by just
> sending HDN_ITEMCLICKW to listview control. And capturing the response
> notifications.

Then just do it that way.
> But the code is:
>
> static LRESULT LISTVIEW_HeaderNotification(LISTVIEW_INFO *infoPtr,
> const NMHEADERW *lpnmh)
> {
>    ...
>    switch (lpnmh->hdr.code)
>    {
>        ...
>        case HDN_ITEMCLICKW:
>        case HDN_ITEMCLICKA:
>        {
>            ...
>            notify_forward_header(infoPtr, lpnmh);
>        }
>        break;
>
> Is that really should be tested for being executed, by simulating
> mouse click? Or I just don't understand something?

It's up to you how to execute the requested functionality.
If that's absolutely necessary it's not too hard to send
WM_LBUTTONDOWN/WM_LBUTTONUP to the control.

[James McKenzie]

Dmitry Timoshkov wrote:
> "Igor Tarasov"  wrote:
> > Now I don't get it. Looked through listview tests and there are not
> > even one mouse-related test. And my patch could be tested by just
> > sending HDN_ITEMCLICKW to listview control. And capturing the response
> > notifications.
>
> Then just do it that way.

Igor:

The key is to see what happens and if it is what you expected.  Can this
also be emulated in any version of Windows to verify this is what
happens there?

James McKenzie

[Igor Tarasov]

> The key is to see what happens and if it is what you expected.  Can this
> also be emulated in any version of Windows to verify this is what
> happens there?

I hope that clicking listview header and then bumping message to the
application happens in any Windows version. I hope.

Anyway, I do understand WHAT I have to do pretty well. What I do not
understand well is HOW I do that. You know, you may understand the
language, but thats not enough to speak in it. Especially not patching
something, but writing something from scratch (as with test).

I've stated many times that I am not C programmer (though programming
is my job, but I do web stuff). And the only reason why I started
patching wine is that several very important for me bugs were not
fixed for quite long, so I started digging into it.

But I've looked through listview tests and it seems to be quite simple
(much simplier than WinAPI and C appeared to me at the very start :)
), so I hope to figure it out.

[Juan Lang]

> But I've looked through listview tests and it seems to be quite simple
> (much simplier than WinAPI and C appeared to me at the very start :)
> ), so I hope to figure it out.

If you get stuck, send what you have and ask for help.  We're trying
to encourage you to keep working on it, so your patch gets accepted :)
--Juan

[Igor Tarasov]

> It's up to you how to execute the requested functionality.
> If that's absolutely necessary it's not too hard to send
> WM_LBUTTONDOWN/WM_LBUTTONUP to the control.

...for experienced C programmer. And I am not one. But I'll try.