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

[Dmitry Timoshkov]

Hi Jacek,

Jacek Caban  wrote:
> This is part of the fix for bug 40282.

Could you please add actual tests that show the handling of disabled
window state? Neither current tests nor the new ones actually check
that at all, having such tests would prevent new regressions in this
area. Feel free to use my tests from the staging repository as a base
if desired, or write your own ones that use the same approach.

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

[Jacek Caban]

On 04/05/16 13:59, Dmitry Timoshkov wrote:
> Hi Jacek,
>
> Jacek Caban  wrote:
> > This is part of the fix for bug 40282.
>
> Could you please add actual tests that show the handling of disabled
> window state? Neither current tests nor the new ones actually check
> that at all, having such tests would prevent new regressions in this
> area. Feel free to use my tests from the staging repository as a base
> if desired, or write your own ones that use the same approach.

This patch is not directly related to enabled/disabled state of
parent/owner. I plan to send such tests together with the other part of
the fix. For the reference, I attached a patch that (on top on this one)
passes all your tests.

Thanks,
Jacek

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

[Dmitry Timoshkov]

Jacek Caban  wrote:
> > > This is part of the fix for bug 40282.
> >
> > Could you please add actual tests that show the handling of disabled
> > window state? Neither current tests nor the new ones actually check
> > that at all, having such tests would prevent new regressions in this
> > area. Feel free to use my tests from the staging repository as a base
> > if desired, or write your own ones that use the same approach.
>
> This patch is not directly related to enabled/disabled state of
> parent/owner. I plan to send such tests together with the other part of
> the fix.

It still looks like shooting in the dark to me without writing the tests
first and only then starting to fix the test failures one by one, so it
becomes absolutely clear what exactly is fixed and where. Once again I'd
like to point out that writing exhaustive tests before starting to add
any fixes is very important for better understanding of the problem,
especially in such fargile areas as user32 window management code.
> For the reference, I attached a patch that (on top on this one)
> passes all your tests.

I have more tests that show that removing DF_OWNERENABLED handling altogether
(like my own patches also do) is wrong. These tests are not part of the staged
patch set.

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

[Jacek Caban]

On 04/05/16 14:34, Dmitry Timoshkov wrote:
> Jacek Caban  wrote:
> > > > This is part of the fix for bug 40282.
> > >
> > > Could you please add actual tests that show the handling of disabled
> > > window state? Neither current tests nor the new ones actually check
> > > that at all, having such tests would prevent new regressions in this
> > > area. Feel free to use my tests from the staging repository as a base
> > > if desired, or write your own ones that use the same approach.
> >
> > This patch is not directly related to enabled/disabled state of
> > parent/owner. I plan to send such tests together with the other part of
> > the fix.
>
> It still looks like shooting in the dark to me without writing the tests
> first and only then starting to fix the test failures one by one, so it
> becomes absolutely clear what exactly is fixed and where. Once again I'd
> like to point out that writing exhaustive tests before starting to add
> any fixes is very important for better understanding of the problem,
> especially in such fargile areas as user32 window management code.

Actually, my patch shows that we handle parent window in a wrong way.
Period. That's what's obviously wrong and that's what I'm fixing at the
moment. I don't touch enabling and disabling owner in this patch. In
fact, it shouldn't change it at all. So please, forget about context of
this bug, do you still have any comment about this particular patch?
> > For the reference, I attached a patch that (on top on this one)
> > passes all your tests.
>
> I have more tests that show that removing DF_OWNERENABLED handling altogether
> (like my own patches also do) is wrong. These tests are not part of the staged
> patch set.

Yep, that's why I only attached it for the reference and didn't send it
to wine-patches. It needs more tests and I don't consider your test good
enough.

Jacek

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

[Dmitry Timoshkov]

