April 2025 - Wine-GitLab - list.winehq.org

Re: [PATCH v21 0/1] MR7843: cmd: Implement tab completion for command line entry.
by eric pouech (＠epo) April 26, 2025

April 26, 2025

eric pouech (@epo) commented about programs/cmd/lineedit.c: > + } else { > + cc = len; > + while (cc > 0 && inputBuffer[cc] != L' ' && inputBuffer[cc] != L'\t') { > + cc--; > + } > + } > + } > + > + if (cc != sc->searchPos || inputBuffer[sc->searchPos] == L'\\') { > + cc++; > + } > + > + sc->insertPos = cc; > +} > + > +static void FindNextMatchingDirectoryEntry(const WCHAR *searchstr, PSEARCH_CONTEXT sc) a couple of comments here: * actually, this is not exactly how native (Win10) behaves: native captures the list of matching files at first tab usage, and doesn't update that list if eg. a file matching the pattern is added * the way done here is a bit more dynamic (as it would capture file creation/deletion), and I don't think it matters much from a user perspective the main thing I'm concerned about is that code to handle backwards shift-tab will be a bit more painful, and also the restart case (when the matched file is deleted) I think code would be simpler and more readable when storing the list of files, but I won't block on that. I let you pick up your favorite one. if you keep current logic, it seems to me that: * \-\>hadOneMatch is only used locally, and should rather be a local variable * \-\>hasSearchResult would make more sense be return value from this function -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101936

1 0

Re: [PATCH v21 0/1] MR7843: cmd: Implement tab completion for command line entry.
by eric pouech (＠epo) April 26, 2025

April 26, 2025

eric pouech (@epo) commented about programs/cmd/lineedit.c: > + > + FindInsertPos(inputBuffer, curPos, &sc); > + } > + > + /* Copy search results to input buffer. */ > + UpdateInputBuffer(inputBuffer, inputBufferLength, &sc); > + > + /* Update cursor position to end of buffer. */ > + curPos = lstrlenW(inputBuffer); > + if (curPos > maxLen) { > + maxLen = curPos; > + } > + } > + break; > + > + case L'\x1B': /* ESC */ looks at that question, and you're right... ReadConsole needs to handle ESC. I'll send a patch later on, so can drop ESC handling from your patch -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101934

1 0

Re: [PATCH v21 0/1] MR7843: cmd: Implement tab completion for command line entry.
by eric pouech (＠epo) April 26, 2025

April 26, 2025

