Jacek Caban jacek@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?