Jacek Caban  wrote:
> > > > > This is part of the fix for bug 40282.
> > > >
> > > > Could you please add actual tests that show the handling of disabled
> > > > window state? Neither current tests nor the new ones actually check
> > > > that at all, having such tests would prevent new regressions in this
> > > > area. Feel free to use my tests from the staging repository as a base
> > > > if desired, or write your own ones that use the same approach.
> > >
> > > This patch is not directly related to enabled/disabled state of
> > > parent/owner. I plan to send such tests together with the other part of
> > > the fix.
> >
> > It still looks like shooting in the dark to me without writing the tests
> > first and only then starting to fix the test failures one by one, so it
> > becomes absolutely clear what exactly is fixed and where. Once again I'd
> > like to point out that writing exhaustive tests before starting to add
> > any fixes is very important for better understanding of the problem,
> > especially in such fargile areas as user32 window management code.
>
> Actually, my patch shows that we handle parent window in a wrong way.
> Period. That's what's obviously wrong and that's what I'm fixing at the
> moment. I don't touch enabling and disabling owner in this patch. In
> fact, it shouldn't change it at all. So please, forget about context of
> this bug, do you still have any comment about this particular patch?

It seems that we heard something similar when you sent the patches last
time that have caused this regression. And absense of good tests in both
cases doesn't add a lot of confidence that there will be no new regressions.
> > > For the reference, I attached a patch that (on top on this one)
> > > passes all your tests.
> >
> > I have more tests that show that removing DF_OWNERENABLED handling altogether
> > (like my own patches also do) is wrong. These tests are not part of the staged
> > patch set.
>
> Yep, that's why I only attached it for the reference and didn't send it
> to wine-patches. It needs more tests and I don't consider your test good
> enough.

Probably my tests are not good for you, but please show your tests first,
so we could make a comparison. Btw, it's pretty clear to me that you used
my tests as a reference for of your test, if my tests were that bad you
wouldn't do that, right? :)

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

[Jacek Caban]

On 04/05/16 15:04, Dmitry Timoshkov wrote:
> Jacek Caban  wrote:
> > > > > > This is part of the fix for bug 40282.
> > > > >
> > > > > Could you please add actual tests that show the handling of disabled
> > > > > window state? Neither current tests nor the new ones actually check
> > > > > that at all, having such tests would prevent new regressions in this
> > > > > area. Feel free to use my tests from the staging repository as a base
> > > > > if desired, or write your own ones that use the same approach.
> > > >
> > > > This patch is not directly related to enabled/disabled state of
> > > > parent/owner. I plan to send such tests together with the other part of
> > > > the fix.
> > >
> > > It still looks like shooting in the dark to me without writing the tests
> > > first and only then starting to fix the test failures one by one, so it
> > > becomes absolutely clear what exactly is fixed and where. Once again I'd
> > > like to point out that writing exhaustive tests before starting to add
> > > any fixes is very important for better understanding of the problem,
> > > especially in such fargile areas as user32 window management code.
> >
> > Actually, my patch shows that we handle parent window in a wrong way.
> > Period. That's what's obviously wrong and that's what I'm fixing at the
> > moment. I don't touch enabling and disabling owner in this patch. In
> > fact, it shouldn't change it at all. So please, forget about context of
> > this bug, do you still have any comment about this particular patch?
>
> It seems that we heard something similar when you sent the patches last
> time that have caused this regression. And absense of good tests in both
> cases doesn't add a lot of confidence that there will be no new regressions.

Once again: how is my *current* patch related at all to what you request?
> > > > For the reference, I attached a patch that (on top on this one)
> > > > passes all your tests.
> > >
> > > I have more tests that show that removing DF_OWNERENABLED handling altogether
> > > (like my own patches also do) is wrong. These tests are not part of the staged
> > > patch set.
> >
> > Yep, that's why I only attached it for the reference and didn't send it
> > to wine-patches. It needs more tests and I don't consider your test good
> > enough.
>
> Probably my tests are not good for you, but please show your tests first,
> so we could make a comparison. Btw, it's pretty clear to me that you used
> my tests as a reference for of your test, if my tests were that bad you
> wouldn't do that, right? :)

