Alexandre Julliard julliard@winehq.org wrote:
Piotr Caban piotr@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.