Re: [PATCH] user32: Handle VK_RETURN KeyDown events more properly in EDITcontrols.
"El." <elptr(a)users.sourceforge.net> wrote:
@@ -4606,6 +4606,10 @@ static LRESULT EDIT_WM_KeyDown(EDITSTATE *es, INT key) { HWND hwndParent = GetParent(es->hwndSelf); DWORD dw = SendMessageW( hwndParent, DM_GETDEFID, 0, 0 ); + + if (GetClassLongW(hwndParent, GCW_ATOM) != WC_DIALOG) + break;
You need to add a test case first to confirm that the fix is correct, and use EDIT_IsInsideDialog to detect if the parent is dialog.
Well I'm not sure what to test in the first place, I didn't assume that modifying some aspect of the code to be in compliance with official "standards" required writing any test case. I might be missing what you mean by the "correctness" of the fix, since I'm not really familiar with Wine's patching practices. http://msdn2.microsoft.com/en-us/library/bb775464(VS.85).aspx As for using EDIT_IsInsideDialog, that I can manage.
On 19/04/2008, El. <eth.bell(a)gmail.com> wrote:
"El." <elptr(a)users.sourceforge.net> wrote:
@@ -4606,6 +4606,10 @@ static LRESULT EDIT_WM_KeyDown(EDITSTATE *es, INT key) { HWND hwndParent = GetParent(es->hwndSelf); DWORD dw = SendMessageW( hwndParent, DM_GETDEFID, 0, 0 ); + + if (GetClassLongW(hwndParent, GCW_ATOM) != WC_DIALOG) + break;
You need to add a test case first to confirm that the fix is correct, and use EDIT_IsInsideDialog to detect if the parent is dialog.
Well I'm not sure what to test in the first place, I didn't assume that modifying some aspect of the code to be in compliance with official "standards" required writing any test case.
I might be missing what you mean by the "correctness" of the fix, since I'm not really familiar with Wine's patching practices.
http://msdn2.microsoft.com/en-us/library/bb775464(VS.85).aspx
The MSDN documentation is a good source of information for how various controls and APIs behave. However, this is only a starting point. The aim of Wine is to match the behaviour of Windows, not the documented behaviour. While the patch is likely to be correct, this is not always the case. The test case will be run on (and verified against) the different versions of Windows. This will also test other things not explicitly documented: what is the return value; is GetLastError set; what is the sequence of messages sent in each case? All of these will add weight to your fix. The tests also serve another purpose. They allow the developers to refactor, rewrite or address other issues in the code and have a greater confidence that they haven't broken anything. If a bug is reported and then fixed, if there isn't a valid test case for that bug, there is a chance that the bug will reappear later on. When you have documentation (such as MSDN) that describes behaviour that is not supported in Wine, or differs in some way, the approach should be to write tests to validate the documentation and then implement it. That way, you have the verification that what you have written is correct. Thank you for improving Wine, - Reece
On 19/04/2008, El. <eth.bell(a)gmail.com> wrote:
You need to add a test case first to confirm that the fix is correct, and use EDIT_IsInsideDialog to detect if the parent is dialog.
Well I'm not sure what to test in the first place, I didn't assume that modifying some aspect of the code to be in compliance with official "standards" required writing any test case.
I might be missing what you mean by the "correctness" of the fix, since I'm not really familiar with Wine's patching practices.
http://msdn2.microsoft.com/en-us/library/bb775464(VS.85).aspx
Reading the MSDN documentation for the behaviour of ES_MULTILINE, it says "When the multiline edit control is in a dialog box, the default response to pressing the ENTER key is to activate the default button. To use the ENTER key as a carriage return, use the ES_WANTRETURN style." Therefore, I think your patch is incomplete, especially the part about the ES_WANTRETURN style. As such, you will need tests to verify this. In response to the enter key there are three behaviours: 1. the default push button is not activated; the edit contents is unchanged. 2. the default push button is activated; the edit contents is unchanged. 3. the default push button is not activated; the edit contents has a new line inserted at the current cursor position. You have the following configurations with their associated behaviours: * an edit control in a dialog without ES_MULTILINE and without ES_WANTRETURN: behaviour (2). * an edit control in a dialog with ES_MULTILINE and without ES_WANTRETURN: behaviour (2). * an edit control in a dialog with ES_MULTILINE and with ES_WANTRETURN: behaviour (3). * an edit control not in a dialog without ES_MULTILINE and without ES_WANTRETURN: behaviour (1). * an edit control not in a dialog with ES_MULTILINE and without ES_WANTRETURN: behaviour (3). * an edit control not in a dialog with ES_MULTILINE and with ES_WANTRETURN: behaviour (3). This is how I interpret that part of the MSDN documentation. This gives you the test cases needed to verify that your fix works in all these possibilities. It also ensures that someone wanting to improve the edit control does not regress this functionality, or breaks something else in the process. Thank you for improving Wine, - Reece
On Sat, Apr 19, 2008 at 4:05 AM, Reece Dunn <msclrhd(a)googlemail.com> wrote:
Reading the MSDN documentation for the behaviour of ES_MULTILINE, it says "When the multiline edit control is in a dialog box, the default response to pressing the ENTER key is to activate the default button. To use the ENTER key as a carriage return, use the ES_WANTRETURN style."
Therefore, I think your patch is incomplete, especially the part about the ES_WANTRETURN style. As such, you will need tests to verify this.
In response to the enter key there are three behaviours: 1. the default push button is not activated; the edit contents is unchanged. 2. the default push button is activated; the edit contents is unchanged. 3. the default push button is not activated; the edit contents has a new line inserted at the current cursor position.
You have the following configurations with their associated behaviours: * an edit control in a dialog without ES_MULTILINE and without ES_WANTRETURN: behaviour (2). * an edit control in a dialog with ES_MULTILINE and without ES_WANTRETURN: behaviour (2). * an edit control in a dialog with ES_MULTILINE and with ES_WANTRETURN: behaviour (3). * an edit control not in a dialog without ES_MULTILINE and without ES_WANTRETURN: behaviour (1). * an edit control not in a dialog with ES_MULTILINE and without ES_WANTRETURN: behaviour (3). * an edit control not in a dialog with ES_MULTILINE and with ES_WANTRETURN: behaviour (3).
This is how I interpret that part of the MSDN documentation. This gives you the test cases needed to verify that your fix works in all these possibilities. It also ensures that someone wanting to improve the edit control does not regress this functionality, or breaks something else in the process.
Edit controls get used in tons of applications, and its behavior can be tricky to get right. Especially in the case of ES_MULTILINE without ES_WANTRETURN. In the last two releases, I have managed to break the edit control several times, and the thing that really helped me figure out the proper fix is to write lots of test cases to cover as many usage scenarios as possible. Of course, writing tests is time consuming, and it's not always possible to cover all the usage cases. With the native notepad bug, it's because I didn't test the behavior when not inside a dialog box well enough. I don't know if Reece's interpretation of MSDN is 100% correct either, so the best thing to do is to write tests. With regards to the patch, if EDIT_IsInsideDialog returns false, then you don't want to send DM_GETDEFID either.
participants (3)
-
El. -
Lei Zhang -
Reece Dunn