I said not good *enough*, I never said they are not good, so stop
accusing me about something I didn't say. And if I had tests ready, I'd
already send this *unrelated* patch. I will learn to not share with you
WIP diffs if you don't understand what they are.

And well, if it matters that much to you, your tests are nice for
overview of the bug, but it's hard to point the exact problem from them,
so more detailed tests written with implementation in mind are needed
and that's how I discovered the problem I'm fixing. They also show that
we're only interested in only WS_CHILD style (and usually just for
wine_todos), so calling them for all style combinations feels a bit like
out-placed shooting in the dark, not showing the exact problem. More
detailed tests are required to show exact problems with WS_CHILD in
dialog templates.

Jacek

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

[Dmitry Timoshkov]

Jacek Caban  wrote:
> > > > > > > This is part of the fix for bug 40282.
> > > > > > Could you please add actual tests that show the handling of disabled
> > > > > > window state? Neither current tests nor the new ones actually check
> > > > > > that at all, having such tests would prevent new regressions in this
> > > > > > area. Feel free to use my tests from the staging repository as a base
> > > > > > if desired, or write your own ones that use the same approach.
> > > > >
> > > > > This patch is not directly related to enabled/disabled state of
> > > > > parent/owner. I plan to send such tests together with the other part of
> > > > > the fix.
> > > >
> > > > It still looks like shooting in the dark to me without writing the tests
> > > > first and only then starting to fix the test failures one by one, so it
> > > > becomes absolutely clear what exactly is fixed and where. Once again I'd
> > > > like to point out that writing exhaustive tests before starting to add
> > > > any fixes is very important for better understanding of the problem,
> > > > especially in such fargile areas as user32 window management code.
> > >
> > > Actually, my patch shows that we handle parent window in a wrong way.
> > > Period. That's what's obviously wrong and that's what I'm fixing at the
> > > moment. I don't touch enabling and disabling owner in this patch. In
> > > fact, it shouldn't change it at all. So please, forget about context of
> > > this bug, do you still have any comment about this particular patch?
> >
> > It seems that we heard something similar when you sent the patches last
> > time that have caused this regression. And absense of good tests in both
> > cases doesn't add a lot of confidence that there will be no new regressions.
>
> Once again: how is my *current* patch related at all to what you request?

All ask you about is writing a comprehensive set of test cases before
you start trying to fix anything. Your fixes simply lack any resonable
justification from the tests. And yes, the regression is caused by enabling
wrong window at the end of dialog message loop, that's why I'm asking
about the tests that check disabled window state.

I'm sure that if I would send such a patch on my own it would be immediately
rejected with the "needs tests" motivation.
> > > > > For the reference, I attached a patch that (on top on this one)
> > > > > passes all your tests.
> > > >
> > > > I have more tests that show that removing DF_OWNERENABLED handling altogether
> > > > (like my own patches also do) is wrong. These tests are not part of the staged
> > > > patch set.
> > >
> > > Yep, that's why I only attached it for the reference and didn't send it
> > > to wine-patches. It needs more tests and I don't consider your test good
> > > enough.
> >
> > Probably my tests are not good for you, but please show your tests first,
> > so we could make a comparison. Btw, it's pretty clear to me that you used
> > my tests as a reference for of your test, if my tests were that bad you
> > wouldn't do that, right? :)
>
> I said not good *enough*, I never said they are not good, so stop
> accusing me about something I didn't say. And if I had tests ready, I'd
> already send this *unrelated* patch. I will learn to not share with you
> WIP diffs if you don't understand what they are.

