Fix windows: mouse wheel scroll broken since v605 #379

[Konstantin-Glukhov]

Separate mouse and keyboard events.

[avih]

Thanks, I can confirm this fixes the mouse wheel scroll issue for me.

Nit: the new code is added with space-indenting, while the surrounding code uses tab indenting.

	currentKey.unicode = ip.Event.KeyEvent.uChar.UnicodeChar;
	currentKey.ascii = ip.Event.KeyEvent.uChar.AsciiChar;

While this is not part of the current patch, and I guess it works, I believe C doesn't guarantee that ip.Event.KeyEvent.uChar.AsciiChar is the lower bits of ip.Event.KeyEvent.uChar.UnicodeChar (the value was set to union member UnicodeChar by ReadConsoleInpoutW, but the code reads it from the union member AsciiChar), so this would probably be a better assignment:

currentKey.ascii = currentKey.unicode & 0xff;

(or even & 0x7f, because char is signed, so technically values above 127 would be implementation-defined behavior, but also, WCHAR values above 127 represent at least two UTF-8 chars, so above 127 is not comparable to char at all)

[avih]

As I've learned recently, if WIN32getch returns 0, then it's an indication that the next value which WIN32getch returns should be interpreted as a keyboard scan code rather than ascii/UTF8 byte.

So maybe win32_kbhit should just return FALSE if it can't convert it to UTF8?

[Konstantin-Glukhov]

Yes, this "pending scan" logic is a little bit weird to me.
My fix is not working at all. It only works for scrolling a mouse wheel :-) I need to think more...
I also ran into a problem with Unicode surrogate pairs, though I think I almost found a solution.

[avih]

My fix is not working at all

Can you describe what the problem actually is? I.e. why mouse wheel got broken in that commit (or at the current code)?

I also ran into a problem with Unicode surrogate pairs

IMHO this issue is not urgent or critical. Being able to enter non-BMP codepoints like emoji is nice, but probably doesn't really stand in anyone's way if it doesn't work.

So if the solution for typing emojis carries risks or too much complexity, it might be better off without it IMHO.

[avih]

Yes, this "pending scan" logic is a little bit weird to me.

It's quite similar to how unicode works: ReadConsoleInput[W] delivers a value that doesn't quite fit into one byte, so WIN32getch splits it into two: 0 and the scan code, delivered one after the other in two consecutive calls.

The "buffer" is currKey.scan (one byte is enough), and the equivalent of utf8_current_byte < utf8_size is pending_scancode.

You can see that the unicode and the scancode are very similar at the begining of WIN32getch.

In fact, they might benefit from a shared infrastructure of queuing a sequence of return values. Currently unicode and scancode each have its own mechanism for basically the same thing.

[Konstantin-Glukhov]

The fact that AsciiChar is signed char does not mean it is treated as a signed number. Which characters above 127 are UTF8 that are not correctly saved in AsciiChar? I want to test this theory.

[avih]

Which characters above 127 are UTF8 that are not correctly saved in AsciiChar?

What do you mean not correctly saved? What is not correctly saved?

I want to test this theory.

What theory?

All the codepoints which are not ASCII-7 are above 127, for instance any non-English European or Asian character ends up as a sequence of UFT-8 bytes which are all bigger than 127.

I don't know what the context of the question is, the fact that it's stored in signed char doesn't necessarily mean it gets broken. But casting values between signed and unsigned, especially with different storage sizes, can sometimes break things.

[avih]

In fact, they might benefit from a shared infrastructure of queuing a sequence of return values. Currently unicode and scancode each have its own mechanism for basically the same thing.

Actually, x11mousebuf is also exactly the same thing - it translate a mouse event from ReadConsoleInput into a sequence of return values for WIN32getch.

So we have the mouse events which are queued at x11mousebuf, and the remaining length is at x11mouseCount.

We have unicode which is queued as UTF-8 at utf8[UTF8_MAX_LENGTH]; and its length is maintained at utf8_size - utf8_next_byte.

And we have scan code which is buffered at currKey.scan, and its "length" is maintained at pending_scancode.

I really do think that a unified system of queuing a sequenc e of return values would make these three systems much much simpler.

[Konstantin-Glukhov]

Maybe I misunderstood what you said about AsciiChar and UnicodeChar:

