Re: [1/2] comctl32/tests: Added tests for mouse events handling (try 3)

[Dmitry Timoshkov]

Daniel Jelinski  wrote:
> Skipping tests on WinNT - older versions of comctl send a different
> set of events.

Then you need to figure out what is different, and make the tests pass
there as well (there is 'optional' flag for that), otherwise you risk
breaking applications written for NT.
> - /* WM_RBUTTONDOWN does not return until it gets another mouse event.
> - Make sure it gets one by posting WM_RBUTTONUP to message queue */
> - PostMessageA(hTree, WM_RBUTTONUP, 0, (LPARAM)0x10001);
> - /* this sequence should NOT send WM_CONTEXTMENU */
> - SendMessageA(hTree, WM_RBUTTONDOWN, 2, (LPARAM)0x10001);
> -
> - /* ditch the first sequence as it processed more messages than we need. Redo */
> - flush_sequences(sequences, NUM_MSG_SEQUENCES);
> -
> - PostMessageA(hTree, WM_RBUTTONUP, 0, (LPARAM)0x10001);
> - SendMessageA(hTree, WM_RBUTTONDOWN, 2, (LPARAM)0x10001);

This can't work. If you use PostMessage you need to flush message queue before
testing the sequence. Using SendMessage after PostMessage doesn't guarantee
that a posted message is handled before a sent one, you may try generating
hardware messages instead.
> -
> - if(sequences[PARENT_SEQ_INDEX]->sequence->message == 0x133)
> - {
> -
>    win_skip("Comctl32 versions prior to 5.80 send different set of events");
> -
>    return;
> - }

Please use symbolic names for messages instead of magic numbers.

[1/2] comctl32/tests: Added tests for mouse events handling (try 3)

[Daniel Jelinski]

2012/4/8 Dmitry Timoshkov :
> Daniel Jelinski  wrote:
> > Skipping tests on WinNT - older versions of comctl send a different
> > set of events.
>
> Then you need to figure out what is different, and make the tests pass
> there as well (there is 'optional' flag for that), otherwise you risk
> breaking applications written for NT.

Okay, I'll do that. I thought that since comctl is supposed to be
backwards compatible, I'll write the tests to just pass on the newer
version.
> > - /* WM_RBUTTONDOWN does not return until it gets another mouse event.
> > - Make sure it gets one by posting WM_RBUTTONUP to message queue */
> > - PostMessageA(hTree, WM_RBUTTONUP, 0, (LPARAM)0x10001);
> > - /* this sequence should NOT send WM_CONTEXTMENU */
> > - SendMessageA(hTree, WM_RBUTTONDOWN, 2, (LPARAM)0x10001);
> > -
> > - /* ditch the first sequence as it processed more messages than we need. Redo */
> > - flush_sequences(sequences, NUM_MSG_SEQUENCES);
> > -
> > - PostMessageA(hTree, WM_RBUTTONUP, 0, (LPARAM)0x10001);
> > - SendMessageA(hTree, WM_RBUTTONDOWN, 2, (LPARAM)0x10001);
>
> This can't work. If you use PostMessage you need to flush message queue before
> testing the sequence. Using SendMessage after PostMessage doesn't guarantee
> that a posted message is handled before a sent one, you may try generating
> hardware messages instead.

I want the posted message to be handled after the sent one, and I get
exactly that. The event handler for WM_RBUTTONDOWN captures all
messages until it finds one of the mouse-related events, so I needed
to post WM_RBUTTONUP first to make sure that SendMessage returns.
Hardware messages are an alternative, but I couldn't find any relevant
examples, so I followed the path of least resistance.

The first PostMessage/SendMessage sequence flushes the message queue,
or at least its interesting part. There are probably better ways
(PeekMessage/DispatchMessage loop comes to mind), but this one just
works.
> > -
> > - if(sequences[PARENT_SEQ_INDEX]->sequence->message == 0x133)
> > - {
> > - win_skip("Comctl32 versions prior to 5.80 send different set of events");
> > - return;
> > - }
>
> Please use symbolic names for messages instead of magic numbers.

Ok, can do.

Best regards,
Daniel

[1/2] comctl32/tests: Added tests for mouse events handling (try 3)

[Vitaliy Margolen]

On 04/08/2012 01:44 PM, Daniel Jelinski wrote:
> Hardware messages are an alternative, but I couldn't find any relevant
> examples, so I followed the path of least resistance.

These tests you wrote won't work. Emulating mouse up/down events with 
anything other then hardware messages will not exercise the entire chain. 
For example search for "mouse_event" or SendInput.

Vitaliy.

