windows: mouse column >= 95 broken by "bogus key down" (regression)

[avih]

Mouse event (click, wheel, etc) at row or column 95 or higher generates unexpected key presses which mess things up.

This is a regression of 010d808 which removed the ascii key field and unified the ascii/unicode values.

Mouse row/column >= 95 ends up as byte value >= 128 (x11 offset + column + 1 where x11 offset is 32 and column >= 95) at the mouse report string (^[M<b><x><y>).

In turn, the code at WIN32getch thinks this is a non-ascii value which is part of whatever, and generates incorrect unicode keypresses using put_wchar, and things get messed up quickly (incorrect jumps at the document, bell, whatever).

Seriously, the sooner someone rewrites this mess of buffers that is the windows KB/mouse/unicode handling code, the better. It's seriously hard to write new code which fits with the current code without introducing subtle bugs, as evident by the bugs which pop up frequently whenever someone touches it (and it's also hard to bisect quickly because half the revisions around it don't build due to the missing cast thing).

[avih]

That's the workaround I'm using in my local build, by introducing yet another state variable. Meh:

diff --git a/screen.c b/screen.c
index 9b17556..6686cd1 100644
--- a/screen.c
+++ b/screen.c
@@ -147,7 +147,7 @@ static int keyCount = 0;
 static WORD curr_attr;
 static int pending_scancode = 0;
 static char x11mousebuf[] = "[M???";    /* Mouse report, after ESC */
-static int x11mousePos, x11mouseCount;
+static int x11mousePos, x11mouseCount, isx11mouse;
 static int win_unget_pending = FALSE;
 static int win_unget_data;
 
@@ -2881,8 +2881,10 @@ public int win32_kbhit(void)
 		currentKey.unicode = x11mousebuf[x11mousePos++];
 		--x11mouseCount;
 		keyCount = 1;
+		isx11mouse = 1;
 		return (TRUE);
 	}
+	isx11mouse = 0;
 
 	/*
 	 * Wait for a real key-down event, but
@@ -3011,7 +3013,7 @@ public char WIN32getch(void)
 		}
 		keyCount--;
 		/* If multibyte character, return its first byte */
-		if (is_ascii_char(currentKey.unicode))
+		if (is_ascii_char(currentKey.unicode) || isx11mouse)
 			ascii = (char) currentKey.unicode;
 		else
 		{

A probably more correct solution would be to use a single unified queue for KB/mouse/UTF8 output bytes, and hopefully that should make everything simpler and much less susceptible to subtle bugs.

[gwsw]

Fixed in d5ede93.
I have rewritten the WIN32C input code to replace most of the state variables with a single queue. Please let me know if you see any issues.

[avih]

The mouse issue in column 95+ is fixed.

The new code looks generally OK (not in a review way, I only glanced at it), with few gotchas:

There appears to be a regression when pressing / to search, and then pasting 开开心心过每一天 and then holding down left arrow for a while, and then right arrow for a while: in few revisions back in master, the cursor would end up at the begining/end as expected, while in current master there seem to be added keys entered at the search prompt.

Can you confirm this regression?

I think WIN32ungetch is wrong, because it uses win32_unget_queue which I believe is the input side of a FIFO (e.g. the mouse thing does win32_unget_queue(ESC); win32_unget_queue('['); ...), but WIN32ungetch should insert at the output side of the FIFO, I.e. if the queue is not empty, then the newly-ungot char will currently be the last to read, while it should be the first.

I think the terminology win32_get_queue and win32_unget_queue is misleading, because it sounds as if one of them cancels the other, but in fact they operate on different sides of the fifo as far as I can tell. The former dequeues and the latter enqueues, but it sounds like a pop/push thing to me

I think i'd have implemented the queue differently (probably static char buffer of 32 or some such, and a repeat count var - it never holds more than one utf8/mouse-report/scan-thingy/etc, possibly with repeat count), but it doesn't really matter. If it works then it works.

Nit without practical impact, put_wchar can write up to 6 bytes, but it's used on a buffer of 4 - UTF8_MAX_LENGTH.

If I think of more things, I'll post here.

One thing I noticed when I also implemented a unified KB queue few months ago, is that if the content is slow to render for whatever reason, then it's better if the repeat count is limited to 2 or 3, because otherwise the queue can keep growing forever - because it feeds keys faster than they can be processed, continuously, and then it becomes very laggy and can take a while till new input is handled.

Dropping arbitrary keys is iffy, because the user could be typing etc, but clipping the repeat count is unlikely to have negative impact, so in my implementation I found that to behave better.

I'll leave it up to you to decide when to close this issue.

[avih]

Another regression - #378 is back in master.

I thought it's the bad unget, so I applied this patch:

diff --git a/screen.c b/screen.c
index 13a05cc..270464f 100644
--- a/screen.c
+++ b/screen.c
@@ -2806,9 +2806,9 @@ static int win32_queued_char(void)
 }
 
 /*
- * Push a char onto the back of the win32_queue.
+ * Push a char onto the input side of the win32_queue FIFO.
  */