I believe C doesn't guarantee that ip.Event.KeyEvent.uChar.AsciiChar is the lower bits of ip.Event.KeyEvent.uChar.UnicodeChar (the value was set to union member UnicodeChar by ReadConsoleInpoutW, but the code reads it from the union member AsciiChar), so this would probably be a better assignment:

currentKey.ascii = currentKey.unicode & 0xff;

The way I interpret your statement is that AsciiChar != (UnicodeChar 0xFF). How is it possible if they are unioned in memory?

[avih]

Maybe I misunderstood what you said about AsciiChar and UnicodeChar:

Ah, now I understand :)

The way I interpret your statement is that AsciiChar != (UnicodeChar 0xFF).

No. I said that C doesn't guarantee this. It might or might not be equal.

How is it possible if they are unioned in memory?

If the architecture is Big Endian, then AsciiChar will probably be the most significant byte of UnicodeChar, but if it's Little Endian, then it will probably be the lower byte.

I don't think there's a Windows system which is Big Endian, so I don't think it's an issue.

But what is the purpose of that test? to check whether it exceeds 8 bits in value? then you could do, for instance:

if (UnidoceChar != (unsigned char)UnicodeChar)
    ...

or something with & ~0x7f or whatever.

Accessing a union member which was not set is a bit of a gray area.

[avih]

If you want to test whether it will be more than one UTF-8 byte (i.e. if you need to call WideCharToMultiByte(...)), then just check whether UnicodeChar is bigger than 127.

[Konstantin-Glukhov]

The reason the mouse wheel is broken because of the UTF code in WIN32getch() overwrites the value of currentKey.ascii variable. The UTF code needs to be in win32_kbhit() not to step on the mouse code.

I am puzzled why the mouse code is inside the loop and the key events code is outside the loop. Shouldn't they follow similar logic?

Also why are there two variables for the mouse buffer: x11mousePos and x11mouseCount? Isn't one enough to move around the buffer? Couldn't it be like:

if (x11mousePos < 5)
	{
		currentKey.ascii = x11mousebuf[x11mousePos++];
		keyCount = 1;
		return (TRUE);
	}

[avih]

The UTF code needs to be in win32_kbhit()

I agree.

I am puzzled why the mouse code is inside the loop and the key events code is outside the loop. Shouldn't they follow similar logic?

They do follow similar logic: if the code knows right away that it's going to ignore this INPUT_RECORD, then it will continue the loop. This happens with mouse move events, KEY_EVENT which is not "down", key events without either scan code or unicode, etc. It's not only for mouse.

Also why are there two variables for the mouse buffer: x11mousePos and x11mouseCount? Isn't one enough to move around the buffer? Couldn't it be like:

There's more than one way to write the same code. Your example will probably work fine too. It comes down to personal preference, and at this case I'm guessing the author preferred to keep it more generic, for instance if one day the mouse sequence could have different lengths.

[Konstantin-Glukhov]

About UnicodeChar vs AsciiChar: ReadConsoleInputW() writes to UnicodeChar and ReadConsoleInputA() writes to AsciiChar.
AsciiChar == (UnicodeChar 0xFF) is always true reguardless of the function: W or A. Union also works correctly reguardless Big or Little Endian. A compiler should guarantee this.

[Konstantin-Glukhov]

In which case wVirtualScanCode can be 0?

[avih]

I really do think that a unified system of queuing a sequenc e of return values would make these three systems much much simpler.

So I unified the unicode/scancode/x11mouse keyboard queues. All three need to return more than one value via WIN32getch(), and each maintained its own variable, queue, and system.

Now they all use the same queue.

Also moved the unicode code from WIN32getch to win32_kbhit where all the other code which converts ReadConsoleInput into one or more return values is.

This happens to fix the mouse wheel issue without any specific effort.

Also fixes repeat count (at the KEY_EVENT) which was not working for unicode.

The Unicode still doesn't handle surrogate pairs, and uses the exact same "system" as before: read one WCHAR and try to convert it to UTF-8.

The patch is the top commit here https://github.com/avih/less/commits/winkbq

For now I'm not opening a PR for it, but leaving it there if others want to have a look.

[Konstantin-Glukhov]

Is mouse supposed to do text selection and copy/paste?

[Konstantin-Glukhov]

I opened a new pull request #385 with the working code