Apparently my comment was not about the WIP diff, and I didn't accuse you
in anything, just mentioned an obvious fact.
> And well, if it matters that much to you, your tests are nice for
> overview of the bug, but it's hard to point the exact problem from them,
> so more detailed tests written with implementation in mind are needed
> and that's how I discovered the problem I'm fixing. They also show that
> we're only interested in only WS_CHILD style (and usually just for
> wine_todos), so calling them for all style combinations feels a bit like
> out-placed shooting in the dark, not showing the exact problem. More
> detailed tests are required to show exact problems with WS_CHILD in
> dialog templates.

Probaly testing all the style combinations is an overkill, but if there is
an existing test that already loops around all of them and adding new tests
there seems like an easy way of checking whether there are any other styles
besides WS_CHILD, WS_POPUP and WS_EX_CONTROLPARENT that may change things
why don't use that opportunity? And without testing them all I'd personally
refrain from claims that only WS_CHILD makes difference. Or you prefer to
wait for another regression report?

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

[Jacek Caban]

Hi Dmitry,

I will ask you one more time to go back to the code and being technical
instead of personal. Take a look at the code and show me a code path,
that makes enabling or disabling any different than before the patch. If
you can't then you will understand that the change is unrelated. If you
do, I'm happy to write a test for that case.

Jacek

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

[Sebastian Lackner]

On 05.04.2016 16:28, Jacek Caban wrote:
> Hi Dmitry,
>
> I will ask you one more time to go back to the code and being technical
> instead of personal. Take a look at the code and show me a code path,
> that makes enabling or disabling any different than before the patch. If
> you can't then you will understand that the change is unrelated. If you
> do, I'm happy to write a test for that case.
>
> Jacek

To both of you, please calm down a bit. Whereas there is nothing obviously wrong
with the patch, I can also understand Dmitrys request to merge more tests first,
before we come to the conclusion that this solution must be right. In fact, when
taking into account the feedback I received quite often to my own patches, the
second approach (first ensure tests cover all areas, then changing the
implementation) is indeed preferred for Wine, so I'm not really sure what the
problem is here. How should Dmitry add his own tests when you want to send all
in a single patch? Since Dmitry already did the effort to track down your
regression, wouldn't it be useful to trust him a bit more and to cooperate?

Imho, what we especially need to ensure is that wineserver handling for various
flag combinations is really correct, before we modify the owner on user32 side.
Based on some tests I did a while ago (when handing for WS_POPUP | WS_CHILD was
added), there are definitely remaining todos. Also please note that neither your
old nor your new code correctly replicates wineserver behaviour because
WIN_CreateWindowEx contains a check: if ((cs->style & (WS_CHILD|WS_POPUP)) != WS_CHILD)
Under specific circumstances the owner argument is simply ignored.

Regards,
Sebastian

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

[Jacek Caban]

Hi Sebastian,

On 04/05/16 17:25, Sebastian Lackner wrote:
> On 05.04.2016 16:28, Jacek Caban wrote:
> > Hi Dmitry,
> >
> > I will ask you one more time to go back to the code and being technical
> > instead of personal. Take a look at the code and show me a code path,
> > that makes enabling or disabling any different than before the patch. If
> > you can't then you will understand that the change is unrelated. If you
> > do, I'm happy to write a test for that case.
> >
> > Jacek
>
> To both of you, please calm down a bit. Whereas there is nothing obviously wrong
> with the patch, I can also understand Dmitrys request to merge more tests first,
> before we come to the conclusion that this solution must be right. In fact, when
> taking into account the feedback I received quite often to my own patches, the
> second approach (first ensure tests cover all areas, then changing the
> implementation) is indeed preferred for Wine, so I'm not really sure what the
> problem is here. How should Dmitry add his own tests when you want to send all
> in a single patch? Since Dmitry already did the effort to track down your
> regression, wouldn't it be useful to trust him a bit more and to cooperate?