-static void win32_unget_queue(char ch)
+static void win32_enqueue(char ch)
 {
 	WIN32_CHAR *wch = (WIN32_CHAR *) ecalloc(1, sizeof(WIN32_CHAR));
 	wch->wc_ch = ch;
@@ -2824,6 +2824,17 @@ static void win32_unget_queue(char ch)
 	}
 }
 
+/*
+ * Push a char onto the output side of the win32_queue (next get is it)
+ */
+static void win32_unget_queue(char ch)
+{
+	WIN32_CHAR *wch = (WIN32_CHAR *) ecalloc(1, sizeof(WIN32_CHAR));
+	wch->wc_ch = ch;
+	wch->wc_next = win32_queue;
+	win32_queue = wch;
+}
+
 /*
  * Get a char from the front of the win32_queue.
  */
@@ -2865,12 +2876,12 @@ static int win32_mouse_event(XINPUT_RECORD *xip)
 		return (FALSE);
 	}
 	/* {{ TODO: change to X11 1006 format. }} */
-	win32_unget_queue(ESC);
-	win32_unget_queue('[');
-	win32_unget_queue('M');
-	win32_unget_queue(b);
-	win32_unget_queue(X11MOUSE_OFFSET + xip->ir.Event.MouseEvent.dwMousePosition.X + 1);
-	win32_unget_queue(X11MOUSE_OFFSET + xip->ir.Event.MouseEvent.dwMousePosition.Y + 1);
+	win32_enqueue(ESC);
+	win32_enqueue('[');
+	win32_enqueue('M');
+	win32_enqueue(b);
+	win32_enqueue(X11MOUSE_OFFSET + xip->ir.Event.MouseEvent.dwMousePosition.X + 1);
+	win32_enqueue(X11MOUSE_OFFSET + xip->ir.Event.MouseEvent.dwMousePosition.Y + 1);
 	return (TRUE);
 }
 
@@ -2962,8 +2973,8 @@ static int win32_scan_code(XINPUT_RECORD *xip)
 	 * An extended key returns a 2 byte sequence consisting of
 	 * a zero byte followed by the scan code.
 	 */
-	win32_unget_queue('\0');
-	win32_unget_queue(scan);
+	win32_enqueue('\0');
+	win32_enqueue(scan);
 	return (TRUE);
 }
 
@@ -2997,7 +3008,7 @@ static int win32_key_event(XINPUT_RECORD *xip)
 		char *p;
 		put_wchar(&up, xip->ichar);
 		for (p = utf8; p < up; ++p)
-			 win32_unget_queue(*p);
+			 win32_enqueue(*p);
 	}
 	return (TRUE);
 }

But that did not fix it. Not sure what's going on...

[avih]

I noticed that the old code actively ignored, or at least referred to scan codes PCK_CAPS_LOCK and PCK_NUM_LOCK, while the new code doesn't test for them. I don't know if this has any practical impact, but I think whatever the old code wanted to achieve about them - is not happening now.

[gwsw]

The regression to #378 should be fixed in ec906cb. I had attempted a simple fix to #435 but that didn't work.

I'm having trouble debugging Unicode issues because my cmd window is inexplicably failing to render many Unicode characters, such as the Euro sign U+20AC and many others, even if I set chcp 65001. This has nothing to do with less; if I just type a file containing a Euro sign it renders as a question mark. I could swear this worked previously so I'm not sure what's going on. When I figure this out I'll investigate the hanzi search regression you mention.

I used a dynamic queue because repeat counts are not predictable, although if I limit repeat to 2 or 3 as you suggest it could be feasible to use a static queue.

[gwsw]

My Unicode problem seems to be related to the fact that I was using a raster font. Changing to SimSun-ExtB makes it behave better.

However I cannot reproduce the problem you report with arrowing around a string of Chinese characters. Is it easy to reproduce for you? I've tried holding down left arrow until it reaches the beginning of the string and then holding down right arrow until it reaches the end several dozen times and I don't see extra chars appear.

BTW I should probably remove support for 5 and 6 byte UTF-8 strings from put_wchar. This code was written before Unicode was changed to only support values up to U+10FFFF in 2003.

[avih]

The regression to #378 should be fixed in ec906cb

