eric pouech (@epo) commented about programs/cmd/wcmdmain.c:
> + len = lstrlenW(inputBuffer);
> + len2 = len;
> +
> + /* Update current input display in console */
> + set_cursor_visible(hOutput, FALSE);
> +
> + /* Calculate cursor position at beginning of prompt, accounting for lines scrolled
> + * due to length of input.
> + */
> + GetConsoleScreenBufferInfo(hOutput, ¤tConsoleInfo);
> + currentConsoleInfo.dwCursorPosition.X = startConsoleInfo.dwCursorPosition.X;
> + len2 -= (currentConsoleInfo.dwSize.X - currentConsoleInfo.dwCursorPosition.X);
> + if (len2 > 0) {
> + currentConsoleInfo.dwCursorPosition.Y -= ((len2 / currentConsoleInfo.dwSize.X) + 1);
> + }
> + SetConsoleCursorPosition(hOutput, currentConsoleInfo.dwCursorPosition);
did you try just reusing startConsoleInfo.dwCursorPosition here instead of recomputing the initial cursor position?
Actually I'm worried that we have to convert between cursor position from/to number of characters
- this makes assumptions about how the string is layed out (one vs multiple lines, one string character == one cell == one glyph on screen (this is not the case for CJK characters; if we support them one day))
- anyway, I don't see a simple straightforward way to do it (ReadConsole doesn't return the farthest cursor point where it has written to, and since it can be on multiple lines...).
- but the less code that does it, the better
the pain here comes from conhost wrapping the strings (in WriteConsole) larger than screen buffer width onto different lines... native use an unbounded line length and wraps it depending on window size.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/7843#note_104473
On Sun May 25 15:34:38 2025 +0000, Alexandros Frantzis wrote:
> Hi @yurih, from what I understand the reason alt(-tab) is problematic is
> because the alt menu mode is enabled when the key is released. However,
> avoiding the release just for this behavior doesn't seem like a viable
> way forward, since other applications may treat alt differently or may
> also require treating other keys the same way.
> I was experimenting a bit and sending WM_CANCELMODE (which winex11 does
> already) seems to help with the alt(-tab) menu situation in my tests.
> Could you try the following and let me know if it helps with your use case:
> ```patch
> --- a/dlls/winewayland.drv/wayland_keyboard.c
> +++ b/dlls/winewayland.drv/wayland_keyboard.c
> @@ -800,6 +800,11 @@ static void keyboard_handle_leave(void *data,
> struct wl_keyboard *wl_keyboard,
> * and for any key repetition to stop. */
> release_all_keys(hwnd);
>
> + if (hwnd == NtUserGetForegroundWindow())
> + {
> + if (!(NtUserGetWindowLongW(hwnd, GWL_STYLE) & WS_MINIMIZE))
> + send_message(hwnd, WM_CANCELMODE, 0, 0);
> + }
> /* FIXME: update foreground window as well */
> }
> ```
Yes it does indeed fix the alt menu mode without my patch. Though I don't think I understood clearly what you meant, as Alt is already filtered (VK_MENU), I just added the left and right keys as they are among the returned keyboard state and are not filtered, making Alt released anyway. You plan to remove the modifier's filtering altogether ?
Also, there's issues with extended scan code (for example my keyboard arrows) that aren't released, though it also happens with winex11.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/6199#note_104470
On Sun May 25 13:31:25 2025 +0000, Jayson Reis wrote:
> Hi there @epo thanks for the review!
> As I could not find any auto formatting tool configured, I tried
> maintaining the formatting around the code I was changing, do you mind
> point me out where did you find it different?
> I will change the calls to GetProcAddress, as there is no public header,
> this means that on console.c I cannot call the Wide function from ascii
> and just convert, right? Meaning I would need to implement the logic twice?
> About being racy, reading the docs I did not see anything special about
> it, it seems like a wrapper around the PeekInputConsole to be less
> blocking, something gives you the feel it should be reimplemented?
> Also, is it a good idea to copy the exe for tests and run on Windows to
> see how it goes?
> At least this showed something to me that my test and implementation
> seem to be wrong like when calling the function with nowait, and it is
> empty, the return should be false but GetLastError seems to be 0
You can call a function without public header, just move that prototype to console.c (either near the top, or just before ExA - I don't know what Wine code style says about that).
The race happens if two threads read from that console simultaneously, both with nowait; Peek returns for both, thread 1 calls Read, and thread 2 calls Read too and blocks because it's empty.
Tests should be run on Windows, yes. That's how we discover which behavior to implement in Wine.
We usually SetLastError(0xdeadbeef) before calling something that should (or shouldn't) set the last error, to make sure we're not seeing a leftover error from a previous operation.
If PeekW returns false, then you should return false too, not think you're in the *count=0 branch and return true.
The code style issue is probably the // comments, we use only /* in Wine. (Also it's spelled that, not tha.)
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/8137#note_104466
Hi there @epo thanks for the review!
As I could not find any auto formatting tool configured, I tried maintaining the formatting around the code I was changing, do you mind point me out where did you find it different?
I will change the calls to GetProcAddress, as there is no public header, this means that on console.c I cannot call the Wide function from ascii and just convert, right? Meaning I would need to implement the logic twice?
About being racy, reading the docs I did not see anything special about it, it seems like a wrapper around the PeekInputConsole to be less blocking, something gives you the feel it should be reimplemented?
Also, is it a good idea to copy the exe for tests and run on Windows to see how it goes?
At least this showed something to me that my test and implementation seem to be wrong like when calling the function with nowait, and it is empty, the return should be false but GetLastError seems to be 0
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/8137#note_104465
a couple of comments:
* please read and conform to [Wine Coding style](https://gitlab.winehq.org/wine/wine/-/wikis/Wine-Developer's-Guide/Coding-Practice#some-notes-about-style)
* AFAICT ReadConsoleInputEx\[AW\] isn't defined in Windows SDK so header files shouldn't be changed
* tests must reflect what Windows does, not what your implementation does (hence the generated failures in CI pipeline)
* this should be implemented using proper (new or modified) IOCTL:s; there's no way this API can be implemented on top of kernel32's one without being racy (as your proposal is). That's the reason why MS introduced this new API.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/8137#note_104458