[PATCH v2 1/8] shell32/autocomplete: Fix going up through the suggestion listbox
When going up past the topmost item in the listbox, go through txtbackup first before wrapping around, just like when going down. This matches Windows behavior. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This series supersedes patches from 150637 to 150639. dlls/shell32/autocomplete.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index e3fe1fd..0471146 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -296,7 +296,7 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT /* Change the selection */ sel = SendMessageW(ac->hwndListBox, LB_GETCURSEL, 0, 0); if (wParam == VK_UP) - sel = ((sel - 1) < 0) ? count - 1 : sel - 1; + sel = ((sel - 1) < -1) ? count - 1 : sel - 1; else sel = ((sel + 1) >= count) ? -1 : sel + 1; SendMessageW(ac->hwndListBox, LB_SETCURSEL, sel, 0); -- 1.9.1
The user can right-click on the editbox control and choose one of these operations, so they need to be handled separately regardless. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/shell32/autocomplete.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 0471146..40e0866 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -363,6 +363,16 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, autocomplete_text(This, hwnd, (This->options & ACO_AUTOAPPEND) && wParam >= ' ' ? autoappend_flag_yes : autoappend_flag_no); return ret; + case WM_CUT: + case WM_CLEAR: + case WM_UNDO: + ret = CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); + autocomplete_text(This, hwnd, autoappend_flag_no); + return ret; + case WM_PASTE: + ret = CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); + autocomplete_text(This, hwnd, autoappend_flag_yes); + return ret; case WM_DESTROY: { WNDPROC proc = This->wpOrigEditProc; -- 1.9.1
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Instead of using the default case to forward it, use the case outside of the switch for this purpose. It's a minor simplification now, but it will be very helpful in future patches (next series), when more messages will be handled. dlls/shell32/autocomplete.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 40e0866..ef835b9 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -348,13 +348,13 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, case CB_SHOWDROPDOWN: if (This->options & ACO_AUTOSUGGEST) ShowWindow(This->hwndListBox, SW_HIDE); - break; + return 0; case WM_KILLFOCUS: if ((This->options & ACO_AUTOSUGGEST) && ((HWND)wParam != This->hwndListBox)) { ShowWindow(This->hwndListBox, SW_HIDE); } - return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); + break; case WM_KEYDOWN: return ACEditSubclassProc_KeyDown(This, hwnd, uMsg, wParam, lParam); case WM_CHAR: @@ -382,11 +382,8 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, destroy_autocomplete_object(This); return CallWindowProcW(proc, hwnd, uMsg, wParam, lParam); } - default: - return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); } - - return 0; + return CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); } static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam) -- 1.9.1
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
Most control characters sent via some CTRL+key combination should not autocomplete at all. ^C is one example, where just copying some text should not show the auto-suggestion box (if not visible). ^V is another example, where it is already handled in WM_PASTE, so it has to be a no-op here, else auto-append from WM_PASTE would complete the text and then the ^V autocompletion would remove every other suggestion in the listbox (which is not desirable nor does it match Windows). Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Rather than check for ^C and ^V as special cases, I decided to ignore most control characters, with notable exceptions like Backspace and Tab characters (since they actually do something) and the newline stuff in that range. Unfortunately, I can't seem to find a better way. I realize this looks like a hack, but it doesn't seem like there's another way to me. ^C is obvious: there's nothing we can do in WM_COPY (even if we handled it, somehow) because the ^C will still be there waiting to be processed after it. ^V is less obvious. Right now, if you paste something via CTRL+V (instead of right-click -> Paste) and it gets auto-appended, the resulting suggestions will be removed because the autocomplete will happen *twice*. The first time in WM_PASTE (properly) and then in ^V when it will "see" the entire text because of the auto-append from WM_PASTE, and thus removes all the other suggestions. This should not happen. Pasting via right-click obviously does not have this problem, but it is the reason we have to do it in WM_PASTE as well, anyway. This makes ^V have the same behavior as Right-click + Paste. dlls/shell32/autocomplete.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index ef835b9..92afbe2 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -359,6 +359,10 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, return ACEditSubclassProc_KeyDown(This, hwnd, uMsg, wParam, lParam); case WM_CHAR: case WM_UNICHAR: + /* Don't autocomplete at all on most control characters */ + if (wParam < '\b' || (wParam > '\r' && wParam < ' ')) + break; + ret = CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); autocomplete_text(This, hwnd, (This->options & ACO_AUTOAPPEND) && wParam >= ' ' ? autoappend_flag_yes : autoappend_flag_no); -- 1.9.1
Maybe filter with iscntrlW() with exceptions?
On Thu, Sep 20, 2018 at 3:51 PM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Maybe filter with iscntrlW() with exceptions?
I only tried to avoid most of the CTRL+something control characters. I see iscntrlW deals with Unicode table (probably some unicode-specific control chars as well). So I think the current method has less implications, but that depends on my understanding of it. Do you think it's a better approach with iscntrlW? (and keep the range between \b and \r then as the exceptions?). Unfortunately it would still look like a semi-hack due to the exceptions which are needed, at least for backspace and maybe tab. Not sure if it's better honestly.
On Thu, Sep 20, 2018 at 06:54:28PM +0300, Gabriel Ivăncescu wrote:
On Thu, Sep 20, 2018 at 3:51 PM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote:
Maybe filter with iscntrlW() with exceptions?
Do you think it's a better approach with iscntrlW? (and keep the range between \b and \r then as the exceptions?). Unfortunately it would still look like a semi-hack due to the exceptions which are needed, at least for backspace and maybe tab. Not sure if it's better honestly.
Yes, I think iscntrlW() would be better. Huw.
On Fri, Sep 21, 2018 at 1:45 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Yes, I think iscntrlW() would be better.
Huw.
Something like this? iscntrlW(wParam) && !(wParam >= '\b' && wParam <= '\r') Or do you have other exceptions in mind?
On Fri, Sep 21, 2018 at 02:05:33PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 21, 2018 at 1:45 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Yes, I think iscntrlW() would be better.
Huw.
Something like this?
iscntrlW(wParam) && !(wParam >= '\b' && wParam <= '\r')
Or do you have other exceptions in mind?
How many exceptions (to iscntrlW()) do you actually need? Huw.
On Fri, Sep 21, 2018 at 2:32 PM, Huw Davies <huw(a)codeweavers.com> wrote:
How many exceptions (to iscntrlW()) do you actually need?
Huw.
Well Backspace for sure, maybe Tab (if it's inserted in an edit control, don't know if that's possible), same doubt about newlines ('\n' and '\r') or "vertical tab" (are those valid in certain edit controls?). I mean if anything '\b' has to be an exception, at the very least. The rest can be ignored I guess, if you want to simplify this to just one check for '\b' instead. Should I just check for backspace?
On Fri, Sep 21, 2018 at 02:39:12PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 21, 2018 at 2:32 PM, Huw Davies <huw(a)codeweavers.com> wrote:
How many exceptions (to iscntrlW()) do you actually need?
Huw.
Well Backspace for sure, maybe Tab (if it's inserted in an edit control, don't know if that's possible), same doubt about newlines ('\n' and '\r') or "vertical tab" (are those valid in certain edit controls?).
I mean if anything '\b' has to be an exception, at the very least. The rest can be ignored I guess, if you want to simplify this to just one check for '\b' instead. Should I just check for backspace?
Doesn't sound like it would be too hard to test (just send some WM_CHARs to the edit). Huw.
On Fri, Sep 21, 2018 at 2:44 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Doesn't sound like it would be too hard to test (just send some WM_CHARs to the edit).
Huw.
I mean, they don't work on our/Windows edit control implementation (might depend on styles, I don't know), but some apps might subclass the edit control before AutoComplete on them and make them useable. I was just wondering since tab / newline / carriage return actually do "something" to the text in question like backspace, so that's why I had doubts whether we should process them or not.
On Fri, Sep 21, 2018 at 02:50:45PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 21, 2018 at 2:44 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Doesn't sound like it would be too hard to test (just send some WM_CHARs to the edit).
Huw.
I mean, they don't work on our/Windows edit control implementation (might depend on styles, I don't know), but some apps might subclass the edit control before AutoComplete on them and make them useable. I was just wondering since tab / newline / carriage return actually do "something" to the text in question like backspace, so that's why I had doubts whether we should process them or not.
Multiline edit controls will take all of the above. Do they work with autocomplete? Huw.
On Fri, Sep 21, 2018 at 2:57 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Multiline edit controls will take all of the above. Do they work with autocomplete?
Huw.
If they handle the same messages, then they should, there's nothing (at least in our) code that specifically asks for a specific type of edit control. So maybe I should just keep it like my example, to be on the safe side.
On Fri, Sep 21, 2018 at 03:34:52PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 21, 2018 at 2:57 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Multiline edit controls will take all of the above. Do they work with autocomplete?
If they handle the same messages, then they should, there's nothing (at least in our) code that specifically asks for a specific type of edit control. So maybe I should just keep it like my example, to be on the safe side.
I was hoping that you might test it with a multiline edit ;-) Huw.
On Fri, Sep 21, 2018 at 3:40 PM, Huw Davies <huw(a)codeweavers.com> wrote:
On Fri, Sep 21, 2018 at 03:34:52PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 21, 2018 at 2:57 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Multiline edit controls will take all of the above. Do they work with autocomplete?
If they handle the same messages, then they should, there's nothing (at least in our) code that specifically asks for a specific type of edit control. So maybe I should just keep it like my example, to be on the safe side.
I was hoping that you might test it with a multiline edit ;-)
Huw.
Well as expected, with Wine's edit control it seems they do work indeed with autocomplete, in the sense that a tab character does end up printed on a multi-line edit control (same with newline of course) and then it does auto-suggest an entry with a tab, so autocomplete works. I couldn't get Windows XP to work yet to print a tab, but since our edit control already handles such characters like tab, I think it's better to allow it to autocomplete just in case. Or am I wrong?
Send the messages directly to the edit control's window procedure to match Windows behavior and reduce needless overhead. However, it appears that Windows allows hijacking WM_GETTEXT and WM_GETTEXTLENGTH for autocompletion, so handle that case separately. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- See the tests in the series for examples of that. dlls/shell32/autocomplete.c | 47 ++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 92afbe2..3326e31 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -90,6 +90,8 @@ static const WCHAR autocomplete_propertyW[] = {'W','i','n','e',' ','A','u','t',' 'c','o','m','p','l','e','t','e',' ', 'c','o','n','t','r','o','l',0}; +static LRESULT APIENTRY ACEditSubclassProc(HWND, UINT, WPARAM, LPARAM); + static inline IAutoCompleteImpl *impl_from_IAutoComplete2(IAutoComplete2 *iface) { return CONTAINING_RECORD(iface, IAutoCompleteImpl, IAutoComplete2_iface); @@ -100,6 +102,22 @@ static inline IAutoCompleteImpl *impl_from_IAutoCompleteDropDown(IAutoCompleteDr return CONTAINING_RECORD(iface, IAutoCompleteImpl, IAutoCompleteDropDown_iface); } +static inline WNDPROC get_edit_proc(IAutoCompleteImpl *ac, HWND hwnd) +{ + WNDPROC edit_proc = (WNDPROC)GetWindowLongPtrW(hwnd, GWLP_WNDPROC); + return (edit_proc == ACEditSubclassProc) ? ac->wpOrigEditProc : edit_proc; +} + +static void set_text_and_selection(IAutoCompleteImpl *ac, HWND hwnd, WCHAR *text, WPARAM start, LPARAM end) +{ + /* Send it directly to the edit control to match Windows XP behavior */ + WNDPROC proc = ac->wpOrigEditProc; + + /* FIXME: send WM_SETREDRAW when the edit control supports it */ + if (CallWindowProcW(proc, hwnd, WM_SETTEXT, 0, (LPARAM)text)) + CallWindowProcW(proc, hwnd, EM_SETSEL, start, end); +} + static size_t format_quick_complete(WCHAR *dst, const WCHAR *qc, const WCHAR *str, size_t str_len) { /* Replace the first %s directly without using snprintf, to avoid @@ -142,8 +160,7 @@ static void autoappend_str(IAutoCompleteImpl *ac, WCHAR *text, UINT len, WCHAR * } else tmp = str; - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)tmp); - SendMessageW(hwnd, EM_SETSEL, len, size - 1); + set_text_and_selection(ac, hwnd, tmp, len, size - 1); if (tmp != str) heap_free(tmp); } @@ -152,7 +169,8 @@ static void autocomplete_text(IAutoCompleteImpl *ac, HWND hwnd, enum autoappend_ { HRESULT hr; WCHAR *text; - UINT cpt, size, len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0); + WNDPROC edit_proc = get_edit_proc(ac, hwnd); + UINT cpt, size, len = CallWindowProcW(edit_proc, hwnd, WM_GETTEXTLENGTH, 0, 0); if (flag != autoappend_flag_displayempty && len == 0) { @@ -164,7 +182,7 @@ static void autocomplete_text(IAutoCompleteImpl *ac, HWND hwnd, enum autoappend_ size = len + 1; if (!(text = heap_alloc(size * sizeof(WCHAR)))) return; - len = SendMessageW(hwnd, WM_GETTEXT, size, (LPARAM)text); + len = CallWindowProcW(edit_proc, hwnd, WM_GETTEXT, size, (LPARAM)text); if (len + 1 != size) text = heap_realloc(text, (len + 1) * sizeof(WCHAR)); @@ -247,17 +265,18 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT { WCHAR *text, *buf; size_t sz; - UINT len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0); + WNDPROC edit_proc = get_edit_proc(ac, hwnd); + UINT len = CallWindowProcW(edit_proc, hwnd, WM_GETTEXTLENGTH, 0, 0); + if (!(text = heap_alloc((len + 1) * sizeof(WCHAR)))) return 0; - len = SendMessageW(hwnd, WM_GETTEXT, len + 1, (LPARAM)text); + len = CallWindowProcW(edit_proc, hwnd, WM_GETTEXT, len + 1, (LPARAM)text); sz = strlenW(ac->quickComplete) + 1 + len; if ((buf = heap_alloc(sz * sizeof(WCHAR)))) { len = format_quick_complete(buf, ac->quickComplete, text, len); - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)buf); - SendMessageW(hwnd, EM_SETSEL, 0, len); + set_text_and_selection(ac, hwnd, buf, 0, len); heap_free(buf); } @@ -309,16 +328,13 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT if (!(msg = heap_alloc((len + 1) * sizeof(WCHAR)))) return 0; len = SendMessageW(ac->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg); - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)msg); - SendMessageW(hwnd, EM_SETSEL, len, len); + set_text_and_selection(ac, hwnd, msg, len, len); heap_free(msg); } else { - UINT len; - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)ac->txtbackup); - len = strlenW(ac->txtbackup); - SendMessageW(hwnd, EM_SETSEL, len, len); + UINT len = strlenW(ac->txtbackup); + set_text_and_selection(ac, hwnd, ac->txtbackup, len, len); } return 0; } @@ -409,8 +425,7 @@ static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, if (!(msg = heap_alloc((len + 1) * sizeof(WCHAR)))) break; len = SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg); - SendMessageW(This->hwndEdit, WM_SETTEXT, 0, (LPARAM)msg); - SendMessageW(This->hwndEdit, EM_SETSEL, 0, len); + set_text_and_selection(This, This->hwndEdit, msg, 0, len); ShowWindow(hwnd, SW_HIDE); heap_free(msg); break; -- 1.9.1
On Thu, Sep 20, 2018 at 02:55:38PM +0300, Gabriel Ivăncescu wrote:
Send the messages directly to the edit control's window procedure to match Windows behavior and reduce needless overhead. However, it appears that Windows allows hijacking WM_GETTEXT and WM_GETTEXTLENGTH for autocompletion, so handle that case separately.
Do you have an application that depends on this? If not, we should leave it as it is. Huw.
On Fri, Sep 21, 2018 at 1:55 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Do you have an application that depends on this? If not, we should leave it as it is.
Huw.
Not that I'm aware of, but I thought matching Windows is a good idea here. I should probably still send it directly to the control though since it reduces needless overhead (especially due to GetPropW), but then the code can be simplified a bit if I don't have to special-case the WM_GETTEXT. The other hijack case needs to remain else I can't add WM_SETTEXT to the messages handled later since it would result in a recursive loop. That said, It's a single extra call (get_edit_proc here), I don't think it's so bad to keep it like this since it's still better (behavior & overhead).
On Fri, Sep 21, 2018 at 02:07:11PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 21, 2018 at 1:55 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Do you have an application that depends on this? If not, we should leave it as it is.
Huw.
Not that I'm aware of, but I thought matching Windows is a good idea here. I should probably still send it directly to the control though since it reduces needless overhead (especially due to GetPropW), but then the code can be simplified a bit if I don't have to special-case the WM_GETTEXT. The other hijack case needs to remain else I can't add WM_SETTEXT to the messages handled later since it would result in a recursive loop.
That said, It's a single extra call (get_edit_proc here), I don't think it's so bad to keep it like this since it's still better (behavior & overhead).
Well firstly, I very much doubt the overhead is going to be relevant for a user input driven auto-complete. We don't necessarily want to copy Windows' implementation exactly--- indeed if it's possible we try to avoid that. As long as apps don't notice the difference it's fine. Just make the changes that you need to add WM_SETTEXT. Huw.
On Fri, Sep 21, 2018 at 2:38 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Well firstly, I very much doubt the overhead is going to be relevant for a user input driven auto-complete.
We don't necessarily want to copy Windows' implementation exactly--- indeed if it's possible we try to avoid that. As long as apps don't notice the difference it's fine.
Just make the changes that you need to add WM_SETTEXT.
Huw.
I think we should avoid wineserver calls where possible, since it disrupts the wineserver also not just the specific application in question (in terms of overhead / blocking). That said, if you don't want Windows behavior, I can get rid of get_edit_proc totally, and just replace SendMessage with CallWindowProc directly, which would still avoid wineserver calls here, and not add anything to the code than just replace the calls. Well of course unless you really want to leave them as SendMessage but I think it's better to avoid wineserver calls if we can.
On Fri, Sep 21, 2018 at 02:45:28PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 21, 2018 at 2:38 PM, Huw Davies <huw(a)codeweavers.com> wrote:
Well firstly, I very much doubt the overhead is going to be relevant for a user input driven auto-complete.
We don't necessarily want to copy Windows' implementation exactly--- indeed if it's possible we try to avoid that. As long as apps don't notice the difference it's fine.
Just make the changes that you need to add WM_SETTEXT.
Huw.
I think we should avoid wineserver calls where possible, since it disrupts the wineserver also not just the specific application in question (in terms of overhead / blocking).
That said, if you don't want Windows behavior, I can get rid of get_edit_proc totally, and just replace SendMessage with CallWindowProc directly, which would still avoid wineserver calls here, and not add anything to the code than just replace the calls.
Well of course unless you really want to leave them as SendMessage but I think it's better to avoid wineserver calls if we can.
As I hinted at above, unless you can type very much faster than me then I don't consider this much of an issue. Huw.
On Fri, Sep 21, 2018 at 3:02 PM, Huw Davies <huw(a)codeweavers.com> wrote:
As I hinted at above, unless you can type very much faster than me then I don't consider this much of an issue.
Huw.
Well it's not much of an issue but it's still a slight improvement for the wineserver to have to process less in its queue, and all it needs is change a few lines (4 in total) from SendMessage to CallWindowProc. Are you sure you don't want me to do that?
On Fri, Sep 21, 2018 at 03:38:39PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 21, 2018 at 3:02 PM, Huw Davies <huw(a)codeweavers.com> wrote:
As I hinted at above, unless you can type very much faster than me then I don't consider this much of an issue.
Well it's not much of an issue but it's still a slight improvement for the wineserver to have to process less in its queue, and all it needs is change a few lines (4 in total) from SendMessage to CallWindowProc.
Are you sure you don't want me to do that?
Yes. It may only be 4 lines, but it's a reasonably fundamental change to how things work which will require time to review and add to the possibility of regressions. So if it's not actually improving things in a noticable manner then it should stay out. Huw.
Huw Davies <huw(a)codeweavers.com> writes:
On Fri, Sep 21, 2018 at 03:38:39PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 21, 2018 at 3:02 PM, Huw Davies <huw(a)codeweavers.com> wrote:
As I hinted at above, unless you can type very much faster than me then I don't consider this much of an issue.
Well it's not much of an issue but it's still a slight improvement for the wineserver to have to process less in its queue, and all it needs is change a few lines (4 in total) from SendMessage to CallWindowProc.
Are you sure you don't want me to do that?
Yes.
It may only be 4 lines, but it's a reasonably fundamental change to how things work which will require time to review and add to the possibility of regressions. So if it's not actually improving things in a noticable manner then it should stay out.
Not to mention that SendMessage doesn't do a wineserver call for the thread-local case, so it's not even improving anything. -- Alexandre Julliard julliard(a)winehq.org
On Fri, Sep 21, 2018 at 4:57 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Huw Davies <huw(a)codeweavers.com> writes:
On Fri, Sep 21, 2018 at 03:38:39PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 21, 2018 at 3:02 PM, Huw Davies <huw(a)codeweavers.com> wrote:
As I hinted at above, unless you can type very much faster than me then I don't consider this much of an issue.
Well it's not much of an issue but it's still a slight improvement for the wineserver to have to process less in its queue, and all it needs is change a few lines (4 in total) from SendMessage to CallWindowProc.
Are you sure you don't want me to do that?
Yes.
It may only be 4 lines, but it's a reasonably fundamental change to how things work which will require time to review and add to the possibility of regressions. So if it's not actually improving things in a noticable manner then it should stay out.
Not to mention that SendMessage doesn't do a wineserver call for the thread-local case, so it's not even improving anything.
-- Alexandre Julliard julliard(a)winehq.org
Yeah but it's not the SendMessage that's the wineserver call I'm talking about, but the fact that it will go through this subclassed window procedure again (it starts at the top), which ends up calling GetPropW to get the This pointer just to forward it to the edit control. As I see, GetPropW uses a wineserver call. In contrast, CallWindowProc on the This->wpOrigEditProc would obviously bypass this (which is what the original would end up doing anyway, since we don't handle WM_GETTEXT and such in our subclassed proc).
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
On Fri, Sep 21, 2018 at 4:57 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Huw Davies <huw(a)codeweavers.com> writes:
On Fri, Sep 21, 2018 at 03:38:39PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 21, 2018 at 3:02 PM, Huw Davies <huw(a)codeweavers.com> wrote:
As I hinted at above, unless you can type very much faster than me then I don't consider this much of an issue.
Well it's not much of an issue but it's still a slight improvement for the wineserver to have to process less in its queue, and all it needs is change a few lines (4 in total) from SendMessage to CallWindowProc.
Are you sure you don't want me to do that?
Yes.
It may only be 4 lines, but it's a reasonably fundamental change to how things work which will require time to review and add to the possibility of regressions. So if it's not actually improving things in a noticable manner then it should stay out.
Not to mention that SendMessage doesn't do a wineserver call for the thread-local case, so it's not even improving anything.
-- Alexandre Julliard julliard(a)winehq.org
Yeah but it's not the SendMessage that's the wineserver call I'm talking about, but the fact that it will go through this subclassed window procedure again (it starts at the top), which ends up calling GetPropW to get the This pointer just to forward it to the edit control. As I see, GetPropW uses a wineserver call.
If it turns out that GetPropW is a bottleneck (very unlikely), it can be optimized in various ways. Please don't obfuscate the code in an attempt to make things faster where it doesn't matter. -- Alexandre Julliard julliard(a)winehq.org
On Fri, Sep 21, 2018 at 5:33 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
If it turns out that GetPropW is a bottleneck (very unlikely), it can be optimized in various ways. Please don't obfuscate the code in an attempt to make things faster where it doesn't matter.
-- Alexandre Julliard julliard(a)winehq.org
Well of course I wouldn't have messed with GetPropW, just replace SendMessage with calling the edit control's procedure directly (because ultimately that's what we ask it, for its data). I personally don't think that's such an obfuscation or actually an obfuscation at all, since it's crystal clear still to what the purpose is (and personally, to me, makes more sense to ask the edit control directly). However, I'll keep it as SendMessage then, unless you change your mind :-) I was just clarifying my reasoning before in respect to where the wineserver call was. A little bonus I guess is that it will also work again with hijacking just like on Windows.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Split from previous patch, reduces needless overhead. dlls/shell32/autocomplete.c | 41 +++++++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 3326e31..d03feb5 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -108,6 +108,11 @@ static inline WNDPROC get_edit_proc(IAutoCompleteImpl *ac, HWND hwnd) return (edit_proc == ACEditSubclassProc) ? ac->wpOrigEditProc : edit_proc; } +static inline LRESULT send_to_LB(IAutoCompleteImpl *ac, HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) +{ + return CallWindowProcW(ac->wpOrigLBoxProc, hwnd, msg, wParam, lParam); +} + static void set_text_and_selection(IAutoCompleteImpl *ac, HWND hwnd, WCHAR *text, WPARAM start, LPARAM end) { /* Send it directly to the edit control to match Windows XP behavior */ @@ -186,7 +191,7 @@ static void autocomplete_text(IAutoCompleteImpl *ac, HWND hwnd, enum autoappend_ if (len + 1 != size) text = heap_realloc(text, (len + 1) * sizeof(WCHAR)); - SendMessageW(ac->hwndListBox, LB_RESETCONTENT, 0, 0); + send_to_LB(ac, ac->hwndListBox, LB_RESETCONTENT, 0, 0); /* Set txtbackup to point to text itself (which must not be released) */ heap_free(ac->txtbackup); @@ -215,7 +220,7 @@ static void autocomplete_text(IAutoCompleteImpl *ac, HWND hwnd, enum autoappend_ } if (ac->options & ACO_AUTOSUGGEST) - SendMessageW(ac->hwndListBox, LB_ADDSTRING, 0, (LPARAM)strs); + send_to_LB(ac, ac->hwndListBox, LB_ADDSTRING, 0, (LPARAM)strs); cpt++; } @@ -228,8 +233,8 @@ static void autocomplete_text(IAutoCompleteImpl *ac, HWND hwnd, enum autoappend_ if (cpt) { RECT r; - UINT height = SendMessageW(ac->hwndListBox, LB_GETITEMHEIGHT, 0, 0); - SendMessageW(ac->hwndListBox, LB_CARETOFF, 0, 0); + UINT height = send_to_LB(ac, ac->hwndListBox, LB_GETITEMHEIGHT, 0, 0); + send_to_LB(ac, ac->hwndListBox, LB_CARETOFF, 0, 0); GetWindowRect(hwnd, &r); SetParent(ac->hwndListBox, HWND_DESKTOP); /* It seems that Windows XP displays 7 lines at most @@ -310,24 +315,24 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT else { INT count, sel; - count = SendMessageW(ac->hwndListBox, LB_GETCOUNT, 0, 0); + count = send_to_LB(ac, ac->hwndListBox, LB_GETCOUNT, 0, 0); /* Change the selection */ - sel = SendMessageW(ac->hwndListBox, LB_GETCURSEL, 0, 0); + sel = send_to_LB(ac, ac->hwndListBox, LB_GETCURSEL, 0, 0); if (wParam == VK_UP) sel = ((sel - 1) < -1) ? count - 1 : sel - 1; else sel = ((sel + 1) >= count) ? -1 : sel + 1; - SendMessageW(ac->hwndListBox, LB_SETCURSEL, sel, 0); + send_to_LB(ac, ac->hwndListBox, LB_SETCURSEL, sel, 0); if (sel >= 0) { WCHAR *msg; UINT len; - len = SendMessageW(ac->hwndListBox, LB_GETTEXTLEN, sel, 0); + len = send_to_LB(ac, ac->hwndListBox, LB_GETTEXTLEN, sel, 0); if (!(msg = heap_alloc((len + 1) * sizeof(WCHAR)))) return 0; - len = SendMessageW(ac->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg); + len = send_to_LB(ac, ac->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg); set_text_and_selection(ac, hwnd, msg, len, len); heap_free(msg); } @@ -414,23 +419,23 @@ static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, switch (uMsg) { case WM_MOUSEMOVE: - sel = SendMessageW(hwnd, LB_ITEMFROMPOINT, 0, lParam); - SendMessageW(hwnd, LB_SETCURSEL, sel, 0); + sel = send_to_LB(This, hwnd, LB_ITEMFROMPOINT, 0, lParam); + send_to_LB(This, hwnd, LB_SETCURSEL, sel, 0); break; case WM_LBUTTONDOWN: - sel = SendMessageW(hwnd, LB_GETCURSEL, 0, 0); + sel = send_to_LB(This, hwnd, LB_GETCURSEL, 0, 0); if (sel < 0) break; - len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); + len = send_to_LB(This, hwnd, LB_GETTEXTLEN, sel, 0); if (!(msg = heap_alloc((len + 1) * sizeof(WCHAR)))) break; - len = SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg); + len = send_to_LB(This, hwnd, LB_GETTEXT, sel, (LPARAM)msg); set_text_and_selection(This, This->hwndEdit, msg, 0, len); ShowWindow(hwnd, SW_HIDE); heap_free(msg); break; default: - return CallWindowProcW(This->wpOrigLBoxProc, hwnd, uMsg, wParam, lParam); + return send_to_LB(This, hwnd, uMsg, wParam, lParam); } return 0; } @@ -749,14 +754,14 @@ static HRESULT WINAPI IAutoCompleteDropDown_fnGetDropDownStatus( if (dropped) { int sel; - sel = SendMessageW(This->hwndListBox, LB_GETCURSEL, 0, 0); + sel = send_to_LB(This, This->hwndListBox, LB_GETCURSEL, 0, 0); if (sel >= 0) { DWORD len; - len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0); + len = send_to_LB(This, This->hwndListBox, LB_GETTEXTLEN, sel, 0); *ppwszString = CoTaskMemAlloc((len+1)*sizeof(WCHAR)); - SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)*ppwszString); + send_to_LB(This, This->hwndListBox, LB_GETTEXT, sel, (LPARAM)*ppwszString); } else *ppwszString = NULL; -- 1.9.1
Windows' AutoComplete seems to bypass hijacking in some cases. However, WM_GETTEXT seems to be able to get hijacked. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Also reduced the Sleep amount to 1/3 of its original, which will be especially useful in future tests (to be added) to prevent them from taking way too long. dlls/shell32/tests/autocomplete.c | 82 +++++++++++++++++++++++++++++++++++---- 1 file changed, 75 insertions(+), 7 deletions(-) diff --git a/dlls/shell32/tests/autocomplete.c b/dlls/shell32/tests/autocomplete.c index b9c6374..e85e30c 100644 --- a/dlls/shell32/tests/autocomplete.c +++ b/dlls/shell32/tests/autocomplete.c @@ -236,6 +236,36 @@ static void createMainWnd(void) CW_USEDEFAULT, CW_USEDEFAULT, 130, 105, NULL, NULL, GetModuleHandleA(NULL), 0); } +static WNDPROC HijackerWndProc_prev; +static const WCHAR HijackerWndProc_txt[] = {'H','i','j','a','c','k','e','d',0}; +static LRESULT CALLBACK HijackerWndProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam) +{ + switch(msg) { + case WM_GETTEXT: + { + size_t len = min(wParam, ARRAY_SIZE(HijackerWndProc_txt)); + memcpy((void*)lParam, HijackerWndProc_txt, len * sizeof(WCHAR)); + return len; + } + case WM_GETTEXTLENGTH: + return ARRAY_SIZE(HijackerWndProc_txt) - 1; + } + return CallWindowProcW(HijackerWndProc_prev, hWnd, msg, wParam, lParam); +} + +static LRESULT CALLBACK HijackerWndProc2(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam) +{ + switch(msg) { + case EM_SETSEL: + lParam = wParam; + break; + case WM_SETTEXT: + lParam = (LPARAM)HijackerWndProc_txt; + break; + } + return CallWindowProcW(HijackerWndProc_prev, hWnd, msg, wParam, lParam); +} + struct string_enumerator { IEnumString IEnumString_iface; @@ -358,18 +388,29 @@ static HRESULT string_enumerator_create(void **ppv, WCHAR **suggestions, int cou return S_OK; } +static void dispatch_messages(void) +{ + MSG msg; + Sleep(33); + while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + } +} + static void test_custom_source(void) { static WCHAR str_alpha[] = {'t','e','s','t','1',0}; static WCHAR str_alpha2[] = {'t','e','s','t','2',0}; static WCHAR str_beta[] = {'a','u','t','o',' ','c','o','m','p','l','e','t','e',0}; + static WCHAR str_au[] = {'a','u',0}; static WCHAR *suggestions[] = { str_alpha, str_alpha2, str_beta }; IUnknown *enumerator; IAutoComplete2 *autocomplete; HWND hwnd_edit; WCHAR buffer[20]; HRESULT hr; - MSG msg; ShowWindow(hMainWnd, SW_SHOW); @@ -385,16 +426,43 @@ static void test_custom_source(void) hr = IAutoComplete2_Init(autocomplete, hwnd_edit, enumerator, NULL, NULL); ok(hr == S_OK, "IAutoComplete_Init failed: %x\n", hr); + SetFocus(hwnd_edit); SendMessageW(hwnd_edit, WM_CHAR, 'a', 1); SendMessageW(hwnd_edit, WM_CHAR, 'u', 1); - Sleep(100); - while (PeekMessageA(&msg, 0, 0, 0, PM_REMOVE)) - { - TranslateMessage(&msg); - DispatchMessageA(&msg); - } + dispatch_messages(); + SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer); + ok(lstrcmpW(str_beta, buffer) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str_beta), wine_dbgstr_w(buffer)); + SendMessageW(hwnd_edit, EM_SETSEL, 0, -1); + SendMessageW(hwnd_edit, WM_CHAR, '\b', 1); + dispatch_messages(); + SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer); + ok(buffer[0] == '\0', "Expected empty string, got %s\n", wine_dbgstr_w(buffer)); + + /* hijack the window procedure */ + HijackerWndProc_prev = (WNDPROC)SetWindowLongPtrW(hwnd_edit, GWLP_WNDPROC, (LONG_PTR)HijackerWndProc); + SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer); + ok(lstrcmpW(HijackerWndProc_txt, buffer) == 0, "Expected %s, got %s\n", wine_dbgstr_w(HijackerWndProc_txt), wine_dbgstr_w(buffer)); + + SendMessageW(hwnd_edit, WM_CHAR, 'a', 1); + SendMessageW(hwnd_edit, WM_CHAR, 'u', 1); + SetWindowLongPtrW(hwnd_edit, GWLP_WNDPROC, (LONG_PTR)HijackerWndProc_prev); + dispatch_messages(); + SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer); + ok(lstrcmpW(str_au, buffer) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str_au), wine_dbgstr_w(buffer)); + SendMessageW(hwnd_edit, EM_SETSEL, 0, -1); + SendMessageW(hwnd_edit, WM_CHAR, '\b', 1); + dispatch_messages(); + SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer); + ok(buffer[0] == '\0', "Expected empty string, got %s\n", wine_dbgstr_w(buffer)); + + HijackerWndProc_prev = (WNDPROC)SetWindowLongPtrW(hwnd_edit, GWLP_WNDPROC, (LONG_PTR)HijackerWndProc2); + SendMessageW(hwnd_edit, WM_CHAR, 'a', 1); + SendMessageW(hwnd_edit, WM_CHAR, 'u', 1); + SetWindowLongPtrW(hwnd_edit, GWLP_WNDPROC, (LONG_PTR)HijackerWndProc_prev); + dispatch_messages(); SendMessageW(hwnd_edit, WM_GETTEXT, ARRAY_SIZE(buffer), (LPARAM)buffer); ok(lstrcmpW(str_beta, buffer) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str_beta), wine_dbgstr_w(buffer)); + /* end of hijacks */ ShowWindow(hMainWnd, SW_HIDE); DestroyWindow(hwnd_edit); -- 1.9.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This should work now that we send it directly to the edit control's procedure. dlls/shell32/autocomplete.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index d03feb5..23ff59a 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -388,6 +388,7 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, autocomplete_text(This, hwnd, (This->options & ACO_AUTOAPPEND) && wParam >= ' ' ? autoappend_flag_yes : autoappend_flag_no); return ret; + case WM_SETTEXT: case WM_CUT: case WM_CLEAR: case WM_UNDO: -- 1.9.1
Sorry, I messed up the component, this should NOT be shell32/tests but shell32/autocomplete.
participants (4)
-
Alexandre Julliard -
Gabriel Ivăncescu -
Huw Davies -
Nikolay Sivov