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...