[PATCH v4 1/4] shell32/autocomplete: Implement a cache and sort the enumerated strings for proper behavior
Windows doesn't reset and re-enumerate it everytime autocompletion happens, and it also sorts the strings. This matches it more closely and makes it more useable on large lists as well.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com ---
v4: Simplified it more and culled many variables.
dlls/shell32/autocomplete.c | 193 ++++++++++++++++++++++++++++++++------------ 1 file changed, 143 insertions(+), 50 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index b3f86f3..5a31259 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -25,7 +25,6 @@ - implement ACO_FILTERPREFIXES style - implement ACO_RTLREADING style - implement ResetEnumerator - - string compares should be case-insensitive, the content of the list should be sorted
*/ #include "config.h" @@ -62,6 +61,8 @@ typedef struct LONG ref; BOOL initialized; BOOL enabled; + UINT enum_strs_num; + WCHAR **enum_strs; HWND hwndEdit; HWND hwndListBox; WNDPROC wpOrigEditProc; @@ -103,10 +104,88 @@ static void set_text_and_selection(IAutoCompleteImpl *ac, HWND hwnd, WCHAR *text CallWindowProcW(proc, hwnd, EM_SETSEL, start, end); }
+static int enumerate_strings_cmpfn(const void *a, const void *b) +{ + return strcmpiW(*(WCHAR* const*)a, *(WCHAR* const*)b); +} + +static void enumerate_strings(IAutoCompleteImpl *ac) +{ + /* + Enumerate all of the strings and sort them in the internal list. + + We don't free the enumerated strings (except on error) to avoid needless + copies, until the next reset (or the object itself is destroyed) + */ + UINT i, cur = 0, array_size = 1024; + LPOLESTR *strs = NULL, *tmp; + ULONG n; + + do + { + if ((tmp = heap_realloc(strs, array_size * sizeof(*strs))) == NULL) + goto fail; + strs = tmp; + + do + IEnumString_Next(ac->enumstr, array_size - cur, &strs[cur], &n); + while (n != 0 && (cur += n) < array_size); + + array_size *= 2; + } while (n != 0); + + /* Allocate even if there were zero strings enumerated, to mark it non-NULL */ + if ((tmp = heap_realloc(strs, cur * sizeof(*strs)))) + { + strs = tmp; + if (cur > 0) + qsort(strs, cur, sizeof(*strs), enumerate_strings_cmpfn); + + ac->enum_strs = strs; + ac->enum_strs_num = cur; + return; + } + +fail: + i = cur; + while (i--) + CoTaskMemFree(strs[i]); + heap_free(strs); +} + +static UINT find_first_matching_enum_str(IAutoCompleteImpl *ac, WCHAR *text, UINT len) +{ + WCHAR **strs = ac->enum_strs; + UINT index = ~0, a = 0, b = ac->enum_strs_num; + while (a < b) + { + UINT i = (a + b - 1) / 2; + int cmp = strncmpiW(text, strs[i], len); + if (cmp == 0) index = i; + if (cmp <= 0) b = i; + else a = i + 1; + } + return index; +} + +static void free_enum_strs(IAutoCompleteImpl *ac) +{ + WCHAR **strs = ac->enum_strs; + if (strs) + { + UINT i = ac->enum_strs_num; + ac->enum_strs = NULL; + while (i--) + CoTaskMemFree(strs[i]); + heap_free(strs); + } +} + static void hide_listbox(IAutoCompleteImpl *ac, HWND hwnd) { ShowWindow(hwnd, SW_HIDE); SendMessageW(hwnd, LB_RESETCONTENT, 0, 0); + free_enum_strs(ac); }
static void show_listbox(IAutoCompleteImpl *ac, UINT cnt) @@ -240,7 +319,11 @@ static LRESULT change_selection(IAutoCompleteImpl *ac, HWND hwnd, UINT key)
static BOOL do_aclist_expand(IAutoCompleteImpl *ac, WCHAR *txt, WCHAR *last_delim) { - WCHAR c = last_delim[1]; + WCHAR c; + free_enum_strs(ac); + IEnumString_Reset(ac->enumstr); /* call before expand */ + + c = last_delim[1]; last_delim[1] = '\0'; IACList_Expand(ac->aclist, txt); last_delim[1] = c; @@ -276,6 +359,9 @@ static BOOL aclist_expand(IAutoCompleteImpl *ac, WCHAR *txt) while (i--) if (strchrW(delims, txt[i])) return do_aclist_expand(ac, txt, &txt[i]); + + /* Windows doesn't expand without a delim, but it does reset */ + free_enum_strs(ac); }
return FALSE; @@ -312,60 +398,60 @@ static BOOL display_matching_strs(IAutoCompleteImpl *ac, WCHAR *text, UINT len, HWND hwnd, enum autoappend_flag flag) { /* Return FALSE if we need to hide the listbox */ - UINT cpt; + WCHAR **str = ac->enum_strs; + UINT cnt, a, b, i; + if (!str) return (ac->options & ACO_AUTOSUGGEST) ? FALSE : TRUE;
- if (ac->options & ACO_AUTOSUGGEST) - { - SendMessageW(ac->hwndListBox, WM_SETREDRAW, FALSE, 0); - SendMessageW(ac->hwndListBox, LB_RESETCONTENT, 0, 0); - } - for (cpt = 0;;) + if (len) { - HRESULT hr; - LPOLESTR strs = NULL; - ULONG fetched; - - hr = IEnumString_Next(ac->enumstr, 1, &strs, &fetched); - if (hr != S_OK) - break; - - if (!strncmpiW(text, strs, len)) + i = find_first_matching_enum_str(ac, text, len); + if (i == ~0) + return (ac->options & ACO_AUTOSUGGEST) ? FALSE : TRUE; + + if (flag == autoappend_flag_yes) + autoappend_str(ac, text, len, str[i], hwnd); + if (!(ac->options & ACO_AUTOSUGGEST)) + return TRUE; + + /* Find the last string that begins with text, starting the search from i, + which is guaranteed to find at least one string (if none other, then i) */ + a = i, b = ac->enum_strs_num; + do { - if (cpt == 0 && flag == autoappend_flag_yes) - { - autoappend_str(ac, text, len, strs, hwnd); - if (!(ac->options & ACO_AUTOSUGGEST)) - { - CoTaskMemFree(strs); - break; - } - } - - if (ac->options & ACO_AUTOSUGGEST) - SendMessageW(ac->hwndListBox, LB_ADDSTRING, 0, (LPARAM)strs); - - cpt++; - } - - CoTaskMemFree(strs); + UINT mid = (a + b - 1) / 2; + if (!strncmpiW(text, str[mid], len)) + a = mid + 1; + else + b = mid; + } while (a < b); } - - if (ac->options & ACO_AUTOSUGGEST) + else { - if (cpt) - { - show_listbox(ac, cpt); - SendMessageW(ac->hwndListBox, WM_SETREDRAW, TRUE, 0); - } - else + if (!(ac->options & ACO_AUTOSUGGEST)) + return TRUE; + i = 0; + a = ac->enum_strs_num; + if (a == 0) return FALSE; } + cnt = a - i; + + SendMessageW(ac->hwndListBox, WM_SETREDRAW, FALSE, 0); + SendMessageW(ac->hwndListBox, LB_RESETCONTENT, 0, 0); + SendMessageW(ac->hwndListBox, LB_INITSTORAGE, cnt, 0); + do + SendMessageW(ac->hwndListBox, LB_INSERTSTRING, -1, (LPARAM)str[i]); + while (++i < a); + + show_listbox(ac, cnt); + SendMessageW(ac->hwndListBox, WM_SETREDRAW, TRUE, 0); return TRUE; }
static void autocomplete_text(IAutoCompleteImpl *ac, HWND hwnd, enum autoappend_flag flag) { WCHAR *text; + BOOL expanded = FALSE; UINT size, len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0);
if (flag != autoappend_flag_displayempty && len == 0) @@ -382,28 +468,35 @@ 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; + expanded = aclist_expand(ac, text); + } + if (expanded || !ac->enum_strs) + { + if (!expanded) IEnumString_Reset(ac->enumstr); + enumerate_strings(ac); }
- /* Set txtbackup to point to text itself (which must not be released) */ + /* Set txtbackup to point to text itself (which must not be released), + and it must be done here since aclist_expand uses it to track changes */ heap_free(ac->txtbackup); ac->txtbackup = text;
if (!display_matching_strs(ac, text, len, hwnd, flag)) - hide_listbox(ac, ac->hwndListBox); + { + /* Hide the listbox, but do not clear the enum strs, to match Windows */ + ShowWindow(ac->hwndListBox, SW_HIDE); + SendMessageW(ac->hwndListBox, LB_RESETCONTENT, 0, 0); + } }
static void destroy_autocomplete_object(IAutoCompleteImpl *ac) { ac->hwndEdit = NULL; + free_enum_strs(ac); if (ac->hwndListBox) DestroyWindow(ac->hwndListBox); IAutoComplete2_Release(&ac->IAutoComplete2_iface);
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/shell32/autocomplete.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 5a31259..22f6ab8 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -608,10 +608,15 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam, hide_listbox(This, This->hwndListBox); return 0; case WM_KILLFOCUS: - if ((This->options & ACO_AUTOSUGGEST) && ((HWND)wParam != This->hwndListBox)) + if (This->options & ACO_AUTOSUGGEST) { - hide_listbox(This, This->hwndListBox); + if ((HWND)wParam == This->hwndListBox) break; + ShowWindow(This->hwndListBox, SW_HIDE); + SendMessageW(This->hwndListBox, LB_RESETCONTENT, 0, 0); } + + /* Reset the enumerator if it's not visible anymore */ + if (!IsWindowVisible(hwnd)) free_enum_strs(This); break; case WM_KEYDOWN: return ACEditSubclassProc_KeyDown(This, hwnd, uMsg, wParam, lParam);
This is needed for auto-append only AutoComplete controls.
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 22f6ab8..8a622b7 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -458,6 +458,8 @@ static void autocomplete_text(IAutoCompleteImpl *ac, HWND hwnd, enum autoappend_ { if (ac->options & ACO_AUTOSUGGEST) hide_listbox(ac, ac->hwndListBox); + else + free_enum_strs(ac); return; }
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com --- dlls/shell32/tests/autocomplete.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/dlls/shell32/tests/autocomplete.c b/dlls/shell32/tests/autocomplete.c index 66bd472..8265bf9 100644 --- a/dlls/shell32/tests/autocomplete.c +++ b/dlls/shell32/tests/autocomplete.c @@ -274,6 +274,7 @@ struct string_enumerator WCHAR **data; int data_len; int cur; + UINT num_resets; UINT num_expand; WCHAR last_expand[32]; }; @@ -349,6 +350,7 @@ static HRESULT WINAPI string_enumerator_Reset(IEnumString *iface) struct string_enumerator *this = impl_from_IEnumString(iface);
this->cur = 0; + this->num_resets++;
return S_OK; } @@ -456,6 +458,7 @@ static void test_aclist_expand(HWND hwnd_edit, void *enumerator) 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}; + obj->num_resets = 0;
ok(obj->num_expand == 0, "Expected 0 expansions, got %u\n", obj->num_expand); SendMessageW(hwnd_edit, WM_SETTEXT, 0, (LPARAM)str1); @@ -464,12 +467,14 @@ static void test_aclist_expand(HWND hwnd_edit, void *enumerator) 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)); + ok(obj->num_resets == 1, "Expected 1 reset, got %u\n", obj->num_resets); 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)); + ok(obj->num_resets == 2, "Expected 2 resets, got %u\n", obj->num_resets); SetFocus(hwnd_edit); SendMessageW(hwnd_edit, WM_CHAR, '_', 1); SendMessageW(hwnd_edit, WM_CHAR, 'b', 1); @@ -479,20 +484,24 @@ static void test_aclist_expand(HWND hwnd_edit, void *enumerator) SendMessageW(hwnd_edit, WM_CHAR, 'q', 1); dispatch_messages(); ok(obj->num_expand == 2, "Expected 2 expansions, got %u\n", obj->num_expand); + ok(obj->num_resets == 2, "Expected 2 resets, got %u\n", obj->num_resets); 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)); + ok(obj->num_resets == 3, "Expected 3 resets, got %u\n", obj->num_resets); 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)); + ok(obj->num_resets == 4, "Expected 4 resets, got %u\n", obj->num_resets); 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); + ok(obj->num_resets == 5, "Expected 5 resets, got %u\n", obj->num_resets); }
static void test_custom_source(void) @@ -502,6 +511,7 @@ static void test_custom_source(void) 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 }; + struct string_enumerator *obj; IUnknown *enumerator; IAutoComplete2 *autocomplete; HWND hwnd_edit; @@ -516,6 +526,7 @@ static void test_custom_source(void) ok(hr == S_OK, "CoCreateInstance failed: %x\n", hr);
string_enumerator_create((void**)&enumerator, suggestions, ARRAY_SIZE(suggestions)); + obj = (struct string_enumerator*)enumerator;
hr = IAutoComplete2_SetOptions(autocomplete, ACO_AUTOSUGGEST | ACO_AUTOAPPEND); ok(hr == S_OK, "IAutoComplete2_SetOptions failed: %x\n", hr); @@ -528,11 +539,14 @@ static void test_custom_source(void) 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)); + ok(obj->num_resets == 1, "Expected 1 reset, got %u\n", obj->num_resets); 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)); + ok(obj->num_resets == 1, "Expected 1 reset, got %u\n", obj->num_resets); + obj->num_resets = 0;
/* hijack the window procedure */ HijackerWndProc_prev = (WNDPROC)SetWindowLongPtrW(hwnd_edit, GWLP_WNDPROC, (LONG_PTR)HijackerWndProc); @@ -545,6 +559,7 @@ static void test_custom_source(void) 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)); + ok(obj->num_resets == 1, "Expected 1 reset, got %u\n", obj->num_resets); SendMessageW(hwnd_edit, EM_SETSEL, 0, -1); SendMessageW(hwnd_edit, WM_CHAR, '\b', 1); dispatch_messages(); @@ -558,6 +573,7 @@ static void test_custom_source(void) 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)); + ok(obj->num_resets == 2, "Expected 2 resets, got %u\n", obj->num_resets); /* end of hijacks */
test_aclist_expand(hwnd_edit, enumerator);
On Wed, Oct 31, 2018 at 01:24:28PM +0200, Gabriel Ivăncescu wrote:
Windows doesn't reset and re-enumerate it everytime autocompletion happens, and it also sorts the strings. This matches it more closely and makes it more useable on large lists as well.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
v4: Simplified it more and culled many variables.
dlls/shell32/autocomplete.c | 193 ++++++++++++++++++++++++++++++++------------ 1 file changed, 143 insertions(+), 50 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index b3f86f3..5a31259 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -25,7 +25,6 @@
implement ACO_FILTERPREFIXES style
implement ACO_RTLREADING style
implement ResetEnumerator
- string compares should be case-insensitive, the content of the list should be sorted
*/
#include "config.h" @@ -62,6 +61,8 @@ typedef struct LONG ref; BOOL initialized; BOOL enabled;
- UINT enum_strs_num;
- WCHAR **enum_strs; HWND hwndEdit; HWND hwndListBox; WNDPROC wpOrigEditProc;
@@ -103,10 +104,88 @@ static void set_text_and_selection(IAutoCompleteImpl *ac, HWND hwnd, WCHAR *text CallWindowProcW(proc, hwnd, EM_SETSEL, start, end); }
+static int enumerate_strings_cmpfn(const void *a, const void *b) +{
- return strcmpiW(*(WCHAR* const*)a, *(WCHAR* const*)b);
+}
+static void enumerate_strings(IAutoCompleteImpl *ac) +{
- /*
Enumerate all of the strings and sort them in the internal list.We don't free the enumerated strings (except on error) to avoid needlesscopies, until the next reset (or the object itself is destroyed)- */
Please move this comment to above the function.
- UINT i, cur = 0, array_size = 1024;
- LPOLESTR *strs = NULL, *tmp;
- ULONG n;
- do
- {
if ((tmp = heap_realloc(strs, array_size * sizeof(*strs))) == NULL)goto fail;strs = tmp;doIEnumString_Next(ac->enumstr, array_size - cur, &strs[cur], &n);while (n != 0 && (cur += n) < array_size);array_size *= 2;- } while (n != 0);
Hopefully you agree that this looks much nicer than the previous versions. There's a slight issue though in that you should check that the return value from IEnumString_Next() == S_OK. You could simply then set n = 0 if that condition isn't met, to break out of the loops. Also, let's change 'n' -> 'read'.
- /* Allocate even if there were zero strings enumerated, to mark it non-NULL */
- if ((tmp = heap_realloc(strs, cur * sizeof(*strs))))
- {
strs = tmp;if (cur > 0)qsort(strs, cur, sizeof(*strs), enumerate_strings_cmpfn);ac->enum_strs = strs;ac->enum_strs_num = cur;return;- }
+fail:
- i = cur;
'i' is superfluous, you can directly decrement 'cur'.
- while (i--)
CoTaskMemFree(strs[i]);- heap_free(strs);
+}
+static UINT find_first_matching_enum_str(IAutoCompleteImpl *ac, WCHAR *text, UINT len) +{
- WCHAR **strs = ac->enum_strs;
- UINT index = ~0, a = 0, b = ac->enum_strs_num;
- while (a < b)
- {
UINT i = (a + b - 1) / 2;int cmp = strncmpiW(text, strs[i], len);if (cmp == 0) index = i;if (cmp <= 0) b = i;else a = i + 1;- }
- return index;
+}
+static void free_enum_strs(IAutoCompleteImpl *ac) +{
- WCHAR **strs = ac->enum_strs;
- if (strs)
- {
UINT i = ac->enum_strs_num;ac->enum_strs = NULL;while (i--)CoTaskMemFree(strs[i]);heap_free(strs);- }
+}
static void hide_listbox(IAutoCompleteImpl *ac, HWND hwnd) { ShowWindow(hwnd, SW_HIDE); SendMessageW(hwnd, LB_RESETCONTENT, 0, 0);
- free_enum_strs(ac);
}
static void show_listbox(IAutoCompleteImpl *ac, UINT cnt) @@ -240,7 +319,11 @@ static LRESULT change_selection(IAutoCompleteImpl *ac, HWND hwnd, UINT key)
static BOOL do_aclist_expand(IAutoCompleteImpl *ac, WCHAR *txt, WCHAR *last_delim) {
- WCHAR c = last_delim[1];
- WCHAR c;
- free_enum_strs(ac);
- IEnumString_Reset(ac->enumstr); /* call before expand */
- c = last_delim[1]; last_delim[1] = '\0'; IACList_Expand(ac->aclist, txt); last_delim[1] = c;
@@ -276,6 +359,9 @@ static BOOL aclist_expand(IAutoCompleteImpl *ac, WCHAR *txt) while (i--) if (strchrW(delims, txt[i])) return do_aclist_expand(ac, txt, &txt[i]);
/* Windows doesn't expand without a delim, but it does reset */free_enum_strs(ac);}
return FALSE;
@@ -312,60 +398,60 @@ static BOOL display_matching_strs(IAutoCompleteImpl *ac, WCHAR *text, UINT len, HWND hwnd, enum autoappend_flag flag) { /* Return FALSE if we need to hide the listbox */
- UINT cpt;
- WCHAR **str = ac->enum_strs;
- UINT cnt, a, b, i;
- if (!str) return (ac->options & ACO_AUTOSUGGEST) ? FALSE : TRUE;
- if (ac->options & ACO_AUTOSUGGEST)
- {
SendMessageW(ac->hwndListBox, WM_SETREDRAW, FALSE, 0);SendMessageW(ac->hwndListBox, LB_RESETCONTENT, 0, 0);- }
- for (cpt = 0;;)
- if (len) {
HRESULT hr;LPOLESTR strs = NULL;ULONG fetched;hr = IEnumString_Next(ac->enumstr, 1, &strs, &fetched);if (hr != S_OK)break;if (!strncmpiW(text, strs, len))
i = find_first_matching_enum_str(ac, text, len);if (i == ~0)return (ac->options & ACO_AUTOSUGGEST) ? FALSE : TRUE;if (flag == autoappend_flag_yes)autoappend_str(ac, text, len, str[i], hwnd);if (!(ac->options & ACO_AUTOSUGGEST))return TRUE;/* Find the last string that begins with text, starting the search from i,which is guaranteed to find at least one string (if none other, then i) */a = i, b = ac->enum_strs_num;do {
if (cpt == 0 && flag == autoappend_flag_yes){autoappend_str(ac, text, len, strs, hwnd);if (!(ac->options & ACO_AUTOSUGGEST)){CoTaskMemFree(strs);break;}}if (ac->options & ACO_AUTOSUGGEST)SendMessageW(ac->hwndListBox, LB_ADDSTRING, 0, (LPARAM)strs);cpt++;}CoTaskMemFree(strs);
UINT mid = (a + b - 1) / 2;if (!strncmpiW(text, str[mid], len))a = mid + 1;elseb = mid;} while (a < b);
So this search loop looks like the code in find_first_matching...(). I'd suggest making that function more general, and having a take a parameter 'BOOL direction' parameter that would find the last in the list if set. You could also pass the start and end indicies to avoid unnecessary searching in this case.
}
- if (ac->options & ACO_AUTOSUGGEST)
- else {
if (cpt){show_listbox(ac, cpt);SendMessageW(ac->hwndListBox, WM_SETREDRAW, TRUE, 0);}else
if (!(ac->options & ACO_AUTOSUGGEST))return TRUE;i = 0;a = ac->enum_strs_num;if (a == 0) return FALSE;- cnt = a - i;
- SendMessageW(ac->hwndListBox, WM_SETREDRAW, FALSE, 0);
- SendMessageW(ac->hwndListBox, LB_RESETCONTENT, 0, 0);
- SendMessageW(ac->hwndListBox, LB_INITSTORAGE, cnt, 0);
- do
SendMessageW(ac->hwndListBox, LB_INSERTSTRING, -1, (LPARAM)str[i]);- while (++i < a);
- show_listbox(ac, cnt);
- SendMessageW(ac->hwndListBox, WM_SETREDRAW, TRUE, 0); return TRUE;
}
There's still a lot of code shuffling in this patch in dispatch_matching_strs(). Can you try to reduce that by more patch splitting? At the moment there's just too much going on to clearly see what this patch is doing.
static void autocomplete_text(IAutoCompleteImpl *ac, HWND hwnd, enum autoappend_flag flag) { WCHAR *text;
BOOL expanded = FALSE; UINT size, len = SendMessageW(hwnd, WM_GETTEXTLENGTH, 0, 0);
if (flag != autoappend_flag_displayempty && len == 0)
@@ -382,28 +468,35 @@ 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;
expanded = aclist_expand(ac, text);- }
- if (expanded || !ac->enum_strs)
- {
if (!expanded) IEnumString_Reset(ac->enumstr);enumerate_strings(ac);
- /* Set txtbackup to point to text itself (which must not be released) */
/* Set txtbackup to point to text itself (which must not be released),
and it must be done here since aclist_expand uses it to track changes */heap_free(ac->txtbackup); ac->txtbackup = text;
if (!display_matching_strs(ac, text, len, hwnd, flag))
hide_listbox(ac, ac->hwndListBox);
- {
/* Hide the listbox, but do not clear the enum strs, to match Windows */ShowWindow(ac->hwndListBox, SW_HIDE);SendMessageW(ac->hwndListBox, LB_RESETCONTENT, 0, 0);- }
So you moved this to a helper as I suggested and then moved it back in the next patch?? You could add a BOOL flag to hide_listbox that performs the reset if it's set.
Huw.
On Thu, Nov 01, 2018 at 09:57:37AM +0000, Huw Davies wrote:
On Wed, Oct 31, 2018 at 01:24:28PM +0200, Gabriel Ivăncescu wrote:
- UINT i, cur = 0, array_size = 1024;
- LPOLESTR *strs = NULL, *tmp;
- ULONG n;
- do
- {
if ((tmp = heap_realloc(strs, array_size * sizeof(*strs))) == NULL)goto fail;strs = tmp;doIEnumString_Next(ac->enumstr, array_size - cur, &strs[cur], &n);while (n != 0 && (cur += n) < array_size);array_size *= 2;- } while (n != 0);
Hopefully you agree that this looks much nicer than the previous versions. There's a slight issue though in that you should check that the return value from IEnumString_Next() == S_OK. You could simply then set n = 0 if that condition isn't met, to break out of the loops.
Sorry, actually test using FAILED(). IEnumString_Next() is supposed to return S_FALSE if the number read < number asked for.
Huw.
On Thu, Nov 1, 2018 at 11:57 AM Huw Davies huw@codeweavers.com wrote:
Hopefully you agree that this looks much nicer than the previous versions. There's a slight issue though in that you should check that the return value from IEnumString_Next() == S_OK. You could simply then set n = 0 if that condition isn't met, to break out of the loops. Also, let's change 'n' -> 'read'.
Yeah, I actually thought about how to simplify it with a do...while, when you gave me the idea to check for n != 0. :-)
'i' is superfluous, you can directly decrement 'cur'.
Sorry, just a habit of mine. I'm used to do iterations with such variables rather than changing the "original" (in this case it doesn't matter obviously).
So this search loop looks like the code in find_first_matching...(). I'd suggest making that function more general, and having a take a parameter 'BOOL direction' parameter that would find the last in the list if set. You could also pass the start and end indicies to avoid unnecessary searching in this case.
Well if it's alright, I'll only add a parameter for the 'start' index, because the 'end' is always the same in both cases; I want to avoid adding too many parameters, because in a later pending patch it will have to find based on potential prefix filtering (so I'll add another parameter), and I want to avoid cluttering the call-site with too many parameters which makes it ugly.
I can also simplify the function and just set "cmp" to "direction" when they compare equal, so I don't have to duplicate the logic (since that's the only situation in which the direction matters).
There's still a lot of code shuffling in this patch in dispatch_matching_strs(). Can you try to reduce that by more patch splitting? At the moment there's just too much going on to clearly see what this patch is doing.
I'm afraid not. Try to look at the whole function to get an idea why it's done this way. Maybe git's diff made it seem like there's more changes than a total rewrite of the function, but it's actually a rewrite.
The previous code added items to the listbox as soon as they got enumerated. In this one we add them at the end after we find the first and last items (since they are sorted).
If you are referring to the else {} block, that just handles the case where the len is zero and we display *all* the items. It has to be done in this patch because it's specific to the enumeration, and it is also needed to not break the behavior compared to before when len == 0.
So you moved this to a helper as I suggested and then moved it back in the next patch?? You could add a BOOL flag to hide_listbox that performs the reset if it's set.
Huw.
If you are referring to the previous 2 patches, I didn't touch hide_listbox at all. I can add a BOOL flag to reset, but I didn't want to change every call of hide_listbox just for one or two cases that need it set to FALSE.
That said there's still a problem with this: since the reset doesn't exist before this patch, should I add a normal hide_listbox (with "broken" behavior that always resets) in this patch, and then in the next patch convert it to hide_listbox with a BOOL flag? Is that alright, or every patch needs to be correct by itself?
If that's not acceptable (i.e. "temporary" slightly broken behavior), I don't know how else to split it up; prior to this patch, there's no "reset" at all. So every hide_listbox call-site would have to be changed in this patch, which makes it larger... and I wanted to avoid that.
On Thu, Nov 01, 2018 at 02:23:50PM +0200, Gabriel Ivăncescu wrote:
So you moved this to a helper as I suggested and then moved it back in the next patch?? You could add a BOOL flag to hide_listbox that performs the reset if it's set.
That said there's still a problem with this: since the reset doesn't exist before this patch, should I add a normal hide_listbox (with "broken" behavior that always resets) in this patch, and then in the next patch convert it to hide_listbox with a BOOL flag? Is that alright, or every patch needs to be correct by itself?
If that's not acceptable (i.e. "temporary" slightly broken behavior), I don't know how else to split it up; prior to this patch, there's no "reset" at all. So every hide_listbox call-site would have to be changed in this patch, which makes it larger... and I wanted to avoid that.
Adding the extra param to hide_listbox in this patch will make the patch bigger, but it's a simple change so it's easy to review.
Huw.
On Thu, Nov 01, 2018 at 02:23:50PM +0200, Gabriel Ivăncescu wrote:
On Thu, Nov 1, 2018 at 11:57 AM Huw Davies huw@codeweavers.com wrote:
There's still a lot of code shuffling in this patch in dispatch_matching_strs(). Can you try to reduce that by more patch splitting? At the moment there's just too much going on to clearly see what this patch is doing.
I'm afraid not. Try to look at the whole function to get an idea why it's done this way. Maybe git's diff made it seem like there's more changes than a total rewrite of the function, but it's actually a rewrite.
Well obviously I am looking at the whole function...
Perhaps it'll look better once the 'search for last' bit is moved out. Avoiding variable names like a and b will help too. 'cnt' could be removed while you're at it.
Huw.
On Thu, Nov 1, 2018 at 3:09 PM Huw Davies huw@codeweavers.com wrote:
On Thu, Nov 01, 2018 at 02:23:50PM +0200, Gabriel Ivăncescu wrote:
On Thu, Nov 1, 2018 at 11:57 AM Huw Davies huw@codeweavers.com wrote:
There's still a lot of code shuffling in this patch in dispatch_matching_strs(). Can you try to reduce that by more patch splitting? At the moment there's just too much going on to clearly see what this patch is doing.
I'm afraid not. Try to look at the whole function to get an idea why it's done this way. Maybe git's diff made it seem like there's more changes than a total rewrite of the function, but it's actually a rewrite.
Well obviously I am looking at the whole function...
Perhaps it'll look better once the 'search for last' bit is moved out. Avoiding variable names like a and b will help too. 'cnt' could be removed while you're at it.
Huw.
Yeah a and b are removed, but I don't know about cnt since it's needed for show_listbox, and i is incremented before that when adding the items to the listbox. So the ability to calculate the count is lost by that point. Sure I can save the original 'i' but that's no different than just setting cnt directly.
Is cnt really a problem though? I thought it makes the code a bit more descriptive in this case; i.e. makes it known exactly what i and a are supposed to be (well now I named them i and k, is that alright?).
-
Gabriel Ivăncescu -
Huw Davies