[PATCH 2/2 v3] user32: Add more dialog creation message tests

[Alexandre Julliard]

Dmitry Timoshkov <dmitry at baikal.ru> writes:

> Alexandre Julliard <julliard at winehq.org> wrote:
>
>> > Piotr Caban <piotr at codeweavers.com> wrote:
>> >> But it doesn't prove anything. It's something like: it's not possible to 
>> >> test if something is done in this place so lets test everything else to 
>> >> make sure it's not done there. Such tests can be added to test 
>> >> DefDlgProc, not to show that my patch is valid.
>> >> I'm not planning to add more tests related to this code. I think that 
>> >> this change is quite obvious and already well tested.
>> >
>> > Many of the tests in msg.c and win.c are created by me, I know the related
>> > code in user32 pretty well, and based on my knowledge I can't agree that
>> > your change is obvious, at least it's absolutely not obvious to me. Focus
>> > handling is very fragile part of user32 code, there are applications that
>> > are very sensitive to any change in that area, and it's very easy to break
>> > them. Please take time to create a more convincing set of tests.
>> 
>> The focus handling is definitely not obvious, but the fact that setting
>> the focus should also select the edit control seems pretty clear from
>> the message trace, and it matches what DefDlgProc already does in other
>> situations. What is it that you find not convincing here?  Are you
>> saying that the whole initial focus setting should be moved to
>> DefDlgProc?
>
> What makes me hesitant is the fact that text selection logic is basically
> duplicated in several places of the dialog management code. So, I'd like
> to know whether the dialog creation code needs somehow route part of that
> logic to DefDlgProc(), and if that's the case perhaps what is going on is
> that DefDlgProc() should be called as a part of dialog creation process.

It's called on WM_INITDIALOG of course. Or do you mean it should be
called explicitly by CreateDialog even with a different window class?
That would be fairly surprising IMO.

I agree the duplication looks suspicious, but the tests indicate that at
least on XP they are indeed different code paths. We could move the
initial focus setting code to the WM_INITDIALOG handling in DefDlgProc,
but that one would be a more risky change, and would definitely require
more tests. It seems orthogonal to Piotr's change though.

-- 
Alexandre Julliard
julliard at winehq.org