On Sat Apr 26 15:12:31 2025 +0000, eric pouech wrote:
> 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)
In the latest changes I have already split out much code from WCMD_ReadConsole into subfunctions. You're saying it's still too long? I suppose that I could split out the code for the individual case statements into separate handler functions, but the code to handle Tab is the bulk of that function.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101954
On Sat Apr 26 15:12:31 2025 +0000, eric pouech wrote:
> 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...)
I missed that case. I tested C:\win<tab> and C:\pro<tab> but resulting paths are different lengths.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101953
On Sat Apr 26 15:12:31 2025 +0000, eric pouech wrote:
> looks to be that curPos can be \> \*numRead, hence reading garbage in
> that case
Line 312 terminates the buffer at *numRead, line 314 sets len to *numRead, and loop at 320 stops when !(curPos < len). Worst case is if curPos == len then we are checking the null terminator.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101952
On Sat Apr 26 17:10:07 2025 +0000, Joe Souza wrote:
> Paths need to be quoted if they are long file names or they contain
> spaces, hence the check for findData.cAlternateFileName.
To do what you're asking here we would need to track whether each level in the path requires quotes, not just the current filename.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101950
On Sat Apr 26 15:12:32 2025 +0000, eric pouech wrote:
> I think quote handling is not done as it should
> from what I see on native: quotes are added when generated word does
> contain characters that would need to be escaped (eg ' ', but didn't
> check all the others)
> need quote is an attribute of current filename (ie they are removed when
> the new (partial) matching file no longer needs them) ; so quotes
> shouldn't be part of the SEARCH_CONTEXT, but only used for display
> (basically: go to beg of word; if needs quotes, print quote; print
> (partial) word; print ending quotes (if needed)
Paths need to be quoted if they are long file names or they contain spaces, hence the check for findData.cAlternateFileName.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101949
didn't play a lot with resulting code, mostly reviewed it
note: dir "c:\\Programs Files\\"\<tab\> should circle among the subdirectories... current code doesn't
also dir c\<tab\> (with a first file displayed), then reduce size of console window to be shorter than input line, will display input on line after the prompt
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_101938