[PATCH v4 1/7] shell32/autocomplete: Fix handling of Return key when an auto-suggestion item is selected
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@gmail.com ---
Supersedes the remaining series from 150642.
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 5cf63fe..a342986 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -77,6 +77,7 @@ typedef struct WCHAR *quickComplete; IEnumString *enumstr; AUTOCOMPLETEOPTIONS options; + UCHAR no_fwd_char; } IAutoCompleteImpl;
enum autoappend_flag @@ -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;
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@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 a342986..afe089b 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))
Signed-off-by: Huw Davies huw@codeweavers.com
There's no point to have it lingering around, since it's always recreated before being shown again.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com ---
Supersedes the remaining series from 150642.
A helper function will be needed after the enumeration redesign, anyway.
dlls/shell32/autocomplete.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index afe089b..c25f81e 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -109,6 +109,12 @@ static void set_text_and_selection(IAutoCompleteImpl *ac, HWND hwnd, WCHAR *text CallWindowProcW(proc, hwnd, EM_SETSEL, start, end); }
+static void hide_listbox(IAutoCompleteImpl *ac, HWND hwnd) +{ + ShowWindow(hwnd, SW_HIDE); + SendMessageW(hwnd, LB_RESETCONTENT, 0, 0); +} + 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 @@ -152,14 +158,14 @@ static BOOL select_item_with_return_key(IAutoCompleteImpl *ac, HWND hwnd) { len = SendMessageW(hwndListBox, LB_GETTEXT, sel, (LPARAM)text); set_text_and_selection(ac, hwnd, text, 0, len); - ShowWindow(hwndListBox, SW_HIDE); + hide_listbox(ac, hwndListBox); ac->no_fwd_char = '\r'; /* RETURN char */ heap_free(text); return TRUE; } } } - ShowWindow(hwndListBox, SW_HIDE); + hide_listbox(ac, hwndListBox); return FALSE; }
@@ -259,7 +265,7 @@ static void autocomplete_text(IAutoCompleteImpl *ac, HWND hwnd, enum autoappend_ if (flag != autoappend_flag_displayempty && len == 0) { if (ac->options & ACO_AUTOSUGGEST) - ShowWindow(ac->hwndListBox, SW_HIDE); + hide_listbox(ac, ac->hwndListBox); return; }
@@ -326,7 +332,7 @@ static void autocomplete_text(IAutoCompleteImpl *ac, HWND hwnd, enum autoappend_ SendMessageW(ac->hwndListBox, WM_SETREDRAW, TRUE, 0); } else - ShowWindow(ac->hwndListBox, SW_HIDE); + hide_listbox(ac, ac->hwndListBox); } }
@@ -350,7 +356,7 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT /* When pressing ESC, Windows hides the auto-suggest listbox, if visible */ if ((ac->options & ACO_AUTOSUGGEST) && IsWindowVisible(ac->hwndListBox)) { - ShowWindow(ac->hwndListBox, SW_HIDE); + hide_listbox(ac, ac->hwndListBox); ac->no_fwd_char = 0x1B; /* ESC char */ return 0; } @@ -377,7 +383,7 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT }
if (ac->options & ACO_AUTOSUGGEST) - ShowWindow(ac->hwndListBox, SW_HIDE); + hide_listbox(ac, ac->hwndListBox); heap_free(text); return 0; } @@ -433,12 +439,12 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, { case CB_SHOWDROPDOWN: if (This->options & ACO_AUTOSUGGEST) - ShowWindow(This->hwndListBox, SW_HIDE); + hide_listbox(This, This->hwndListBox); return 0; case WM_KILLFOCUS: if ((This->options & ACO_AUTOSUGGEST) && ((HWND)wParam != This->hwndListBox)) { - ShowWindow(This->hwndListBox, SW_HIDE); + hide_listbox(This, This->hwndListBox); } break; case WM_KEYDOWN: @@ -504,7 +510,7 @@ static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, break; len = SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg); set_text_and_selection(This, This->hwndEdit, msg, 0, len); - ShowWindow(hwnd, SW_HIDE); + hide_listbox(This, hwnd); heap_free(msg); break; default:
Signed-off-by: Huw Davies huw@codeweavers.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/shell32/autocomplete.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index c25f81e..ca4227d 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -775,6 +775,8 @@ static HRESULT WINAPI IAutoComplete2_fnSetOptions(
if ((This->options & ACO_AUTOSUGGEST) && This->hwndEdit && !This->hwndListBox) create_listbox(This); + else if (!(This->options & ACO_AUTOSUGGEST) && This->hwndListBox) + hide_listbox(This, This->hwndListBox);
return hr; }
Signed-off-by: Huw Davies huw@codeweavers.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com ---
Tab goes down while Shift+Tab goes up through the list.
Note that Tab doesn't work with ACO_UPDOWNKEYDROPSLIST on Windows so it doesn't here either.
dlls/shell32/autocomplete.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index ca4227d..43c93ef 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -23,13 +23,13 @@ - ACO_AUTOAPPEND style - ACO_AUTOSUGGEST style - ACO_UPDOWNKEYDROPSLIST style + - ACO_USETAB style
- Handle pwzsRegKeyPath and pwszQuickComplete in Init
TODO: - implement ACO_SEARCH style - implement ACO_FILTERPREFIXES style - - implement ACO_USETAB style - implement ACO_RTLREADING style - implement ResetEnumerator - string compares should be case-insensitive, the content of the list should be sorted @@ -205,7 +205,7 @@ static LRESULT change_selection(IAutoCompleteImpl *ac, HWND hwnd, UINT key) } } } - else if (key == VK_UP) + else if (key == VK_UP || (key == VK_TAB && (GetKeyState(VK_SHIFT) & 0x8000))) sel = ((sel - 1) < -1) ? count - 1 : sel - 1; else sel = ((sel + 1) >= count) ? -1 : sel + 1; @@ -391,6 +391,14 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT if (select_item_with_return_key(ac, hwnd)) return 0; break; + case VK_TAB: + if ((ac->options & (ACO_AUTOSUGGEST | ACO_USETAB)) == (ACO_AUTOSUGGEST | ACO_USETAB) + && IsWindowVisible(ac->hwndListBox) && !(GetKeyState(VK_CONTROL) & 0x8000)) + { + ac->no_fwd_char = '\t'; + return change_selection(ac, hwnd, wParam); + } + break; case VK_UP: case VK_DOWN: case VK_PRIOR: @@ -646,7 +654,6 @@ static HRESULT WINAPI IAutoComplete2_fnInit(
if (This->options & ACO_SEARCH) FIXME(" ACO_SEARCH not supported\n"); if (This->options & ACO_FILTERPREFIXES) FIXME(" ACO_FILTERPREFIXES not supported\n"); - if (This->options & ACO_USETAB) FIXME(" ACO_USETAB not supported\n"); if (This->options & ACO_RTLREADING) FIXME(" ACO_RTLREADING not supported\n");
if (!hwndEdit || !punkACL)
On 09/28/2018 02:44 PM, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
Tab goes down while Shift+Tab goes up through the list.
Note that Tab doesn't work with ACO_UPDOWNKEYDROPSLIST on Windows so it doesn't here either.
dlls/shell32/autocomplete.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index ca4227d..43c93ef 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -23,13 +23,13 @@ - ACO_AUTOAPPEND style - ACO_AUTOSUGGEST style - ACO_UPDOWNKEYDROPSLIST style
ACO_USETAB style
Handle pwzsRegKeyPath and pwszQuickComplete in Init
TODO:
- implement ACO_SEARCH style
- implement ACO_FILTERPREFIXES style
- implement ACO_USETAB style
- implement ACO_RTLREADING style
- implement ResetEnumerator
- string compares should be case-insensitive, the content of the list should be sorted
Removing from TODO list is obviously right thing to do, but "Implemented" list is useless in my opinion. I'd remove it completely when last TODO entry is gone for example.
@@ -205,7 +205,7 @@ static LRESULT change_selection(IAutoCompleteImpl *ac, HWND hwnd, UINT key) } } }
- else if (key == VK_UP)
- else if (key == VK_UP || (key == VK_TAB && (GetKeyState(VK_SHIFT) & 0x8000))) sel = ((sel - 1) < -1) ? count - 1 : sel - 1; else sel = ((sel + 1) >= count) ? -1 : sel + 1;
@@ -391,6 +391,14 @@ static LRESULT ACEditSubclassProc_KeyDown(IAutoCompleteImpl *ac, HWND hwnd, UINT if (select_item_with_return_key(ac, hwnd)) return 0; break;
case VK_TAB:if ((ac->options & (ACO_AUTOSUGGEST | ACO_USETAB)) == (ACO_AUTOSUGGEST | ACO_USETAB)&& IsWindowVisible(ac->hwndListBox) && !(GetKeyState(VK_CONTROL) & 0x8000)){ac->no_fwd_char = '\t';return change_selection(ac, hwnd, wParam);}break; case VK_UP: case VK_DOWN: case VK_PRIOR:@@ -646,7 +654,6 @@ static HRESULT WINAPI IAutoComplete2_fnInit(
if (This->options & ACO_SEARCH) FIXME(" ACO_SEARCH not supported\n"); if (This->options & ACO_FILTERPREFIXES) FIXME(" ACO_FILTERPREFIXES not supported\n");
if (This->options & ACO_USETAB) FIXME(" ACO_USETAB not supported\n"); if (This->options & ACO_RTLREADING) FIXME(" ACO_RTLREADING not supported\n");
if (!hwndEdit || !punkACL)
On Fri, Sep 28, 2018 at 2:57 PM Nikolay Sivov nsivov@codeweavers.com wrote:
Removing from TODO list is obviously right thing to do, but "Implemented" list is useless in my opinion. I'd remove it completely when last TODO entry is gone for example.
I agree but I think it should be removed at the end only. It would look pretty silly to have only some of them in the Implemented list, while others aren't, unless you want to get rid of all of them right now.
It should probably be done in a new patch though after this one if we do it.
Signed-off-by: Huw Davies huw@codeweavers.com
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com ---
This enables Total Commander to show full paths on many edit boxes, like on Windows, instead of just the current directory's entries.
I couldn't get the / delimiter to work at all on Windows XP, so MSDN is probably wrong here. Despite that, I've actually added support for it in this patch, as a Wine extension, to help with unix-style paths autocompletion since we also support those.
dlls/shell32/autocomplete.c | 73 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 71 insertions(+), 2 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 43c93ef..cd2abd3 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -2,6 +2,7 @@ * AutoComplete interfaces implementation. * * Copyright 2004 Maxime Bellengé maxime.bellenge@laposte.net + * Copyright 2018 Gabriel Ivăncescu gabrielopcode@gmail.com * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -24,7 +25,7 @@ - ACO_AUTOSUGGEST style - ACO_UPDOWNKEYDROPSLIST style - ACO_USETAB style - + - IACList::Expand - Handle pwzsRegKeyPath and pwszQuickComplete in Init
TODO: @@ -76,6 +77,7 @@ typedef struct WCHAR *txtbackup; WCHAR *quickComplete; IEnumString *enumstr; + IACList *aclist; AUTOCOMPLETEOPTIONS options; UCHAR no_fwd_char; } IAutoCompleteImpl; @@ -229,6 +231,54 @@ static LRESULT change_selection(IAutoCompleteImpl *ac, HWND hwnd, UINT key) return 0; }
+static void aclist_expand(IAutoCompleteImpl *ac, WCHAR *txt) +{ + /* call IACList::Expand only when needed + '/' is allowed as a delim for unix paths */ + WCHAR c, *p, *last_delim, *old_txt = ac->txtbackup; + size_t i = 0; + + /* skip the shared prefix */ + while ((c = tolowerW(txt[i])) == tolowerW(old_txt[i])) + { + if (c == '\0') return; + i++; + } + + /* they differ at this point, check for a delim further in txt */ + last_delim = NULL; + for (p = &txt[i]; *p != '\0'; p++) + if (*p == '\' || *p == '/') + last_delim = p; + if (last_delim) + goto expand; + + /* txt has no delim after i, check for a delim further in old_txt */ + for (p = &old_txt[i]; *p != '\0'; p++) + if (*p == '\' || *p == '/') + { + /* find the delim before i (if any) */ + while (i--) + { + if (txt[i] == '\' || txt[i] == '/') + { + last_delim = &txt[i]; + goto expand; + } + } + break; /* Windows doesn't expand without a delim */ + } + + /* they differ, but without any different delims, so no need to expand */ + return; + +expand: + c = last_delim[1]; + last_delim[1] = '\0'; + IACList_Expand(ac->aclist, txt); + last_delim[1] = c; +} + static void autoappend_str(IAutoCompleteImpl *ac, WCHAR *text, UINT len, WCHAR *str, HWND hwnd) { DWORD sel_start; @@ -276,6 +326,17 @@ static void autocomplete_text(IAutoCompleteImpl *ac, HWND hwnd, enum autoappend_ if (len + 1 != size) text = heap_realloc(text, (len + 1) * sizeof(WCHAR));
+ /* Reset it here to simplify the logic in aclist_expand for + empty strings, since it tracks changes using txtbackup, + and Reset needs to be called before IACList::Expand */ + IEnumString_Reset(ac->enumstr); + if (ac->aclist) + { + aclist_expand(ac, text); + if (text[len - 1] == '\' || text[len - 1] == '/') + flag = autoappend_flag_no; + } + /* Set txtbackup to point to text itself (which must not be released) */ heap_free(ac->txtbackup); ac->txtbackup = text; @@ -285,7 +346,6 @@ static void autocomplete_text(IAutoCompleteImpl *ac, HWND hwnd, enum autoappend_ SendMessageW(ac->hwndListBox, WM_SETREDRAW, FALSE, 0); SendMessageW(ac->hwndListBox, LB_RESETCONTENT, 0, 0); } - IEnumString_Reset(ac->enumstr); for (cpt = 0;;) { LPOLESTR strs = NULL; @@ -615,6 +675,8 @@ static ULONG WINAPI IAutoComplete2_fnRelease( heap_free(This->txtbackup); if (This->enumstr) IEnumString_Release(This->enumstr); + if (This->aclist) + IACList_Release(This->aclist); heap_free(This); } return refCount; @@ -671,6 +733,13 @@ static HRESULT WINAPI IAutoComplete2_fnInit( return E_NOINTERFACE; }
+ /* Prevent txtbackup from ever being NULL to simplify aclist_expand */ + if ((This->txtbackup = heap_alloc_zero(sizeof(WCHAR))) == NULL) + return E_OUTOFMEMORY; + + if (FAILED (IUnknown_QueryInterface (punkACL, &IID_IACList, (LPVOID*)&This->aclist))) + This->aclist = NULL; + This->initialized = TRUE; This->hwndEdit = hwndEdit;
On Fri, Sep 28, 2018 at 02:44:11PM +0300, Gabriel Ivăncescu wrote:
@@ -229,6 +231,54 @@ static LRESULT change_selection(IAutoCompleteImpl *ac, HWND hwnd, UINT key) return 0; }
+static void aclist_expand(IAutoCompleteImpl *ac, WCHAR *txt) +{
- /* call IACList::Expand only when needed
'/' is allowed as a delim for unix paths */- WCHAR c, *p, *last_delim, *old_txt = ac->txtbackup;
- size_t i = 0;
- /* skip the shared prefix */
- while ((c = tolowerW(txt[i])) == tolowerW(old_txt[i]))
- {
if (c == '\0') return;i++;- }
- /* they differ at this point, check for a delim further in txt */
- last_delim = NULL;
- for (p = &txt[i]; *p != '\0'; p++)
if (*p == '\\' || *p == '/')last_delim = p;- if (last_delim)
goto expand;- /* txt has no delim after i, check for a delim further in old_txt */
- for (p = &old_txt[i]; *p != '\0'; p++)
if (*p == '\\' || *p == '/'){/* find the delim before i (if any) */while (i--){if (txt[i] == '\\' || txt[i] == '/'){last_delim = &txt[i];goto expand;}}break; /* Windows doesn't expand without a delim */}- /* they differ, but without any different delims, so no need to expand */
- return;
+expand:
- c = last_delim[1];
- last_delim[1] = '\0';
- IACList_Expand(ac->aclist, txt);
- last_delim[1] = c;
+}
This is going to need some work; trying to follow this code is just too hard.
Huw.
On Mon, Oct 15, 2018 at 1:15 PM Huw Davies huw@codeweavers.com wrote:
This is going to need some work; trying to follow this code is just too hard.
Huw.
The expand method gets called with the path ending in a slash, but only if it's needed (the tests show this). For example, if the text is a\b\c\d and the previous text was a\b\c\e, there's no expansion as both would expand to a\b\c\ and Windows doesn't do it redundantly (this is needed for things like paste or replacing selections to work correctly, we can't just keep track of last character input, again, the tests show this with selection deletions and caret movements and others).
Now what the code does is 3 "stages". First it skips all the shared prefix (case insensitively). So basically the beginning of both txtbackup and the new txt are skipped if identical (and stops where they mismatch). At this point we know all characters before i (the mismatch pos) are identical so no need to check them.
Then, starting from the mismatch position, it looks for a delimiter in the new text. If it finds one, obviously, it will look for the last delim and expand the whole text up to that last delim (since they differ).
If it doesn't find one, it checks if txtbackup (the old text) has a delim after they mismatch, because if they don't, it means they only differ by non-delim characters, so no expansion is needed. (e.g. a\b\c\foo and a\b\c\bar don't need any expansion)
But if there is a delim in txtbackup (e.g. a\b\c\foo and a\b\c\bar\baz), we need to find the last delim in the shared prefix, so it scans backwards for the first delim (in this case, to expand a\b\c\ since otherwise a\b\c\bar\ was expanded before).
I know I could comment it better, but I don't want to be too verbose so some advice would be appreciated.
On Mon, Oct 15, 2018 at 02:24:01PM +0300, Gabriel Ivăncescu wrote:
On Mon, Oct 15, 2018 at 1:15 PM Huw Davies huw@codeweavers.com wrote:
This is going to need some work; trying to follow this code is just too hard.
Huw.
The expand method gets called with the path ending in a slash, but only if it's needed (the tests show this). For example, if the text is a\b\c\d and the previous text was a\b\c\e, there's no expansion as both would expand to a\b\c\ and Windows doesn't do it redundantly (this is needed for things like paste or replacing selections to work correctly, we can't just keep track of last character input, again, the tests show this with selection deletions and caret movements and others).
Now what the code does is 3 "stages". First it skips all the shared prefix (case insensitively). So basically the beginning of both txtbackup and the new txt are skipped if identical (and stops where they mismatch). At this point we know all characters before i (the mismatch pos) are identical so no need to check them.
Then, starting from the mismatch position, it looks for a delimiter in the new text. If it finds one, obviously, it will look for the last delim and expand the whole text up to that last delim (since they differ).
If it doesn't find one, it checks if txtbackup (the old text) has a delim after they mismatch, because if they don't, it means they only differ by non-delim characters, so no expansion is needed. (e.g. a\b\c\foo and a\b\c\bar don't need any expansion)
But if there is a delim in txtbackup (e.g. a\b\c\foo and a\b\c\bar\baz), we need to find the last delim in the shared prefix, so it scans backwards for the first delim (in this case, to expand a\b\c\ since otherwise a\b\c\bar\ was expanded before).
I know I could comment it better, but I don't want to be too verbose so some advice would be appreciated.
Well we've dicussed gotos already. Admittedly this one isn't inside a nested switch, but still.
You might also find strpbrkW() helpful.
Huw.
On Mon, Oct 15, 2018 at 2:55 PM Huw Davies huw@codeweavers.com wrote:
On Mon, Oct 15, 2018 at 02:24:01PM +0300, Gabriel Ivăncescu wrote:
On Mon, Oct 15, 2018 at 1:15 PM Huw Davies huw@codeweavers.com wrote:
This is going to need some work; trying to follow this code is just too hard.
Huw.
The expand method gets called with the path ending in a slash, but only if it's needed (the tests show this). For example, if the text is a\b\c\d and the previous text was a\b\c\e, there's no expansion as both would expand to a\b\c\ and Windows doesn't do it redundantly (this is needed for things like paste or replacing selections to work correctly, we can't just keep track of last character input, again, the tests show this with selection deletions and caret movements and others).
Now what the code does is 3 "stages". First it skips all the shared prefix (case insensitively). So basically the beginning of both txtbackup and the new txt are skipped if identical (and stops where they mismatch). At this point we know all characters before i (the mismatch pos) are identical so no need to check them.
Then, starting from the mismatch position, it looks for a delimiter in the new text. If it finds one, obviously, it will look for the last delim and expand the whole text up to that last delim (since they differ).
If it doesn't find one, it checks if txtbackup (the old text) has a delim after they mismatch, because if they don't, it means they only differ by non-delim characters, so no expansion is needed. (e.g. a\b\c\foo and a\b\c\bar don't need any expansion)
But if there is a delim in txtbackup (e.g. a\b\c\foo and a\b\c\bar\baz), we need to find the last delim in the shared prefix, so it scans backwards for the first delim (in this case, to expand a\b\c\ since otherwise a\b\c\bar\ was expanded before).
I know I could comment it better, but I don't want to be too verbose so some advice would be appreciated.
Well we've dicussed gotos already. Admittedly this one isn't inside a nested switch, but still.
You might also find strpbrkW() helpful.
Huw.
Ah fair point, I guess I can use strpbrkW for the delims' search, forgot about it.
But what's the problem with this goto? It's the same as failure gotos which are used a lot in Wine's codebase. I used it because to me it seems much cleaner of the purpose and the label's name (i.e. it clearly expands given the last_delim, and also jumps to the end of the function, no control flow issues).
The function itself is small and fits in one page so having an extra helper function for this seems quite a bit overkill and less elegant in encapsulation to me... (if C supported local/inner functions it wouldn't be less elegant but of course IMO)
I guess I could also duplicate the expand code but that's even worse for maintenance.
On Mon, Oct 15, 2018 at 03:32:50PM +0300, Gabriel Ivăncescu wrote:
On Mon, Oct 15, 2018 at 2:55 PM Huw Davies huw@codeweavers.com wrote:
On Mon, Oct 15, 2018 at 02:24:01PM +0300, Gabriel Ivăncescu wrote:
On Mon, Oct 15, 2018 at 1:15 PM Huw Davies huw@codeweavers.com wrote:
This is going to need some work; trying to follow this code is just too hard.
I know I could comment it better, but I don't want to be too verbose so some advice would be appreciated.
Well we've dicussed gotos already. Admittedly this one isn't inside a nested switch, but still.
But what's the problem with this goto? It's the same as failure gotos which are used a lot in Wine's codebase. I used it because to me it seems much cleaner of the purpose and the label's name (i.e. it clearly expands given the last_delim, and also jumps to the end of the function, no control flow issues).
The function itself is small and fits in one page so having an extra helper function for this seems quite a bit overkill and less elegant in encapsulation to me... (if C supported local/inner functions it wouldn't be less elegant but of course IMO)
I guess I could also duplicate the expand code but that's even worse for maintenance.
A helper function will be just fine.
Huw.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com ---
I had to remove the #include of initguid.h because it gave a duplicate IID_ShellFolderView definition with shlobj.h, which is needed for IACList.
dlls/shell32/tests/autocomplete.c | 114 +++++++++++++++++++++++++++++++++++--- 1 file changed, 106 insertions(+), 8 deletions(-)
diff --git a/dlls/shell32/tests/autocomplete.c b/dlls/shell32/tests/autocomplete.c index e85e30c..66bd472 100644 --- a/dlls/shell32/tests/autocomplete.c +++ b/dlls/shell32/tests/autocomplete.c @@ -25,8 +25,8 @@ #include "windows.h" #include "shobjidl.h" #include "shlguid.h" -#include "initguid.h" #include "shldisp.h" +#include "shlobj.h"
#include "wine/heap.h" #include "wine/test.h" @@ -269,10 +269,13 @@ static LRESULT CALLBACK HijackerWndProc2(HWND hWnd, UINT msg, WPARAM wParam, LPA struct string_enumerator { IEnumString IEnumString_iface; + IACList IACList_iface; LONG ref; WCHAR **data; int data_len; int cur; + UINT num_expand; + WCHAR last_expand[32]; };
static struct string_enumerator *impl_from_IEnumString(IEnumString *iface) @@ -282,15 +285,19 @@ static struct string_enumerator *impl_from_IEnumString(IEnumString *iface)
static HRESULT WINAPI string_enumerator_QueryInterface(IEnumString *iface, REFIID riid, void **ppv) { + struct string_enumerator *this = impl_from_IEnumString(iface); if (IsEqualGUID(riid, &IID_IEnumString) || IsEqualGUID(riid, &IID_IUnknown)) + *ppv = &this->IEnumString_iface; + else if (IsEqualGUID(riid, &IID_IACList)) + *ppv = &this->IACList_iface; + else { - IUnknown_AddRef(iface); - *ppv = iface; - return S_OK; + *ppv = NULL; + return E_NOINTERFACE; }
- *ppv = NULL; - return E_NOINTERFACE; + IUnknown_AddRef(&this->IEnumString_iface); + return S_OK; }
static ULONG WINAPI string_enumerator_AddRef(IEnumString *iface) @@ -361,7 +368,7 @@ static HRESULT WINAPI string_enumerator_Clone(IEnumString *iface, IEnumString ** return E_NOTIMPL; }
-static IEnumStringVtbl string_enumerator_vtlb = +static IEnumStringVtbl string_enumerator_vtbl = { string_enumerator_QueryInterface, string_enumerator_AddRef, @@ -372,12 +379,54 @@ static IEnumStringVtbl string_enumerator_vtlb = string_enumerator_Clone };
+static struct string_enumerator *impl_from_IACList(IACList *iface) +{ + return CONTAINING_RECORD(iface, struct string_enumerator, IACList_iface); +} + +static HRESULT WINAPI aclist_QueryInterface(IACList *iface, REFIID riid, void **ppv) +{ + return string_enumerator_QueryInterface(&impl_from_IACList(iface)->IEnumString_iface, riid, ppv); +} + +static ULONG WINAPI aclist_AddRef(IACList *iface) +{ + return string_enumerator_AddRef(&impl_from_IACList(iface)->IEnumString_iface); +} + +static ULONG WINAPI aclist_Release(IACList *iface) +{ + return string_enumerator_Release(&impl_from_IACList(iface)->IEnumString_iface); +} + +static HRESULT WINAPI aclist_Expand(IACList *iface, const WCHAR *expand) +{ + struct string_enumerator *this = impl_from_IACList(iface); + + /* see what we get called with and how many times, + don't actually do any expansion of the strings */ + memcpy(this->last_expand, expand, min((lstrlenW(expand) + 1)*sizeof(WCHAR), sizeof(this->last_expand))); + this->last_expand[ARRAY_SIZE(this->last_expand) - 1] = '\0'; + this->num_expand++; + + return S_OK; +} + +static IACListVtbl aclist_vtbl = +{ + aclist_QueryInterface, + aclist_AddRef, + aclist_Release, + aclist_Expand +}; + static HRESULT string_enumerator_create(void **ppv, WCHAR **suggestions, int count) { struct string_enumerator *object;
object = heap_alloc_zero(sizeof(*object)); - object->IEnumString_iface.lpVtbl = &string_enumerator_vtlb; + object->IEnumString_iface.lpVtbl = &string_enumerator_vtbl; + object->IACList_iface.lpVtbl = &aclist_vtbl; object->ref = 1; object->data = suggestions; object->data_len = count; @@ -399,6 +448,53 @@ static void dispatch_messages(void) } }
+static void test_aclist_expand(HWND hwnd_edit, void *enumerator) +{ + struct string_enumerator *obj = (struct string_enumerator*)enumerator; + static WCHAR str1[] = {'t','e','s','t',0}; + static WCHAR str1a[] = {'t','e','s','t','\',0}; + static WCHAR str2[] = {'t','e','s','t','\','f','o','o','\','b','a','r','\','b','a',0}; + static WCHAR str2a[] = {'t','e','s','t','\','f','o','o','\','b','a','r','\',0}; + static WCHAR str2b[] = {'t','e','s','t','\','f','o','o','\','b','a','r','\','b','a','z','_','b','b','q','\',0}; + + ok(obj->num_expand == 0, "Expected 0 expansions, got %u\n", obj->num_expand); + SendMessageW(hwnd_edit, WM_SETTEXT, 0, (LPARAM)str1); + SendMessageW(hwnd_edit, EM_SETSEL, ARRAY_SIZE(str1) - 1, ARRAY_SIZE(str1) - 1); + SendMessageW(hwnd_edit, WM_CHAR, '\', 1); + dispatch_messages(); + ok(obj->num_expand == 1, "Expected 1 expansion, got %u\n", obj->num_expand); + ok(lstrcmpW(obj->last_expand, str1a) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str1a), wine_dbgstr_w(obj->last_expand)); + SendMessageW(hwnd_edit, WM_SETTEXT, 0, (LPARAM)str2); + SendMessageW(hwnd_edit, EM_SETSEL, ARRAY_SIZE(str2) - 1, ARRAY_SIZE(str2) - 1); + SendMessageW(hwnd_edit, WM_CHAR, 'z', 1); + dispatch_messages(); + ok(obj->num_expand == 2, "Expected 2 expansions, got %u\n", obj->num_expand); + ok(lstrcmpW(obj->last_expand, str2a) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str2a), wine_dbgstr_w(obj->last_expand)); + SetFocus(hwnd_edit); + SendMessageW(hwnd_edit, WM_CHAR, '_', 1); + SendMessageW(hwnd_edit, WM_CHAR, 'b', 1); + SetFocus(0); + SetFocus(hwnd_edit); + SendMessageW(hwnd_edit, WM_CHAR, 'b', 1); + SendMessageW(hwnd_edit, WM_CHAR, 'q', 1); + dispatch_messages(); + ok(obj->num_expand == 2, "Expected 2 expansions, got %u\n", obj->num_expand); + SendMessageW(hwnd_edit, WM_CHAR, '\', 1); + dispatch_messages(); + ok(obj->num_expand == 3, "Expected 3 expansions, got %u\n", obj->num_expand); + ok(lstrcmpW(obj->last_expand, str2b) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str2b), wine_dbgstr_w(obj->last_expand)); + SendMessageW(hwnd_edit, EM_SETSEL, ARRAY_SIZE(str1a) - 1, -1); + SendMessageW(hwnd_edit, WM_CHAR, 'x', 1); + SendMessageW(hwnd_edit, WM_CHAR, 'y', 1); + dispatch_messages(); + ok(obj->num_expand == 4, "Expected 4 expansions, got %u\n", obj->num_expand); + ok(lstrcmpW(obj->last_expand, str1a) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str1a), wine_dbgstr_w(obj->last_expand)); + SendMessageW(hwnd_edit, EM_SETSEL, ARRAY_SIZE(str1) - 1, -1); + SendMessageW(hwnd_edit, WM_CHAR, 'x', 1); + dispatch_messages(); + ok(obj->num_expand == 4, "Expected 4 expansions, got %u\n", obj->num_expand); +} + static void test_custom_source(void) { static WCHAR str_alpha[] = {'t','e','s','t','1',0}; @@ -464,6 +560,8 @@ static void test_custom_source(void) ok(lstrcmpW(str_beta, buffer) == 0, "Expected %s, got %s\n", wine_dbgstr_w(str_beta), wine_dbgstr_w(buffer)); /* end of hijacks */
+ test_aclist_expand(hwnd_edit, enumerator); + ShowWindow(hMainWnd, SW_HIDE); DestroyWindow(hwnd_edit); }
On 09/28/2018 02:44 PM, Gabriel Ivăncescu wrote:
case WM_CHAR: case WM_UNICHAR:
if (wParam == This->no_fwd_char) return 0;This->no_fwd_char = '\0';
I don't know how important it is in practice, but shouldn't it go through MapVirtualKey, storing VK_ code instead of a char?
On Fri, Sep 28, 2018 at 3:00 PM Nikolay Sivov nsivov@codeweavers.com wrote:
On 09/28/2018 02:44 PM, Gabriel Ivăncescu wrote:
case WM_CHAR: case WM_UNICHAR:
if (wParam == This->no_fwd_char) return 0;This->no_fwd_char = '\0';I don't know how important it is in practice, but shouldn't it go through MapVirtualKey, storing VK_ code instead of a char?
Isn't that the opposite effect of what's needed here? Or are you saying I should use it when setting no_fwd_char itself? In that case, I don't think it will work for the return key, since it's different depending on whether CTRL is pressed or not (like in quickComplete).
On Fri, Sep 28, 2018 at 03:11:37PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 28, 2018 at 3:00 PM Nikolay Sivov nsivov@codeweavers.com wrote:
On 09/28/2018 02:44 PM, Gabriel Ivăncescu wrote:
case WM_CHAR: case WM_UNICHAR:
if (wParam == This->no_fwd_char) return 0;This->no_fwd_char = '\0';I don't know how important it is in practice, but shouldn't it go through MapVirtualKey, storing VK_ code instead of a char?
Isn't that the opposite effect of what's needed here? Or are you saying I should use it when setting no_fwd_char itself? In that case, I don't think it will work for the return key, since it's different depending on whether CTRL is pressed or not (like in quickComplete).
I'm wondering whether a simple flag 'ignore_next_char' would be cleaner.
Huw.
On Tue, Oct 9, 2018 at 2:54 PM Huw Davies huw@codeweavers.com wrote:
On Fri, Sep 28, 2018 at 03:11:37PM +0300, Gabriel Ivăncescu wrote:
On Fri, Sep 28, 2018 at 3:00 PM Nikolay Sivov nsivov@codeweavers.com wrote:
On 09/28/2018 02:44 PM, Gabriel Ivăncescu wrote:
case WM_CHAR: case WM_UNICHAR:
if (wParam == This->no_fwd_char) return 0;This->no_fwd_char = '\0';I don't know how important it is in practice, but shouldn't it go through MapVirtualKey, storing VK_ code instead of a char?
Isn't that the opposite effect of what's needed here? Or are you saying I should use it when setting no_fwd_char itself? In that case, I don't think it will work for the return key, since it's different depending on whether CTRL is pressed or not (like in quickComplete).
I'm wondering whether a simple flag 'ignore_next_char' would be cleaner.
Huw.
I don't think that's necessarily correct in all situations, depending on the application, since there's no guarantee it will call TranslateMessage all the time. And also, WM_CHAR can be sent artificially without any WM_KEYDOWN, for example with AutoHotkey it's quite common. In this case it will end up ignoring an unrelated character if we use the flag method.
-
Gabriel Ivăncescu -
Huw Davies -
Nikolay Sivov