Confirmed fixed.

I'm having trouble debugging Unicode issues because my cmd window is inexplicably failing to render many Unicode characters

My Unicode problem seems to be related to the fact that I was using a raster font. Changing to SimSun-ExtB makes it behave better.

Yes, it's a font issue, but more than that, the native windows conhost.exe (that's the window in which cmd.exe runs) uses GDI rendering, which doesn't support font fallback, so the font you choose need to have all the glyphs you're expecting. Typically courier-new is reasonably big, but it silll won't have all glyphs.

I recommend trying the new Windows Terminal, or the new conhost which is shipped with it (called OpenConsole.exe at the zip/install dir - just double click it to open the new cmd prompt window). You can download one of the zipped releases and extract only OpenConsole.exe - it's stand alone, and does support font fallback and other improvements (and is the future conhost in Windows, developed by the terminal/console team at MS).

I used a dynamic queue because repeat counts are not predictable, although if I limit repeat to 2 or 3 as you suggest it could be feasible to use a static queue.

Even if repeat is big, it's just a number to repeat the buffer. It's not different data on each iteration...

However I cannot reproduce the problem you report with arrowing around a string of Chinese characters. Is it easy to reproduce for you?

Yes with 5dcf7b5, no (fixed) with ec906cb.

So as far as I can tell there are no remaining bugs which I know of.

I'm leaving the queue implementation, repeat clipping, and handling of PCK_CAPS_LOCK/PCK_NUM_LOCK up to you, as well as closing this issue.

[gwsw]

Ok, good. I'll review the remaining issues and decide what to do.

The repeat issue is that (at least the way I implemented it), a large repeat count does add a large number of entries to the queue, since each entry is just one byte. If I receive a non-ASCII char with a large repeat, I add multiple copies of the UTF-8 sequence to the queue. It would be possible just use a repeat count along with a variable somewhere that tells that N entries in the queue need to be repeated and another variable keeping track of a position within those N entries, and logic that steps through those N entries and doesn't remove them until the repeat count is exhausted, but it seemed simpler to do it this way.

[avih]

It would be possible just use a repeat count along with a variable somewhere that tells that N entries in the queue need to be repeated and another variable keeping track of a position within those N entries, and logic that steps through those N entries and doesn't remove them until the repeat count is exhausted, but it seemed simpler to do it this way.

Maybe. I think I'd try something like this (untested). whether it's simpler or not might be subjective. It might also be buggy :)

static char q[32];
static int qlen = 0, qpos = 0, qrepeat = 0, qug = -1;

void qreset() {  // unget is unaffected
    qlen = qpos = qrepeat = 0;
}

void qunget(char ch) {
    qug = (unsigned char)ch;
}

void qhas() {
    return qrepeat || qpos < qlen || qug >= 0;
}

void qadd(char ch) {
    q[qlen++] = ch;
}

void qmultiply(int n) {
    qrepeat = n - 1;
}

int qget() {
    if (qug >= 0) {
        int ret = qug;
        qug = -1;
        return ret;
    }
    if (qpos < qlen)
        return (unsigned char)q[qpos++];
    if (qrepeat > 0) {
        --qrepeat;
        qpos = 0;
        return qget();
    }
    return -1;
}

[gwsw]

I dunno, I'd worry about unforeseen interactions between the repeat variable and other actions. Like, could WIN32ungetch() get called while repeat is >1, so the repeat starts applying to the wrong char? I don't know, and I'd prefer not to have to do that analysis every time this code is changed. That kind of interaction between state variables is why the code was so hard to manage before this rewrite.

[avih]

Like, could WIN32ungetch() get called while repeat is >1, so the repeat starts applying to the wrong char?

Not a problem. Unget here is independent of the actual buffer, like it was before your rewrite.

The usage is: reset, fill buffer if has key/mouse/etc event, set repeat ("multiply") count if not 1 (1 is the default), consume/unget while not empty, rinse and repeat.

I'd prefer not to have to do that analysis every time this code is changed

Sure.

That kind of interaction between state variables is why the code was so hard to manage before this rewrite.

I wouldn't say so. The reason it was hard to manage is because it grew "organically", where each new functionality added its own states and buffers which were handled in different places, and the interaction between the states becames too subtle to work with.

In contrast, this is a single black box of 20 LOC which was written with the generalized requirements in mind - for what we know now and for other future "push X bytes and repeat them N times". you verify it once and then use it according to its specification.

But anyway, as I said, if it works (and I think master does) then it's fine. It's more personal preference than anything else.

[gwsw]

Ok, thanks. I'm satisfied with the current code.

[avih]

Right. Thanks for the refactor, and for less :)