[PATCH v4 1/4] shell32/autocomplete: Implement a cache and sort the enumerated strings for proper behavior
31 Oct
2018
31 Oct
'18
11:24 a.m.
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(a)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);
--
1.9.1
31 Oct
31 Oct
11:24 a.m.
New subject: [PATCH v4 2/4] shell32/autocomplete: Don't reset the enumerator when losing focus if the edit control is still visible
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)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);
--
1.9.1
11:24 a.m.
New subject: [PATCH v4 3/4] shell32/autocomplete: Reset the enumerator when the listbox is empty even when auto-suggest is disabled
This is needed for auto-append only AutoComplete controls.
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)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;
}
--
1.9.1
11:24 a.m.
New subject: [PATCH v4 4/4] shell32/tests: Test the amount of enumerator resets for AutoComplete
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)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);
--
1.9.1
1 Nov
1 Nov
9:57 a.m.
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(a)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)
+ */
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;
+
+ do
+ IEnumString_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;
+ else
+ b = 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.
10:21 a.m.
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:
Sorry, actually test using FAILED(). IEnumString_Next() is supposed to
return S_FALSE if the number read < number asked for.
Huw.
+ 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);
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.
12:23 p.m.
On Thu, Nov 1, 2018 at 11:57 AM Huw Davies <huw(a)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.
12:35 p.m.
On Thu, Nov 01, 2018 at 02:23:50PM +0200, Gabriel Ivăncescu wrote:
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.
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.
1:09 p.m.
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(a)codeweavers.com> wrote:
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.
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.
3:04 p.m.
On Thu, Nov 1, 2018 at 3:09 PM Huw Davies <huw(a)codeweavers.com> wrote:
On Thu, Nov 01, 2018 at 02:23:50PM +0200, Gabriel Ivăncescu wrote:
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?).
On Thu, Nov 1, 2018 at 11:57 AM Huw Davies
<huw(a)codeweavers.com> wrote:
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. 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.
2013
days inactive
2014
days old
9 comments
2 participants
participants (2)
-
Gabriel Ivăncescu -
Huw Davies