April 2016 - Wine-Devel - list.winehq.org

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

by Jacek Caban

On 04/05/16 17:55, Dmitry Timoshkov wrote: > Jacek Caban <jacek(a)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. > 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/d13a44e4aa12fa5ad3498c3993…

April 5, 2016

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

by 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/d13a44e4aa12fa5ad3498c3993…

April 5, 2016

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

by Dmitry Timoshkov

Jacek Caban <jacek(a)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. > 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. -- Dmitry.

April 5, 2016

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

by 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

April 5, 2016

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

by 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

April 5, 2016

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

by Dmitry Timoshkov

Jacek Caban <jacek(a)codeweavers.com> 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? -- Dmitry.

April 5, 2016

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

by Jacek Caban

On 04/05/16 15:04, Dmitry Timoshkov wrote: > Jacek Caban <jacek(a)codeweavers.com> 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

April 5, 2016

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

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=21903 Your paranoid android. === w864 (64 bit win) === win.c:8544: Test failed: msg.message = 320

April 5, 2016

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

by Dmitry Timoshkov

Jacek Caban <jacek(a)codeweavers.com> 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? :) -- Dmitry.

April 5, 2016

Re: [PATCH 2/3] mshtml: Added IHTMLWindow6::postMessage semi-stub implementation.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=21874 Your paranoid android. === wxppro (32 bit) === Found no trace of the test summary line or of a crash === w2003std (32 bit) === Found no trace of the test summary line or of a crash === wvistau64 (32 bit) === Found no trace of the test summary line or of a crash === w2008s64 (32 bit) === Found no trace of the test summary line or of a crash === w7u (32 bit) === Found no trace of the test summary line or of a crash === w7pro64 (32 bit) === Found no trace of the test summary line or of a crash === w8 (32 bit) === Found no trace of the test summary line or of a crash === w864 (32 bit) === Found no trace of the test summary line or of a crash === w1064 (32 bit) === Found no trace of the test summary line or of a crash === wvistau64 (64 bit) === Found no trace of the test summary line or of a crash === w2008s64 (64 bit) === Found no trace of the test summary line or of a crash === w7pro64 (64 bit) === Found no trace of the test summary line or of a crash === w864 (64 bit) === Found no trace of the test summary line or of a crash === w1064 (64 bit) === Found no trace of the test summary line or of a crash

April 5, 2016