windows: mouse wheel scroll broken since v605

[avih]

Specifically, mouse wheel scroll is broken on Windows since 7b977a2 ("Added support for UTF-8 commands on Windows (#261)") up to and including current master (v635).

@Konstantin-Glukhov since it's your commit, can you (or someone else) confirm that this commit broke mouse wheel? any idea how to fix it?

I debugged it a bit (at this exact commit), and this line:
7b977a2#diff-1de596521a6bd2aaf938f5f87ef7be66407a9e4ee616f3f923574a9a1337a88cR2874

x11mousebuf[2] = X11MOUSE_OFFSET + (((int)ip.Event.MouseEvent.dwButtonState < 0) ? X11MOUSE_WHEEL_DOWN : X11MOUSE_WHEEL_UP);

is still getting the negative/non-negative value correctly on scroll down/up, so it does not seem like an issue of incorrect dwButtonState value.

So I couldn't put my finger on anything which looks wrong, but it doesn't scroll. Scrolling the wheel also seems to mess with the prompt (the status line).

In the the previous commit ("open v604") it scrolls fine and the prompt is fine too.

I didn't try to follow it further.

[Konstantin-Glukhov]

Could you be more specific which shell is affected and how it can be reproduced? I have no issue with the scroll using less version version 633 in Windows Terminal. Is it Dos shell C:\WINDOWS\system32\cmd.exe affected?

[Konstantin-Glukhov]

I just tested less version 590 with both cmd.exe and Windows Terminal. I do not see any difference in mouse scroll behavior with version 633. Windows Terminal scroll works fine and cmd.exe scroll is disabled when less is launched. I could not notice anything happening to the prompt either - it stays the same in both cmd.exe and Windows Terminal.

[avih]

Apologies if my description was not clear enough. Let me retry.

I just tested less version 590 with both cmd.exe and Windows Terminal. I do not see any difference in mouse scroll behavior with version 633. Windows Terminal scroll works fine and cmd.exe scroll is disabled when less is launched

What you're describing is when less is launched without --mouse, where indeed the windows console doesn't scroll its content by default, while the windows terminal does scroll its content.

But I was referring to mouse wheel scroll when invoking less --mouse. This makes less scroll some lines when scrolling the mouse wheel, similar to pressing the up/down arrows, with number of lines per wheel-scroll configurable via --wheel-lines.

So the issue happens to me both in windows terminal and in cmd.exe window, but let's focus on cmd.exe window, with these steps:

1. Run less -M --mouse file (the -M only to show long prompt/status-line).
2. Place the mouse pointer over the console window and scroll the mouse wheel down/up.

Expected result (and actual result up to v603):
The content displayed by less is scrolled down/up similar to how it would be scrolled with the up/down arrow keys (and the prompt/status line always stay at the bottom).

Actual reauls from in v605 and later:
The status line flickers, sometimes terminal bell is audible, and the content doesn't scroll.

Here's a screencast using the official v590 and v608 builds, in cmd.exe console on Windows 10:

less-mouse.v590-v608.mp4

[Konstantin-Glukhov]

Was there only my UTF change between v590 and v608? How did you narrow down the cause of the problem to my commit? I just want rule out the other changes before starting digging. The UTF change is quite simple. I do not see anything in the code that can affect the mouse besides the black box function ReadConsoleInputW(). There is no memory copy functions, only simple assignments to stack variables - we can rule out memory corruption here. Though I don't know what ReadConsoleInputW() does internally, I bet it messes with the mouse somehow.

[avih]

Was there only my UTF change between v590 and v608? How did you narrow down the cause of the problem to my commit

I (git) bisected it to your commit, compiling and testing in every step of the bisection.

I do not see anything in the code that can affect the mouse besides the black box function ReadConsoleInputW().

Same. I couldn't put my finger on anything which looks wrong, and yet, for me it breaks in exactly this commit.

Can you at least confirm it also breaks for you in this commit (or at least when comparing v590 to v608)?

[avih]

If you want, here are my builds of the earlier commit (less.v603-2-g1101069.exe), and the offending commit (less.v603-3-g7b977a2.exe).

They're with debug symbols, if you need, built using gcc 13.1 x86_64 from w64devkit.

less-v603-2-3.avih.zip

EDIT: reuploaded the same binaries but with different names. I previously used git describe to generate the name, but it included my build-script patches on top, so the hash was wrong. Now the hashes are the exact upstream hashes.

[Konstantin-Glukhov]

I think I found the fix. See my pull request #381.

[Konstantin-Glukhov]

On an unrelated subject, should funcs.h and help.c be added to the code base? It is hard to do development if the dependencies are missing. Also I have a suggestion: split Linux and Windows code into different files. This will make it way easier to maintain the code.

[avih]

should funcs.h and help.c be added to the code base?

funcs.h is generated at Makefile.wng (the mingw makefile) using grep and sed, so if you have those in your path (for instance, two copies of busybox.exe - busybox-w32 - one renamed to grep.exe and one renamed to sed.exe), and copy the relevant part from Makefile.wng to the visual studio makefile, it should generate funcs.h.

help.c is generated at Makefile.wng as

perl mkhelp.pl < less.hlp > help.c

If you don't have perl but do have sh and od (again, for instance from busybox-w32), then you can run this instead:

sh.exe mkhelp.sh < less.hlp > help.c