To be honest, I don't. I said it multiple time: his requests are
unrelated. I will send those tests when they will be related to the
patch I send. In fact I'd be writing them now if I wouldn't have to deal
with this nonsense. But still and again, this it's unrelated to this
patch, so this stays as is. Let's keep it separated, I will try to
explain it one last time in a different mail.
> Imho, what we especially need to ensure is that wineserver handling for various
> flag combinations is really correct, before we modify the owner on user32 side.
> Based on some tests I did a while ago (when handing for WS_POPUP | WS_CHILD was
> added), there are definitely remaining todos. Also please note that neither your
> old nor your new code correctly replicates wineserver behaviour because
> WIN_CreateWindowEx contains a check: if ((cs->style & (WS_CHILD|WS_POPUP)) != WS_CHILD)
> Under specific circumstances the owner argument is simply ignored.

Yes, it's intentionally different, because my tests show it should be
different. If we pass WS_CHILD, then the argument that we pass as owner
is in fact a parent. A parent does not have the requirements of being
top level window (or one with no WS_CHILD flag to be precise). Otherwise
it's an owner, which has the restriction. That's how CreateWindow works,
it's tested and fixed by [1]. Tests that I included in the patch show
that dialogs created by DialogBoxIndirectParam are different. They have
the same restriction applied to the case with WS_CHILD as if it was an
owner. My tests clearly show that. So to conclude:
- I did fix todos you talk about. At least ones I know about.
- I did add test that proves it's different in this case and that it has
to be done in user32.
Does that sound right to you?

Jacek

[1]
http://source.winehq.org/git/wine.git/commitdiff/d13a44e4aa12fa5ad3498c39934...

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

[Dmitry Timoshkov]

Jacek Caban  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.
> Take a look at the code and show me a code path,
> that makes enabling or disabling any different than before the patch. If
> you can't then you will understand that the change is unrelated. If you
> do, I'm happy to write a test for that case.

Sorry, but that's your job to demonstrate with a bunch of test cases
that your patch is correct, I'm just pointing out to obvious omissions
in the provided tests. 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.

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

[Jacek Caban]

On 04/05/16 17:55, Dmitry Timoshkov wrote:
> Jacek Caban  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.
> 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.

Jacek

[1]
http://source.winehq.org/git/wine.git/commitdiff/d13a44e4aa12fa5ad3498c39934...

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

[Dmitry Timoshkov]

Jacek Caban  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:
> - 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?
> - 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.

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

[Jacek Caban]

On 4/6/16 5:32 AM, Dmitry Timoshkov wrote:
> 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.

No, it fixes issue I found while looking at this, but it's a separated 
and isolated issue. I don't touch the code that you ask me to write a 
test for. You fail to understand that.

BTW, your tests don't test what you ask me to test and they were enough 
for you to write a wrong fix. Otherwise you'd know that this code in the 
end of the loop should be removed. Then again, it's not related to this 
particular patch.

Jacek

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

[Dmitry Timoshkov]

Jacek Caban  wrote:
> > 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.
>
> No, it fixes issue I found while looking at this, but it's a separated 
> and isolated issue. I don't touch the code that you ask me to write a 
> test for. You fail to understand that.

I perfectly understand what you are trying to fix, and you actually mentioned
it yourself already: "This is part of the fix for bug 40282". And that bug is
a regression caused by your previous changes not backed up by the tests, and
I already explained that the source of the regression is in wrong handling
of enable/disable of the dialog owner, and once again I'd like to point out
that fixing this without any tests simply can't work.
> BTW, your tests don't test what you ask me to test and they were enough 
> for you to write a wrong fix. Otherwise you'd know that this code in the 
> end of the loop should be removed. Then again, it's not related to this 
> particular patch.

Well, at lease I have written some tests before I tried to fix anything,
and I'm not claiming that my fix is perfect/correct/useful at all. You are
more than welcome to show your own exhaustive tests that check every detail
of the dialog owner enable/disable logic.

P.S.
I'm not the person who commits patches to Wine, why are you arguing with me
if it's pretty clear that you are not going to write any tests at all in order
to demonstrate that your patch is correct?

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

[Jacek Caban]

