user32: Use top level window as dialog parent for modal dialogs.

[Dmitry Timoshkov]

Jacek Caban <jacek at codeweavers.com> wrote:

> >> I will ask you one more time to go back to the code and being technical
> >> instead of personal.
> > Hmm? My comments have pure technical background, all I ask is about
> > adding more tests to show that your patch is correct and to prevent
> > possible regressions in future.
> 
> And if you want me to take you seriously as a reviewer, then start
> reading the code. Let me read it for you. We call EnableWindow() in two
> functions:
> 
> 1) DIALOG_CreateIndirect
> Before and after my patch, we use exactly the same window in
> DIALOG_CreateIndirect: I didn't change that logic at all. Do you see it?
> 
> 2) DIALOG_DoDialogBox
> In DIALOG_DoDialogBox we currently use GetWindow(GW_OWNER) (which is a
> subject for future changes). My patch doesn't change it as well. Here we
> have two cases:
> 
> 2a) dialog is WS_CHILD: For WS_CHILD, owner is NULL with and without my
> patch.
> 2b) For other cases, we have a real owner, but then again, the owner
> will be the same before and after my patch. We know that, I added a test
> and fix for that in [1] and I ensure that in tests included to the patch.
> 
> End of story.
> 
> As I told you, I will write a patch for that, when I will work on that.
> This patch is about other, unrelated, issue that I do test in the patch.

The source of the regression that your patch is supposedly tries to fix is
that the dialog code enables wrong window at the end of dialog message loop.
Neither old nor current tests don't even pretend that they are testing that
correct window is being enabled/disabled. And that's what I ask for: add such
tests. You had refused to add more tests previously and as the result we have
a regression.

Regarding the claims that only WS_CHILD style makes a difference: apply
my tests and in the tests replace the checks (WS_CHILD|WS_POPUP) == WS_CHILD
by just testing for WS_CHILD and the test will fail. If your tests don't
fail then your tests simply don't check all the possible style combinations
that may lead to failures.

> >  If that's too much of an additional effort for
> > you for some reason then I have no idea why you decided to work on this
> > at all, just drop this, user32 is too complex area for hasty craft, and
> > requires a lot of time to write the tests and investigate things.
> 
> And that's personal again.

Then every comment requesting to add more tests that actually confirm that
the proposed changes are correct while the sender doesn't want to write such
tests are personal. Refusing to create more tests to prove the correctness
of the patch doesn't add a lot of confidence to that patch at all. Writing
more tests is always an additional work, and claiming that a request to add
the tests is personal sounds at least strange.

-- 
Dmitry.