Re: [PATCH 3/5] user32: Don't enable owner in DIALOG_DoDialogBox.
Jacek Caban <jacek(a)codeweavers.com> wrote:
--- a/dlls/user32/controls.h +++ b/dlls/user32/controls.h @@ -241,7 +241,6 @@ typedef struct tagDIALOGINFO } DIALOGINFO;
#define DF_END 0x0001 -#define DF_OWNERENABLED 0x0002
As I already mentioned during our conversation in wine-devel my (not yet submitted) tests show that completely removing DF_OWNERENABLED handling is wrong. And once again I'd like to see the tests submitted first with todo_wine statements in appropriate places, and the patches sent only after that with separate fixes for every failure found by the tests. Sending a bunch of tests as the final patch doesn't show what didn't actually work before, and what exactly has been fixed by the patch series. -- Dmitry.
On 04/06/16 15:04, Dmitry Timoshkov wrote:
Jacek Caban <jacek(a)codeweavers.com> wrote:
--- a/dlls/user32/controls.h +++ b/dlls/user32/controls.h @@ -241,7 +241,6 @@ typedef struct tagDIALOGINFO } DIALOGINFO;
#define DF_END 0x0001 -#define DF_OWNERENABLED 0x0002 As I already mentioned during our conversation in wine-devel my (not yet submitted) tests show that completely removing DF_OWNERENABLED handling is wrong.
Well, you mentioned it in context of your patches where you remove it and always enable owner. I agree that's wrong, my tests show that. I remove it in a different way. Could you share those tests (WIP is fine) or run my patches against them?
And once again I'd like to see the tests submitted first with todo_wine statements in appropriate places, and the patches sent only after that with separate fixes for every failure found by the tests. Sending a bunch of tests as the final patch doesn't show what didn't actually work before, and what exactly has been fixed by the patch series.
Not really, those tests heavily depend on WM_ENTERIDLE being sent to the right window. Otherwise tests would hang so todo_wine won't help much. Jacek
Jacek Caban <jacek(a)codeweavers.com> wrote:
On 04/06/16 15:24, Jacek Caban wrote:
Could you share those tests (WIP is fine) or run my patches against them?
Ping. Did you have a chance to run those tests?
No. But since you already know that removing DF_OWNERENABLED handling is wrong it shouldn't be too hard to add appropriate tests. -- Dmitry.
On 04/11/16 01:06, Dmitry Timoshkov wrote:
Jacek Caban <jacek(a)codeweavers.com> wrote:
On 04/06/16 15:24, Jacek Caban wrote:
Could you share those tests (WIP is fine) or run my patches against them? Ping. Did you have a chance to run those tests? No. But since you already know that removing DF_OWNERENABLED handling is wrong it shouldn't be too hard to add appropriate tests.
No, as far as I know removing is right and I added tests that reach EnableWindow() in every possible code path. I know only that you said it's wrong with no prove to back that. I guess that you misinterpreted results of your own tests and the fact that you claim you didn't bother to run my patches against those tests tells pretty much all about quality and intentions of your reviews. Jacek
Jacek Caban <jacek(a)codeweavers.com> wrote:
On 04/06/16 15:24, Jacek Caban wrote:
Could you share those tests (WIP is fine) or run my patches against them? Ping. Did you have a chance to run those tests? No. But since you already know that removing DF_OWNERENABLED handling is wrong it shouldn't be too hard to add appropriate tests.
No, as far as I know removing is right and I added tests that reach EnableWindow() in every possible code path. I know only that you said it's wrong with no prove to back that. I guess that you misinterpreted results of your own tests and the fact that you claim you didn't bother to run my patches against those tests tells pretty much all about quality and intentions of your reviews.
Actually the fact that you can't reproduce the described above behaviour with in your own tests tells quite a bit (yes, EndDialog() is not supposed to enable a previously disabled owner under some conditions). You really shouldn't be surprised that much by the lack of help from me to reproduce this really simple thing: you decided to dismiss the cooperation request from Sebastian, and were pretty unpleasant during the whole conversation. -- Dmitry.
On 04/11/16 13:21, Dmitry Timoshkov wrote:
Actually the fact that you can't reproduce the described above behaviour with in your own tests tells quite a bit (yes, EndDialog() is not supposed to enable a previously disabled owner under some conditions).
Yes, that's the case if you reparent the dialog. My tests show that and implementation behaves as expected. We don't need DF_OWNERENABLED for that.
you decided to dismiss the cooperation request from Sebastian,
I didn't dismiss it, I replied to his technical objections. I think I explained things enough given that he didn't have more comments. Jacek
Jacek Caban <jacek(a)codeweavers.com> wrote:
Actually the fact that you can't reproduce the described above behaviour with in your own tests tells quite a bit (yes, EndDialog() is not supposed to enable a previously disabled owner under some conditions).
Yes, that's the case if you reparent the dialog. My tests show that and implementation behaves as expected. We don't need DF_OWNERENABLED for that.
If you mean the trick with GWLP_HWNDPARENT then I'm not sure it's the best or really correct way of solving the problem. But yes, it looks like a good workaround. -- Dmitry.
On 04/11/16 14:24, Dmitry Timoshkov wrote:
Jacek Caban <jacek(a)codeweavers.com> wrote:
Actually the fact that you can't reproduce the described above behaviour with in your own tests tells quite a bit (yes, EndDialog() is not supposed to enable a previously disabled owner under some conditions). Yes, that's the case if you reparent the dialog. My tests show that and implementation behaves as expected. We don't need DF_OWNERENABLED for that. If you mean the trick with GWLP_HWNDPARENT then I'm not sure it's the best or really correct way of solving the problem. But yes, it looks like a good workaround.
I'm going to ignore your comment about not being correct or being a workaround because they are not backed by any technical argument. All that's left is that it works as expected so your previous concern is already addressed by the patch. Thanks. Jacek
participants (2)
-
Dmitry Timoshkov -
Jacek Caban