On 4/6/16 10:05 AM, Dmitry Timoshkov wrote:
> Jacek Caban  wrote:
> > > 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.
> >
> > No, it fixes issue I found while looking at this, but it's a separated
> > and isolated issue. I don't touch the code that you ask me to write a
> > test for. You fail to understand that.
>
> I perfectly understand what you are trying to fix, and you actually mentioned
> it yourself already: "This is part of the fix for bug 40282". And that bug is
> a regression caused by your previous changes not backed up by the tests, and
> I already explained that the source of the regression is in wrong handling
> of enable/disable of the dialog owner, and once again I'd like to point out
> that fixing this without any tests simply can't work.

You said it yourself, you comment on my comment, not the code. I added 
tests for the code I change.
> > BTW, your tests don't test what you ask me to test and they were enough
> > for you to write a wrong fix. Otherwise you'd know that this code in the
> > end of the loop should be removed. Then again, it's not related to this
> > particular patch.
>
> Well, at lease I have written some tests before I tried to fix anything,
> and I'm not claiming that my fix is perfect/correct/useful at all. You are
> more than welcome to show your own exhaustive tests that check every detail
> of the dialog owner enable/disable logic.
> P.S.
> I'm not the person who commits patches to Wine, why are you arguing with me
> if it's pretty clear that you are not going to write any tests at all in order
> to demonstrate that your patch is correct?

I already told you a few time that I will send it when I will be 
relevant. In fact I already did, but it's not yet ready for sending.

Stop wasting my time.

Jacek

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

[Dmitry Timoshkov]

Jacek Caban  wrote:
> > > > 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.
> > >
> > > No, it fixes issue I found while looking at this, but it's a separated
> > > and isolated issue. I don't touch the code that you ask me to write a
> > > test for. You fail to understand that.
> >
> > I perfectly understand what you are trying to fix, and you actually mentioned
> > it yourself already: "This is part of the fix for bug 40282". And that bug is
> > a regression caused by your previous changes not backed up by the tests, and
> > I already explained that the source of the regression is in wrong handling
> > of enable/disable of the dialog owner, and once again I'd like to point out
> > that fixing this without any tests simply can't work.
>
> You said it yourself, you comment on my comment, not the code. I added 
> tests for the code I change.

Your assumption is incorrect, but I really feel discouraged by your attempts
to pretend that I comment not on your patch or that your patch fixes something
not related to my comments.
> > > BTW, your tests don't test what you ask me to test and they were enough
> > > for you to write a wrong fix. Otherwise you'd know that this code in the
> > > end of the loop should be removed. Then again, it's not related to this
> > > particular patch.
> >
> > Well, at lease I have written some tests before I tried to fix anything,
> > and I'm not claiming that my fix is perfect/correct/useful at all. You are
> > more than welcome to show your own exhaustive tests that check every detail
> > of the dialog owner enable/disable logic.
> > P.S.
> > I'm not the person who commits patches to Wine, why are you arguing with me
> > if it's pretty clear that you are not going to write any tests at all in order
> > to demonstrate that your patch is correct?
>
> I already told you a few time that I will send it when I will be 
> relevant. In fact I already did, but it's not yet ready for sending.
>
> Stop wasting my time.

Why are you replying at all? Once again: I'm not the person who commits
the patches to Wine, and I'm pretty skeptical that this kind of replies
is going to help to strengthen your arguments.

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

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> P.S.
> I'm not the person who commits patches to Wine, why are you arguing with me
> if it's pretty clear that you are not going to write any tests at all in order
> to demonstrate that your patch is correct?

I'm not committing the patches until I'm satisfied that the tests are
sufficient, and if you feel that they are not, I want to understand why,
and make sure that the necessary tests get written. Your technical
feedback is welcome and appreciated.

But accusing Jacek of refusing to write tests at all is neither true nor
helpful. It would be nice if you could refrain from such comments and
concentrate on technical issues, where I believe that you do have useful
knowledge to contribute. Can we please all try to get along?

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

