Re: [PATCH 2/2 v3] user32: Add more dialog creation message tests
Piotr Caban <piotr(a)codeweavers.com> wrote:
Signed-off-by: Piotr Caban <piotr(a)codeweavers.com> --- dlls/user32/tests/msg.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+)
It's getting better, but again doesn't show a lot of details. Please add parameters for most of the recorded messages in the sequence, in particular EM_SETSEL and WM_GETDLGCODE are the mandatory ones. Also, in order to see the origin of EM_SETSEL I'd suggest to call DefDlgProc() manually and use defwndproc_counter around it together with the defwinproc flag. -- Dmitry.
On 02/16/16 14:08, Dmitry Timoshkov wrote:
It's getting better, but again doesn't show a lot of details. Please add parameters for most of the recorded messages in the sequence, in particular EM_SETSEL and WM_GETDLGCODE are the mandatory ones. Also, in order to see the origin of EM_SETSEL I'd suggest to call DefDlgProc() manually and use defwndproc_counter around it together with the defwinproc flag.
DefDlgProc can't be called manually in this case. There are already some comments about it in the tests. Thanks, Piotr
Piotr Caban <piotr(a)codeweavers.com> wrote:
On 02/16/16 14:08, Dmitry Timoshkov wrote:
It's getting better, but again doesn't show a lot of details. Please add parameters for most of the recorded messages in the sequence, in particular EM_SETSEL and WM_GETDLGCODE are the mandatory ones. Also, in order to see the origin of EM_SETSEL I'd suggest to call DefDlgProc() manually and use defwndproc_counter around it together with the defwinproc flag.
DefDlgProc can't be called manually in this case. There are already some comments about it in the tests.
TestDlgProcA() calls DefDlgProc() this way without any problem. Even of calling DefDlgProc() is not desirable from inside of a dialog procedure for some reason, then the reason should be explained, and it's always possible to call DefDlgProc() outside of dialog proc directly and record the produced messages, there are examples in the tests how to do that. -- Dmitry.
On 17/02/16 06:20, Dmitry Timoshkov wrote:
Piotr Caban<piotr(a)codeweavers.com> wrote:
On 02/16/16 14:08, Dmitry Timoshkov wrote:
It's getting better, but again doesn't show a lot of details. Please add parameters for most of the recorded messages in the sequence, in particular EM_SETSEL and WM_GETDLGCODE are the mandatory ones. Also, in order to see the origin of EM_SETSEL I'd suggest to call DefDlgProc() manually and use defwndproc_counter around it together with the defwinproc flag. DefDlgProc can't be called manually in this case. There are already some comments about it in the tests. TestDlgProcA() calls DefDlgProc() this way without any problem. It's because this test is using CreateWindow function family instead of CreateDialog*. Even of calling DefDlgProc() is not desirable from inside of a dialog procedure for some reason, then the reason should be explained, As I said it's already explained in the tests. Look e.g. on comment in test_dlg_proc function. and it's always possible to call DefDlgProc() outside of dialog proc directly and record the produced messages, there are examples in the tests how to do that. But it doesn't prove anything. It's something like: it's not possible to test if something is done in this place so lets test everything else to make sure it's not done there. Such tests can be added to test DefDlgProc, not to show that my patch is valid. I'm not planning to add more tests related to this code. I think that this change is quite obvious and already well tested.
Thanks, Piotr
Piotr Caban <piotr(a)codeweavers.com> wrote:
On 02/16/16 14:08, Dmitry Timoshkov wrote:
It's getting better, but again doesn't show a lot of details. Please add parameters for most of the recorded messages in the sequence, in particular EM_SETSEL and WM_GETDLGCODE are the mandatory ones. Also, in order to see the origin of EM_SETSEL I'd suggest to call DefDlgProc() manually and use defwndproc_counter around it together with the defwinproc flag. DefDlgProc can't be called manually in this case. There are already some comments about it in the tests. TestDlgProcA() calls DefDlgProc() this way without any problem. It's because this test is using CreateWindow function family instead of CreateDialog*.
Then please create a window for testing using a way that is appropriate for your testing purposes and that allows using DefDlgProc().
Even of calling DefDlgProc() is not desirable from inside of a dialog procedure for some reason, then the reason should be explained, As I said it's already explained in the tests. Look e.g. on comment in test_dlg_proc function.
Since I've created that function I know why that comment has been added, and that comment has nothing to do with the way of testing I described above.
and it's always possible to call DefDlgProc() outside of dialog proc directly and record the produced messages, there are examples in the tests how to do that. But it doesn't prove anything. It's something like: it's not possible to test if something is done in this place so lets test everything else to make sure it's not done there. Such tests can be added to test DefDlgProc, not to show that my patch is valid. I'm not planning to add more tests related to this code. I think that this change is quite obvious and already well tested.
Many of the tests in msg.c and win.c are created by me, I know the related code in user32 pretty well, and based on my knowledge I can't agree that your change is obvious, at least it's absolutely not obvious to me. Focus handling is very fragile part of user32 code, there are applications that are very sensitive to any change in that area, and it's very easy to break them. Please take time to create a more convincing set of tests. -- Dmitry.
On 17/02/16 10:37, Dmitry Timoshkov wrote:
On 02/16/16 14:08, Dmitry Timoshkov wrote:
It's getting better, but again doesn't show a lot of details. Please add parameters for most of the recorded messages in the sequence, in particular EM_SETSEL and WM_GETDLGCODE are the mandatory ones. Also, in order to see the origin of EM_SETSEL I'd suggest to call DefDlgProc() manually and use defwndproc_counter around it together with the defwinproc flag. DefDlgProc can't be called manually in this case. There are already some comments about it in the tests. TestDlgProcA() calls DefDlgProc() this way without any problem. It's because this test is using CreateWindow function family instead of CreateDialog*. Then please create a window for testing using a way that is appropriate for your testing purposes and that allows using DefDlgProc(). You're not paying much attention to what's in the patch. I'm modifying part of CreateDialog function so it can't be tested if CreateWindow is used. Many of the tests in msg.c and win.c are created by me, I know the related code in user32 pretty well, and based on my knowledge I can't agree that your change is obvious, at least it's absolutely not obvious to me. Focus handling is very fragile part of user32 code, there are applications that are very sensitive to any change in that area, and it's very easy to break them. Please take time to create a more convincing set of tests. I'm not changing anything focus related. I'm just adding text selection to code that was already setting focus.
Piotr Caban <piotr(a)codeweavers.com> wrote:
On 17/02/16 10:37, Dmitry Timoshkov wrote:
On 02/16/16 14:08, Dmitry Timoshkov wrote:
It's getting better, but again doesn't show a lot of details. Please add parameters for most of the recorded messages in the sequence, in particular EM_SETSEL and WM_GETDLGCODE are the mandatory ones. Also, in order to see the origin of EM_SETSEL I'd suggest to call DefDlgProc() manually and use defwndproc_counter around it together with the defwinproc flag. DefDlgProc can't be called manually in this case. There are already some comments about it in the tests. TestDlgProcA() calls DefDlgProc() this way without any problem. It's because this test is using CreateWindow function family instead of CreateDialog*. Then please create a window for testing using a way that is appropriate for your testing purposes and that allows using DefDlgProc(). You're not paying much attention to what's in the patch. I'm modifying part of CreateDialog function so it can't be tested if CreateWindow is used.
I'm sure that there are ways of testing of the discussed functionality, and personally I find exisitng tests not convincing, and not explaining the reason of adding a proposed patch.
Many of the tests in msg.c and win.c are created by me, I know the related code in user32 pretty well, and based on my knowledge I can't agree that your change is obvious, at least it's absolutely not obvious to me. Focus handling is very fragile part of user32 code, there are applications that are very sensitive to any change in that area, and it's very easy to break them. Please take time to create a more convincing set of tests. I'm not changing anything focus related. I'm just adding text selection to code that was already setting focus.
Your patch is changing a part of the code that handles setting the focus, so it is focus related. I already explained the reason why I ask for additional tests, I only could add that I spent too much time already figuring out all kinds of broken user32 behaviour, and I definitely don't won't to see more of questionable code added in that area. Adding more tests is always good, it not only excersises the functionality of the target, it's also a way to better understand behind the scene. user32 is not the piece of code that can be changed just basing on a test of couple lines long, this area required much more tests to confirm that the observed behaviour and the proposed change are absolutely correct. Please demonstrate with the tests that you deeply understand what is going on in the area your patch is testing. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Piotr Caban <piotr(a)codeweavers.com> wrote:
But it doesn't prove anything. It's something like: it's not possible to test if something is done in this place so lets test everything else to make sure it's not done there. Such tests can be added to test DefDlgProc, not to show that my patch is valid. I'm not planning to add more tests related to this code. I think that this change is quite obvious and already well tested.
Many of the tests in msg.c and win.c are created by me, I know the related code in user32 pretty well, and based on my knowledge I can't agree that your change is obvious, at least it's absolutely not obvious to me. Focus handling is very fragile part of user32 code, there are applications that are very sensitive to any change in that area, and it's very easy to break them. Please take time to create a more convincing set of tests.
The focus handling is definitely not obvious, but the fact that setting the focus should also select the edit control seems pretty clear from the message trace, and it matches what DefDlgProc already does in other situations. What is it that you find not convincing here? Are you saying that the whole initial focus setting should be moved to DefDlgProc? -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard <julliard(a)winehq.org> wrote:
Piotr Caban <piotr(a)codeweavers.com> wrote:
But it doesn't prove anything. It's something like: it's not possible to test if something is done in this place so lets test everything else to make sure it's not done there. Such tests can be added to test DefDlgProc, not to show that my patch is valid. I'm not planning to add more tests related to this code. I think that this change is quite obvious and already well tested.
Many of the tests in msg.c and win.c are created by me, I know the related code in user32 pretty well, and based on my knowledge I can't agree that your change is obvious, at least it's absolutely not obvious to me. Focus handling is very fragile part of user32 code, there are applications that are very sensitive to any change in that area, and it's very easy to break them. Please take time to create a more convincing set of tests.
The focus handling is definitely not obvious, but the fact that setting the focus should also select the edit control seems pretty clear from the message trace, and it matches what DefDlgProc already does in other situations. What is it that you find not convincing here? Are you saying that the whole initial focus setting should be moved to DefDlgProc?
What makes me hesitant is the fact that text selection logic is basically duplicated in several places of the dialog management code. So, I'd like to know whether the dialog creation code needs somehow route part of that logic to DefDlgProc(), and if that's the case perhaps what is going on is that DefDlgProc() should be called as a part of dialog creation process. -- Dmitry.
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Alexandre Julliard <julliard(a)winehq.org> wrote:
Piotr Caban <piotr(a)codeweavers.com> wrote:
But it doesn't prove anything. It's something like: it's not possible to test if something is done in this place so lets test everything else to make sure it's not done there. Such tests can be added to test DefDlgProc, not to show that my patch is valid. I'm not planning to add more tests related to this code. I think that this change is quite obvious and already well tested.
Many of the tests in msg.c and win.c are created by me, I know the related code in user32 pretty well, and based on my knowledge I can't agree that your change is obvious, at least it's absolutely not obvious to me. Focus handling is very fragile part of user32 code, there are applications that are very sensitive to any change in that area, and it's very easy to break them. Please take time to create a more convincing set of tests.
The focus handling is definitely not obvious, but the fact that setting the focus should also select the edit control seems pretty clear from the message trace, and it matches what DefDlgProc already does in other situations. What is it that you find not convincing here? Are you saying that the whole initial focus setting should be moved to DefDlgProc?
What makes me hesitant is the fact that text selection logic is basically duplicated in several places of the dialog management code. So, I'd like to know whether the dialog creation code needs somehow route part of that logic to DefDlgProc(), and if that's the case perhaps what is going on is that DefDlgProc() should be called as a part of dialog creation process.
It's called on WM_INITDIALOG of course. Or do you mean it should be called explicitly by CreateDialog even with a different window class? That would be fairly surprising IMO. I agree the duplication looks suspicious, but the tests indicate that at least on XP they are indeed different code paths. We could move the initial focus setting code to the WM_INITDIALOG handling in DefDlgProc, but that one would be a more risky change, and would definitely require more tests. It seems orthogonal to Piotr's change though. -- Alexandre Julliard julliard(a)winehq.org
Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
Alexandre Julliard <julliard(a)winehq.org> wrote:
Piotr Caban <piotr(a)codeweavers.com> wrote:
But it doesn't prove anything. It's something like: it's not possible to test if something is done in this place so lets test everything else to make sure it's not done there. Such tests can be added to test DefDlgProc, not to show that my patch is valid. I'm not planning to add more tests related to this code. I think that this change is quite obvious and already well tested.
Many of the tests in msg.c and win.c are created by me, I know the related code in user32 pretty well, and based on my knowledge I can't agree that your change is obvious, at least it's absolutely not obvious to me. Focus handling is very fragile part of user32 code, there are applications that are very sensitive to any change in that area, and it's very easy to break them. Please take time to create a more convincing set of tests.
The focus handling is definitely not obvious, but the fact that setting the focus should also select the edit control seems pretty clear from the message trace, and it matches what DefDlgProc already does in other situations. What is it that you find not convincing here? Are you saying that the whole initial focus setting should be moved to DefDlgProc?
What makes me hesitant is the fact that text selection logic is basically duplicated in several places of the dialog management code. So, I'd like to know whether the dialog creation code needs somehow route part of that logic to DefDlgProc(), and if that's the case perhaps what is going on is that DefDlgProc() should be called as a part of dialog creation process.
I've added a test that shows that DefDlgProc is not involved here. If you still find it unconvincing, please let me know what other tests you'd like to see. -- Alexandre Julliard julliard(a)winehq.org
participants (3)
-
Alexandre Julliard -
Dmitry Timoshkov -
Piotr Caban