[PATCH v3 1/6] shell32/autocomplete: Use the same font for the AutoComplete listbox as the edit control
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This looks much nicer and less out of place. v3: Use SendMessage. dlls/shell32/autocomplete.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index fe5bba2..9acd3f4 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -384,6 +384,10 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, ret = CallWindowProcW(This->wpOrigEditProc, hwnd, uMsg, wParam, lParam); autocomplete_text(This, hwnd, autoappend_flag_yes); return ret; + case WM_SETFONT: + if (This->hwndListBox) + SendMessageW(This->hwndListBox, WM_SETFONT, wParam, lParam); + break; case WM_DESTROY: { WNDPROC proc = This->wpOrigEditProc; @@ -434,9 +438,16 @@ static void create_listbox(IAutoCompleteImpl *This) 0, 0, 0, 0, GetParent(This->hwndEdit), NULL, shell32_hInstance, NULL); if (This->hwndListBox) { + HFONT edit_font; + This->wpOrigLBoxProc = (WNDPROC) SetWindowLongPtrW( This->hwndListBox, GWLP_WNDPROC, (LONG_PTR) ACLBoxSubclassProc); SetWindowLongPtrW( This->hwndListBox, GWLP_USERDATA, (LONG_PTR)This); SetParent(This->hwndListBox, HWND_DESKTOP); + + /* Use the same font as the edit control, as it gets destroyed before it anyway */ + edit_font = (HFONT)SendMessageW(This->hwndEdit, WM_GETFONT, 0, 0); + if (edit_font) + SendMessageW(This->hwndListBox, WM_SETFONT, (WPARAM)edit_font, FALSE); } else This->options &= ~ACO_AUTOSUGGEST; -- 1.9.1
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- This gets rid of annoying caret movement when replacing a selection that's not at the end (so it matches Windows). dlls/shell32/autocomplete.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 9acd3f4..621c5fd 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -136,9 +136,15 @@ static size_t format_quick_complete(WCHAR *dst, const WCHAR *qc, const WCHAR *st static void autoappend_str(IAutoCompleteImpl *ac, WCHAR *text, UINT len, WCHAR *str, HWND hwnd) { + DWORD sel_start; WCHAR *tmp; size_t size; + /* Don't auto-append unless the caret is at the end */ + SendMessageW(hwnd, EM_GETSEL, (WPARAM)&sel_start, 0); + if (sel_start != len) + return; + /* The character capitalization can be different, so merge text and str into a new string */ size = len + strlenW(&str[len]) + 1; -- 1.9.1
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- It's meant to be the same as on Windows; these keys always go to the visible bottom or top of the list, unless the selection is already there in which case they scroll by one page. However, when wrapping around, they go through txtbackup just like the arrow keys. The logic for the arrow keys has not been changed. PageDown/PageUp also do work with ACO_UPDOWNKEYDROPSLIST on Windows (they show the listbox). dlls/shell32/autocomplete.c | 93 ++++++++++++++++++++++++++++++--------------- 1 file changed, 63 insertions(+), 30 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 621c5fd..5cf63fe 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -134,6 +134,66 @@ static size_t format_quick_complete(WCHAR *dst, const WCHAR *qc, const WCHAR *st return dst - base; } +static LRESULT change_selection(IAutoCompleteImpl *ac, HWND hwnd, UINT key) +{ + INT count = SendMessageW(ac->hwndListBox, LB_GETCOUNT, 0, 0); + INT sel = SendMessageW(ac->hwndListBox, LB_GETCURSEL, 0, 0); + if (key == VK_PRIOR || key == VK_NEXT) + { + if (sel < 0) + sel = (key == VK_PRIOR) ? count - 1 : 0; + else + { + INT base = SendMessageW(ac->hwndListBox, LB_GETTOPINDEX, 0, 0); + INT pgsz = SendMessageW(ac->hwndListBox, LB_GETLISTBOXINFO, 0, 0); + pgsz = max(pgsz - 1, 1); + if (key == VK_PRIOR) + { + if (sel == 0) + sel = -1; + else + { + if (sel == base) base -= min(base, pgsz); + sel = base; + } + } + else + { + if (sel == count - 1) + sel = -1; + else + { + base += pgsz; + if (sel >= base) base += pgsz; + sel = min(base, count - 1); + } + } + } + } + else if (key == 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); + if (sel >= 0) + { + WCHAR *msg; + UINT len = SendMessageW(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); + set_text_and_selection(ac, hwnd, msg, len, len); + heap_free(msg); + } + else + { + UINT len = strlenW(ac->txtbackup); + set_text_and_selection(ac, hwnd, ac->txtbackup, len, len); + } + return 0; +} + static void autoappend_str(IAutoCompleteImpl *ac, WCHAR *text, UINT len, WCHAR *str, HWND hwnd) { DWORD sel_start; @@ -287,6 +347,8 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT break; case VK_UP: case VK_DOWN: + case VK_PRIOR: + case VK_NEXT: /* Two cases here: - if the listbox is not visible and ACO_UPDOWNKEYDROPSLIST is set, display it with all the entries, without selecting any @@ -304,36 +366,7 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT } } else - { - INT count, sel; - count = SendMessageW(ac->hwndListBox, LB_GETCOUNT, 0, 0); - - /* Change the selection */ - sel = SendMessageW(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); - if (sel >= 0) - { - WCHAR *msg; - UINT len; - - len = SendMessageW(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); - set_text_and_selection(ac, hwnd, msg, len, len); - heap_free(msg); - } - else - { - UINT len = strlenW(ac->txtbackup); - set_text_and_selection(ac, hwnd, ac->txtbackup, len, len); - } - return 0; - } + return change_selection(ac, hwnd, wParam); break; case VK_DELETE: { -- 1.9.1
Thanks for making this a helper function. Signed-off-by: Huw Davies <huw(a)codeweavers.com>
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- v3: Use bitfield for future expansion. dlls/shell32/autocomplete.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 5cf63fe..d9b109d 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -67,8 +67,9 @@ typedef struct IAutoComplete2 IAutoComplete2_iface; IAutoCompleteDropDown IAutoCompleteDropDown_iface; LONG ref; - BOOL initialized; - BOOL enabled; + BYTE initialized : 1; + BYTE enabled : 1; + AUTOCOMPLETEOPTIONS options; HWND hwndEdit; HWND hwndListBox; WNDPROC wpOrigEditProc; @@ -76,7 +77,6 @@ typedef struct WCHAR *txtbackup; WCHAR *quickComplete; IEnumString *enumstr; - AUTOCOMPLETEOPTIONS options; } IAutoCompleteImpl; enum autoappend_flag -- 1.9.1
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> ---
v3: Use bitfield for future expansion.
dlls/shell32/autocomplete.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Why is packing relevant at all? How many of these structures are you expecting to allocate? -- Alexandre Julliard julliard(a)winehq.org
On Thu, Sep 27, 2018 at 1:55 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Why is packing relevant at all? How many of these structures are you expecting to allocate?
-- Alexandre Julliard julliard(a)winehq.org
It's not that it's really relevant (patch was really minor), but the original used two BOOLs and then I had to add a UCHAR (next patch, this was split from), each of which would have 3 bytes of padding (and more on x86_64 later, when I'll have to add more fields), which just seems a bit wasteful for the cache and a bit inelegant to me. I mean it's a trivial change anyway, it's not like it makes the code itself worse off, IMO. :-)
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
On Thu, Sep 27, 2018 at 1:55 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
Why is packing relevant at all? How many of these structures are you expecting to allocate?
-- Alexandre Julliard julliard(a)winehq.org
It's not that it's really relevant (patch was really minor), but the original used two BOOLs and then I had to add a UCHAR (next patch, this was split from), each of which would have 3 bytes of padding (and more on x86_64 later, when I'll have to add more fields), which just seems a bit wasteful for the cache and a bit inelegant to me. I mean it's a trivial change anyway, it's not like it makes the code itself worse off, IMO. :-)
It's a gratuitous change, and in many cases would generate worse code because of the need to manipulate bits. Losing a couple of padding bytes here and there is not a problem that needs fixing. -- Alexandre Julliard julliard(a)winehq.org
On Thu, Sep 27, 2018 at 2:18 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
It's a gratuitous change, and in many cases would generate worse code because of the need to manipulate bits. Losing a couple of padding bytes here and there is not a problem that needs fixing.
-- Alexandre Julliard julliard(a)winehq.org
Originally I didn't use bit fields, only converted them to BOOLEAN (1 byte so all 3 along with UCHAR fit in just one 4-byte block). I was suggested to use bit fields, and it makes sense in this context so I think it's okay -- booleans on bit fields are efficient (only one instruction), there's no unpacking that has to be done and so on. (this only applies to 1-bit bit fields, i.e. booleans or flags) Of course this is totally minor but I even taking minor into account, despite this the minor CPU cache improvements outweight it, probably. Lastly, this opens up easier expansion into more boolean flags into the future (if needed) instead of having to use bitwise operators and #define. Sorry for long explanation, just want to give my reasoning.
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
On Thu, Sep 27, 2018 at 2:18 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
It's a gratuitous change, and in many cases would generate worse code because of the need to manipulate bits. Losing a couple of padding bytes here and there is not a problem that needs fixing.
-- Alexandre Julliard julliard(a)winehq.org
Originally I didn't use bit fields, only converted them to BOOLEAN (1 byte so all 3 along with UCHAR fit in just one 4-byte block).
I was suggested to use bit fields, and it makes sense in this context so I think it's okay -- booleans on bit fields are efficient (only one instruction), there's no unpacking that has to be done and so on. (this only applies to 1-bit bit fields, i.e. booleans or flags)
Of course this is totally minor but I even taking minor into account, despite this the minor CPU cache improvements outweight it, probably. Lastly, this opens up easier expansion into more boolean flags into the future (if needed) instead of having to use bitwise operators and #define.
Sorry for long explanation, just want to give my reasoning.
The basic rule is that you don't make changes unless there's a reason. When things are simply a matter of taste and either way works fine, the existing code should be left alone. -- Alexandre Julliard julliard(a)winehq.org
On Thu, Sep 27, 2018 at 4:08 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
The basic rule is that you don't make changes unless there's a reason. When things are simply a matter of taste and either way works fine, the existing code should be left alone.
-- Alexandre Julliard julliard(a)winehq.org
Yes but isn't a reason to possibly re-arrange it, when adding more fields (patch 5/6)? Of course, it changes the overall layout, so it won't just feel like it's built on top of each other, but rather like it fits and belongs better together. i.e. like it was "designed like this from the beginning" which is cleaner than trying to avoid to change existing fields in my opinion.
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
On Thu, Sep 27, 2018 at 4:08 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
The basic rule is that you don't make changes unless there's a reason. When things are simply a matter of taste and either way works fine, the existing code should be left alone.
-- Alexandre Julliard julliard(a)winehq.org
Yes but isn't a reason to possibly re-arrange it, when adding more fields (patch 5/6)? Of course, it changes the overall layout, so it won't just feel like it's built on top of each other, but rather like it fits and belongs better together. i.e. like it was "designed like this from the beginning" which is cleaner than trying to avoid to change existing fields in my opinion.
If it's necessary to make things easier to understand, yes. That doesn't seem to be the case here. The important resource here is not a few bytes of memory or a couple of CPU cycles, it's developer's time. Making unnecessary changes has a cost in brain cycles, when reviewing the patch, when searching through the history, and when trying to understand why things are done a certain way. Please keep that in mind. -- Alexandre Julliard julliard(a)winehq.org
On Thu, Sep 27, 2018 at 5:04 PM, Alexandre Julliard <julliard(a)winehq.org> wrote:
If it's necessary to make things easier to understand, yes. That doesn't seem to be the case here.
The important resource here is not a few bytes of memory or a couple of CPU cycles, it's developer's time. Making unnecessary changes has a cost in brain cycles, when reviewing the patch, when searching through the history, and when trying to understand why things are done a certain way. Please keep that in mind.
-- Alexandre Julliard julliard(a)winehq.org
Yes in those situations, but I think this patch is pretty harmless as it is, it doesn't make it harder to understand, just moving around 1 member and using bitfield for flags. The new field would have to be added anyway. I guess the original rejected form with BOOLEAN instead of BOOL would have been less changes. But anyway please consider it, so that the rest of the patches in this series can also be committed, then I can send the IACList::Expand patch series and finally finish the original series this is all based from for now... and move on to the redesign of the enumeration (to finish two of the TODO at the top of the file)
When selecting an item from the AutoComplete's listbox, the Return key should act the same as a left click on it (place the text, select it, and hide the listbox). This matches Windows behavior. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Also fixes quickComplete since a VK_RETURN is sent as a WM_CHAR of '\n' when CTRL is pressed and it must not be forwarded in WM_CHAR. Furthermore, we have to process this in WM_KEYDOWN to ensure correct behavior. The problem isn't the autocompletion here, but rather the edit control, which must not receive that character at all. no_fwd_char is also needed for future patches (and even the next patch) to suppress forwarding the respective char from the KeyDown (for example, when ACO_USETAB will be implemented). dlls/shell32/autocomplete.c | 39 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index d9b109d..d4bc8b1 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -69,6 +69,7 @@ typedef struct LONG ref; BYTE initialized : 1; BYTE enabled : 1; + UCHAR no_fwd_char; AUTOCOMPLETEOPTIONS options; HWND hwndEdit; HWND hwndListBox; @@ -134,6 +135,34 @@ static size_t format_quick_complete(WCHAR *dst, const WCHAR *qc, const WCHAR *st return dst - base; } +static BOOL select_item_with_return_key(IAutoCompleteImpl *ac, HWND hwnd) +{ + WCHAR *text; + HWND hwndListBox = ac->hwndListBox; + if (!(ac->options & ACO_AUTOSUGGEST)) + return FALSE; + + if (IsWindowVisible(hwndListBox)) + { + INT sel = SendMessageW(hwndListBox, LB_GETCURSEL, 0, 0); + if (sel >= 0) + { + UINT len = SendMessageW(hwndListBox, LB_GETTEXTLEN, sel, 0); + if ((text = heap_alloc((len + 1) * sizeof(WCHAR)))) + { + len = SendMessageW(hwndListBox, LB_GETTEXT, sel, (LPARAM)text); + set_text_and_selection(ac, hwnd, text, 0, len); + ShowWindow(hwndListBox, SW_HIDE); + ac->no_fwd_char = '\r'; /* RETURN char */ + heap_free(text); + return TRUE; + } + } + } + ShowWindow(hwndListBox, SW_HIDE); + return FALSE; +} + static LRESULT change_selection(IAutoCompleteImpl *ac, HWND hwnd, UINT key) { INT count = SendMessageW(ac->hwndListBox, LB_GETCOUNT, 0, 0); @@ -324,6 +353,8 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT WCHAR *text, *buf; size_t sz; UINT len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0); + ac->no_fwd_char = '\n'; /* CTRL+RETURN char */ + if (!(text = heap_alloc((len + 1) * sizeof(WCHAR)))) return 0; len = SendMessageW(hwnd, WM_GETTEXT, len + 1, (LPARAM)text); @@ -342,8 +373,8 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT return 0; } - if (ac->options & ACO_AUTOSUGGEST) - ShowWindow(ac->hwndListBox, SW_HIDE); + if (select_item_with_return_key(ac, hwnd)) + return 0; break; case VK_UP: case VK_DOWN: @@ -375,6 +406,7 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT return ret; } } + ac->no_fwd_char = '\0'; return CallWindowProcW(ac->wpOrigEditProc, hwnd, uMsg, wParam, lParam); } @@ -404,6 +436,9 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, return ACEditSubclassProc_KeyDown(This, hwnd, uMsg, wParam, lParam); case WM_CHAR: case WM_UNICHAR: + if (wParam == This->no_fwd_char) return 0; + This->no_fwd_char = '\0'; + /* Don't autocomplete at all on most control characters */ if (iscntrlW(wParam) && !(wParam >= '\b' && wParam <= '\r')) break; -- 1.9.1
This isn't particularly pretty, but I don't see another way. Signed-off-by: Huw Davies <huw(a)codeweavers.com>
When the listbox is visible, ESC should hide it. Only when it's not visible should it be forwarded to the edit control. This matches Windows behavior. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- no_fwd_char is needed because we cannot send an ESC character in WM_CHAR to the edit control, which clears the text. We have to handle it in KeyDown though, just like VK_RETURN in previous patch. dlls/shell32/autocomplete.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index d4bc8b1..f650bf2 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -346,6 +346,15 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT { switch (wParam) { + case VK_ESCAPE: + /* When pressing ESC, Windows hides the auto-suggest listbox, if visible */ + if ((ac->options & ACO_AUTOSUGGEST) && IsWindowVisible(ac->hwndListBox)) + { + ShowWindow(ac->hwndListBox, SW_HIDE); + ac->no_fwd_char = 0x1B; /* ESC char */ + return 0; + } + break; case VK_RETURN: /* If quickComplete is set and control is pressed, replace the string */ if (ac->quickComplete && (GetKeyState(VK_CONTROL) & 0x8000)) -- 1.9.1
Signed-off-by: Huw Davies <huw(a)codeweavers.com>
participants (3)
-
Alexandre Julliard -
Gabriel Ivăncescu -
Huw Davies