[Dmitry Timoshkov]

Alexandre Julliard  wrote:
> > P.S.
> > I'm not the person who commits patches to Wine, why are you arguing with me
> > if it's pretty clear that you are not going to write any tests at all in order
> > to demonstrate that your patch is correct?
>
> I'm not committing the patches until I'm satisfied that the tests are
> sufficient, and if you feel that they are not, I want to understand why,
> and make sure that the necessary tests get written. Your technical
> feedback is welcome and appreciated.

I think that I've already described the technical details about both the source
of the regression that Jacek is trying to fix, and the fact that the tests that
could confirm that the fix is correct simply don't exist at all.
> But accusing Jacek of refusing to write tests at all is neither true nor
> helpful. It would be nice if you could refrain from such comments and
> concentrate on technical issues, where I believe that you do have useful
> knowledge to contribute. Can we please all try to get along?

So, is Jacek actually going to write the tests that would help avoiding new
regressions in the area of dialog owner enable/disable management logic
instead of pretending that his patch fixes something else?

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

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> I think that I've already described the technical details about both the source
> of the regression that Jacek is trying to fix, and the fact that the tests that
> could confirm that the fix is correct simply don't exist at all.

I haven't seen Jacek claim that the patch fixes the regression. I think
we all agree that more changes are needed, with corresponding tests of
course.

As far as I can tell, your objection is not to this specific patch, but
to the fact that we shouldn't commit anything until we have a full
solution?  If so, I certainly don't have a problem with waiting.
> So, is Jacek actually going to write the tests that would help avoiding new
> regressions in the area of dialog owner enable/disable management logic
> instead of pretending that his patch fixes something else?

Yes, he is writing more tests, he said so already.

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

[Dmitry Timoshkov]

Alexandre Julliard  wrote:
> > I think that I've already described the technical details about both the source
> > of the regression that Jacek is trying to fix, and the fact that the tests that
> > could confirm that the fix is correct simply don't exist at all.
>
> I haven't seen Jacek claim that the patch fixes the regression. I think
> we all agree that more changes are needed, with corresponding tests of
> course.
>
> As far as I can tell, your objection is not to this specific patch, but
> to the fact that we shouldn't commit anything until we have a full
> solution?  If so, I certainly don't have a problem with waiting.

My objection is that this patch pretends to be a partial fix for the
regression while there is no any tests for the regression source at all,
or the tests that actually demonstrate how all of this is supposed to work
with correct implementation.

At the same time we see numerous claims that my tests are not good enough
and the fix I've created for this regression is not correct, while there
nothing has been shown as a viable alternative to them, just empty claims
and hints (stay tuned, I'm working on something!) and accusations that I'm
not understanding the proposed fix.

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

[Jacek Caban]

On 04/06/16 11:43, Dmitry Timoshkov wrote:
> So, is Jacek actually going to write the tests that would help avoiding new
> regressions in the area of dialog owner enable/disable management logic

Once again: yes and I don't need you to tell me that, I'd do that
anyway, it's more than obvious to me. I will send those tests when I
will fix it. Still, this patch is unrelated.
> instead of pretending that his patch fixes something else?

I don't pretend anything, go back to the code and see. I fix parent of
created window. See it yourself, it's right there. That's it.

Jacek

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

[Dmitry Timoshkov]

Jacek Caban  wrote:
> > instead of pretending that his patch fixes something else?
>
> I don't pretend anything, go back to the code and see. I fix parent of
> created window. See it yourself, it's right there. That's it.

You are clamimg that your patch is a partial fix for the regression,
perhaps it's better to start with sending the tests that show what's
wrong with current implementation and only then start sending patches
one by one with fixes for every problem found by the tests targeted
separately?

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

[Jacek Caban]

On 04/06/16 13:00, Dmitry Timoshkov wrote:
> You are clamimg

And again, you comment the claim, not the code.

Jacek