Where mkhelp.sh is:

#!/bin/sh

# same as mkhelp.pl, but using posix sh and od

od -b -v -A n | (
    d=$(LC_ALL=C date -u)
    echo "/* This file was generated by mkhelp.sh from less.hlp at $d */"
    echo '#include "less.h"'
    echo 'constant char helpdata[] = {'

    while read line; do  # octal values, typically 16 per line
        buf="   "  # without line buf it could be slow without builtin printf
        for o in $line; do
            buf="$buf 0$o,"
        done
        echo "$buf"
    done

    echo '    0 };'
    echo 'constant int size_helpdata = sizeof(helpdata) - 1;'
)

I think I found the fix. See my pull request #381.

Nice (didn't try it out yet).

Minor nits: there's some space/tab issue at the patch at the line of ascii = currentKey.ascii; and probably the whole block of // If multibyte character, return its first byte, and I think you removed the final newline at the file.

Off topic for the mouse wheel subject but relating to the unicode handling - I think that the unicode patch doesn't take surrogate pairs into account. I.e. it always tries to convert a single WCHAR into UTF-8, but any codepoint above U+FFFF (for instance emojis) is actually two WCHAR chars as surrogate pairs (because Windows Unicode is UTF16-LE), so I'd think that such unicode codepoints would not work correctly, though I didn't try it out.

[avih]

I think I found the fix. See my pull request #381.

Nice (didn't try it out yet).

I just tested it, and can confirm it fixes the issue of mouse wheel scroll for me too.

[Konstantin-Glukhov]

You have a point about handling Unicode. It was just the first shot at handling it. It will take some work if we want to handle all code bases. I have a few minor improvements/edge cases handling to add. I will send another pull request.

On the funcs.h and help.c - I think there should be some pure Windows solution, just using strait VC environment.

[avih]

n the funcs.h and help.c - I think there should be some pure Windows solution

I intend to send a PR for this, at least for Makefile.wng when some posix tools are available (all included in busybox-w32, cygwin, msys2, maybe gnu-win32, etc).

I don't know about pure windows solution. Do you mean like powershell? or compiling a C program to be used instead of these scripts? What else do you have on windows which can be used to generate funcs.h and help.c? Windows is pretty weak in the utilities and scripting departments...

But if someone adds a good solution, it could be nice, though personally I'm fine with the posix utilities solution.

[avih]

For future reference: the exact reason why the Unicode support effectively broke all mouse-related things is due to this sequence of events:

• WIN32getch() is called.
• It calls win32_kbhit(), and nothing is pending, so win32_kbhit sets both currentKey.ascii and currentKey.unicode to 0, and tries to read the next console input.
• The input happens to be a mouse event, so the x11mouse code wants to send a sequence of 6 bytes. It sets currentKey.ascii to the first byte (27 - ESC), and prepares a buffer of 5 more bytes which it intends to set up similarly (set currentKey.ascii) in future calls to win32_kbhit.
• After win32_kbhit returns, WIN32getch continues, and the unicode code checks whether currentKey.ascii is the same as currentKey.unicode, assuming that if they differ then it must mean it should convert currentKey.unicode to UTF-8, and return the UTF-8 bytes in the current and following calls to WIN32getch.
• Turns out they are indeed different. currentKey.ascii is 27 from the x11 mouse, and currentKey.unicode is 0 because that's how win32_kbhit resets it.
• Nevertheless, the unicode code converts this 0 value into UTF-8, and sets currentKey.ascii to the first byte of this UTF-8 sequence, and consequently WIN32getch returns this value - instead of the pending 27 (ESC) which the x11mouse code intended to be returned.
• The next time WIN32getch is called, and assuming the UTF-8 buffer is now empty, it calls win32_kbhit, and the x11 mouse code now sends its second byte (of the 6 it intended to send), and for exactly the same reason as before (the ascii value differs from the unicode value), the unicode code at WIN32getch overrides it yet again.
• Same goes for all the 4 additional bytes which the x11 mouse wanted to deliver. The unicode code simply replaced each x11mouse byte with a UTF-8 sequence Codepoint 0....

And so mouse never worked since unicode support was added...

The underlaying issue is that the unicode code at WIN32getch assumes that because .ascii and .unicode were originally overlapping union members of KEY_EVENT, they must represent the same value, but failing to take into account that the x11 mouse code set only one of them without them ever getting copied from the union at all - because it was a mouse event and not a key event...

A trivial fix would be to ensure that unicode is not 0, by changing this at WIN32getch:

		// If multibyte character, return its first byte
		if (currentKey.ascii != currentKey.unicode)

into this:

		// If multibyte character, return its first byte
		if (currentKey.unicode && currentKey.ascii != currentKey.unicode)

or even simpler, just test that it's indeed more than one UTF-8 byte:

		// If multibyte character, return its first byte
		if (currentKey.unicode > 127)

(confirmed fixing the mouse wheel issue)

[gwsw]

Fixed in 6fd6739 by avih's last suggestion.

[avih]

added the awaiting close label

Is it waiting for me to close it?

[gwsw]

I prefer that the original reporter do the close when possible, but I will do it eventually if you do not.

[avih]

I prefer that the original reporter do the close when possible

Sure. Different projects have different preferences... now I know ;)

Thanks for merging a fix.