[1/2] comctl32/tests: Added tests for mouse events handling (try 3)

[Daniel Jeliński]

Dnia 09-04-2012 o 04:13:44 Vitaliy Margolen 
napisał(a):
> On 04/08/2012 01:44 PM, Daniel Jelinski wrote:
> > Hardware messages are an alternative, but I couldn't find any relevant
> > examples, so I followed the path of least resistance.
>
> These tests you wrote won't work. Emulating mouse up/down events with  
> anything other then hardware messages will not exercise the entire  
> chain. For example search for "mouse_event" or SendInput.

Thank you for your feedback. I agree, the tests are not equivalent to real  
mouse input. However they do expose the differences between WinAPI and  
Wine. I wrote them just to make sure (and prove) that my fix to comctl  
implementation does the right thing, and that I do not introduce new bugs  
in place of the old one.

If I remove the changes to tests and post changes to the library only,  
will they get accepted?

Daniel

[1/2] comctl32/tests: Added tests for mouse events handling (try 3)

[Daniel Jelinski]

Hello,
I see that my patch hasn't been accepted yet. Well if some explanation
could help it through, here goes:
Bug 19222 makes using MS SQL Server management studio a pain. The
application displays database structure in a tree view, and most
actions are executed from a context menu displayed by right-clicking
on a relevant position in the tree view. Unfortunately Wine suffers
from a bug - right click displays the menu, but selecting any option
from the menu causes another menu to be displayed at the current mouse
position. Selecting an option from this second menu works as expected.
Also opening the popup menu by pressing context menu key on the
keyboard works properly.
The usual suspect in such cases is that incorrect messages are sent,
or messages are sent in incorrect order. I tried to run the
application with native comctl, but unfortunately it refused to work -
probably comctl version 6.0 is required, while winetricks installs
version 5.80. So I created my own application in Delphi. It dispayed
the same problem with builtin comctl and worked correctly with native.
Then I tested it with WINEDEBUG=+message. The outstanding differences
were:
- on native comctl treeview sent two messages to the main window -
first WM_NOTIFY (NM_RCLICK) and then WM_CONTEXTMENU
- on builtin comctl treeview sent first WM_CONTEXTMENU to self, which
then got propagated to main window, and then WM_NOTIFY(NM_RCLICK).
There was something peculiar - WM_RBUTTONUP never appeared in the
logs. I checked to see why and found a function TREEVIEW_TrackMouse.
This function is called by WM_RBUTTONDOWN handler and it captures all
events from the message queue until it finds either WM_RBUTTONUP or
WM_MOUSEMOVE with coordinates sufficiently far from the origin of the
click. There was also a comment stating that "This is quite unusual
piece of code, but that's how it's implemented in Windows.". Given the
absence of WM_RBUTTONUP in message logs I consider this assertion to
be true.
With this knowledge I proceeded to write tests. I wanted to emulate
right button down/up sequence. I couldn't send both messages by
SendMessage, because SendMessage sends messages directly to the window
procedure, and WM_RBUTTONUP has to be delivered by message queue. I
also didn't want to push both messages to the queue - this would net
me several more irrelevant messages like WM_PAINT and could cause
failures if someone moved the (physical) mouse during the test. This
is why I chose to post WM_RBUTTONUP and then send WM_RBUTTONDOWN.
Later, thanks to our friendly bot Marvin, I found out that older
versions of comctl sent WM_PAINT messages during the button
down/button up sequence, bypassing the message queue. It took me
several tries to find out which messages are sent, but now the patch
works correctly on all tested comctl versions.
Then I corrected Wine to pass the tests. I tested both applications
(Microsoft's and mine) to see if they work correctly when the correct
messages are sent. They do.

If there is anything else I can do to help this patch get accepted,
let me know. I'd like to have a clean git origin before I start fixing
other treeview problems.
Thanks for your patience,
Daniel

[1/2] comctl32/tests: Added tests for mouse events handling (try 3)

[Bruno Jesus]

On Sun, Apr 15, 2012 at 07:38, Daniel Jelinski  wrote:
> Hello,
> ...
> Bug 19222 makes using MS SQL Server management studio a pain. The
> application displays database structure in a tree view, and most
> actions are executed from a context menu displayed by right-clicking
> on a relevant position in the tree view. Unfortunately Wine suffers
> from a bug - right click displays the menu, but selecting any option
> from the menu causes another menu to be displayed at the current mouse
> position. Selecting an option from this second menu works as expected.

This problem is also seem in FlashFXP so your patch may fix it there as well.
http://bugs.winehq.org/show_bug.cgi?id=19350#c4
> Thanks for your patience,
> Daniel

Bruno