eric pouech (@epo) commented about programs/cmd/lineedit.c: > + } > + oldCurPos = curPos; > + curPos = 0; > + while (curPos < len && > + inputBuffer[curPos] != L'\t' && > + inputBuffer[curPos] != L'\x1B' > + ) { > + > + curPos++; > + } > + /* curPos is often numRead - 1, but not always, as in the case where history is retrieved > + * and then user backspaces to somewhere mid-string and then hits Tab. > + */ > + TRACE("numRead: %lu, curPos: %u\n", *numRead, curPos); > + > + switch (inputBuffer[curPos]) { looks to be that curPos can be \> \*numRead, hence reading garbage in that case -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101933

1 0

Re: [PATCH v21 0/1] MR7843: cmd: Implement tab completion for command line entry.
by eric pouech (＠epo) April 26, 2025

April 26, 2025

eric pouech (@epo) commented about programs/cmd/lineedit.c: > + /* curPos is often numRead - 1, but not always, as in the case where history is retrieved > + * and then user backspaces to somewhere mid-string and then hits Tab. > + */ > + TRACE("numRead: %lu, curPos: %u\n", *numRead, curPos); > + > + switch (inputBuffer[curPos]) { > + case L'\t': > + TRACE("TAB: [%s]\n", wine_dbgstr_w(inputBuffer)); > + inputBuffer[curPos] = L'\0'; > + len = curPos; > + sc.haveSearchResult = FALSE; > + > + /* See if we need to reset an existing search. */ > + if (sc.hSearch != INVALID_HANDLE_VALUE) { > + /* If tab key was hit at a different location than last time */ > + if (curPos != oldCurPos) { that's not sufficient, you have also to compare that inputbuffer up to current pos hasn't changed since previous call (eg directory with za, za2 and zb zb2 files... circle twice from "za", replace "za" by "zb" and circle with tab...) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101932

1 0

Re: [PATCH v21 0/1] MR7843: cmd: Implement tab completion for command line entry.
by eric pouech (＠epo) April 26, 2025

April 26, 2025

eric pouech (@epo) commented about programs/cmd/lineedit.c: > + csbi.dwCursorPosition.X = 0; /* First column in the row. */ > + cCount -= chars; > + } > +} > + > +static void SetCursorVisible(const HANDLE hOutput, const BOOL visible) > +{ > + CONSOLE_CURSOR_INFO cursorInfo; > + > + if (GetConsoleCursorInfo(hOutput, &cursorInfo)) { > + cursorInfo.bVisible = visible; > + SetConsoleCursorInfo(hOutput, &cursorInfo); > + } > +} > + > +static void FindSearchPos(const WCHAR *inputBuffer, int len, PSEARCH_CONTEXT sc) same remark as above: please use WCMD_parameter to loop across words and determine which word contains current cursor positiion -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101931

1 0

Re: [PATCH v21 0/1] MR7843: cmd: Implement tab completion for command line entry.
by eric pouech (＠epo) April 26, 2025

April 26, 2025

eric pouech (@epo) commented about programs/cmd/lineedit.c: > + BOOL ignoreTrailingQuote; > + BOOL isDirSearch; > + BOOL haveSearchResult; > + BOOL hadOneMatch; > + int lastStartQuote; > + int searchPos; > + int insertPos; > +} SEARCH_CONTEXT, *PSEARCH_CONTEXT; > + > + > +static BOOL IsDirectoryOperation(const WCHAR *inputBuffer, const ULONG inputBufferLength) > +{ > + int cc = 0; > + BOOL ret = FALSE; > + > + while (cc < inputBufferLength && (inputBuffer[cc] == L' ' || inputBuffer[cc] == L'\t')) { you need to handle '@' as well -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101929

1 0

Re: [PATCH v21 0/1] MR7843: cmd: Implement tab completion for command line entry.
by eric pouech (＠epo) April 26, 2025

April 26, 2025

eric pouech (@epo) commented about programs/cmd/lineedit.c: > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA > + */ > + > +#define WIN32_LEAN_AND_MEAN > + > +#include "wcmd.h" > +#include "wine/debug.h" > + > +WINE_DEFAULT_DEBUG_CHANNEL(cmd); > + > +typedef struct _SEARCH_CONTEXT > +{ > + HANDLE hSearch; in new code, we tend not to use camel case for function names or fields but rather snake case -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101930

1 0

Re: [PATCH v21 0/1] MR7843: cmd: Implement tab completion for command line entry.
by eric pouech (＠epo) April 26, 2025

April 26, 2025

eric pouech (@epo) commented about programs/cmd/lineedit.c: > +/* I don't think it's worth it to create another source file for only \~400 LoC -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101928

1 0

Re: [PATCH v21 0/1] MR7843: cmd: Implement tab completion for command line entry.
by eric pouech (＠epo) April 26, 2025

April 26, 2025

eric pouech (@epo) commented about programs/cmd/lineedit.c: > +typedef struct _SEARCH_CONTEXT > +{ > + HANDLE hSearch; > + WIN32_FIND_DATAW findData; > + BOOL haveQuotes; > + BOOL ignoreTrailingQuote; > + BOOL isDirSearch; > + BOOL haveSearchResult; > + BOOL hadOneMatch; > + int lastStartQuote; > + int searchPos; > + int insertPos; > +} SEARCH_CONTEXT, *PSEARCH_CONTEXT; > + > + > +static BOOL IsDirectoryOperation(const WCHAR *inputBuffer, const ULONG inputBufferLength) I'd like to see the code for specific handling of first word in a separate commit (just for clarity and make review easier) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101927

1 0

Re: [PATCH v21 0/1] MR7843: cmd: Implement tab completion for command line entry.
by eric pouech (＠epo) April 26, 2025

April 26, 2025

thanks a couple of high level comments: * I don't think having a separate source file just for \~400 LoC is worth it * WCMD_ReadConsole is way too big as a function and need to be split in smaller pieces. * you should also be using the command line parsing functions instead of rewriting them (eg WCMD_parameter to loop against the various parameters) from quick testing: * when looping around files sometimes filename is inserted in quotes, while it shouldn't. Native seems to insert quotes only when filename contains specific characters (spaces...), but to also remove them if next file doesn't contain any... * using "cd c:\\Program Files"\<tab\>\<backspace\>\\\<tab\> doesn't behave as native... native circles in the directory content, current MR only adds a double quote * got a crash when playing with MR (perhaps one of the buffer overflow stated above, perhaps not) * when the width of the console is smaller than the full line input (eg spread at least on two lines), circling gives some strange result (cursor and display when from bottom of screen buffer back to top, will previous content not shifted) likely the current appoach of just having a handle to findfirst/findnext will not fit all the (long term) goals. at some point, we want to support shift-tab which circles backwards in the matching files. and I fail to see what using FindFirstFileEx brings as value, as you filter against directory when needed (I'm not asking for supporting shift-tab right now, but rather to put in place a structure were it wil fit simply when we implement it) support for cd/rmdir/ and friends should be added in a second patch I didn't go further in the review given the amount a remarks and problems stated above. I'll restart when they are solved (will look in // to the ESC issue raised) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101926

1 0