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