windows: mouse sometimes stops working with piped input

[avih]

Test case (using regexp.c from the less source code):

• In cmd.exe shell, run type regexp.c | less --mouse
• Scroll the mouse wheel down once or twice to confirm it scrolls the document (and not the terminal).
• Press G to go to the end of the document.
• Scroll the mouse wheel up.

Expected result: The document is scrolled backwards (and the prompt/status line stays at the bottom of the screen).

Actual result: No scroll (or the terminal is scrolled, depending on the terminal). Mouse buttons (left/right click) also don't work.

Some data points:

• Mouse is broken immediately if using type regexp.c | less +Gg --mouse.
• Also happens when using LESSOPEN in pipe mode (| type %s or | source-highlight ... %s - the latter is the main reason for filing this issue).
• No issue with less [+Gg] --mouse < regexp.c.
• No issue with busybox cat regexp.c | less +Gg --mouse (using busybox-w32).
• After the mouse stops working, typing --mouse in less results in Ignore mouse input (press RETURN) and after ENTER the mouse is indeed still not working, but then typing --mouse again results in Use the mouse for scrolling (press RETURN), and after ENTER the mouse is working again.

So it looks like the mouse is de-initialized when using G with piped input (but not from busybox cat, and not with redirected input?), but not re-initialized once the G command completes.

[avih]

Hmm.. I don't think it's missing init. Both init/deinit and init_mouse/deinit_mouse are called exactly once, nested correctly both when the issue happens and when the issue doesn't happen.

Not sure what else can break the mouse handling...

Can you reproduce the issue?

[gwsw]

I can reproduce this. It appears that when it gets in this mode, PeekConsoleInputW is returning 0 even when mouse events occur. I'm not sure what's going on either. I'll try to look into this later today.

[gwsw]

What I see is that init_mouse changes the console mode to 0x91 ( ENABLE_PROCESSED_INPUT | ENABLE_MOUSE_INPUT | ENABLE_EXTENDED_FLAGS). Calling GetConsoleMode in win32_kbhit confirms that this value is set. However when EOF is reached, GetConsoleMode suddenly starts returning 0x87. This happens if you press G or just scroll down to the end of the file. I've instrumented every call to SetConsoleMode in less and it is not getting called. I wonder if type is setting the console mode when it completes and that is interfering with less? That would explain why it doesn't happen with cat or redirected stdin.

[avih]

I wonder if type is setting the console mode when it completes and that is interfering with less? That would explain why it doesn't happen with cat or redirected stdin.

Well, I don't know what type does, but it also happens with gnu source-highlight (which I built myself), and there's no SetConsole at the codebase...

Also, I think SetConsoleMode is supposed to be a per-process thing, but not 100% sure.

At one stage I thought it might be related to CRLF line endings (regexp.c is checked out with unix line ending on my system, but I thought maybe type or source-highlight produce CRLF), but even when piping it through cat | unix2dos there's no issue.

[gwsw]

I would have expected SetConsoleMode to be per-process, but I don't see how it's getting changed. I've never tried running less under the Visual C debugger, but maybe doing that would help determine if SetConsoleMode is somehow being called from within less. I'm not sure if the debugger allows setting a breakpoint on a system call that's not part of the source.

[avih]

You could always #define SetConsoleMode my_set_console_mode and make all calls go through a function which you control...

[avih]

Also, if you prefer gdb, then you can try out w64devkit. No installation required, just unzip, double click the main exe (which will open a busybox-w32 shell), then run make -f Makefile.wng at the shell, then you can use gdb - which is shipped with w64devkit.

And quick tip about the shell: it's a fairly complete posix shell, and busybox-w32 has all the important *nix utils (which also have shortcuts at the bin/ dir of w64devkit). You can use forward slashes in paths, but there's no virtual root path of /. you need to use e.g. cd d:/foo/bar and then all the relative paths are in drive d, until you cd into another drive. Of couse, things like make -C x:/whatever ... work too.

[avih]

For what it's worth, I went back as far as I can build less, which is v540 from 2018, and the issue already exists there.

[gwsw]

The problem does not appear to ever happen with a 4096 byte input file, but always happens immediately (no movement required) with a 4097 byte file. 4096 is not the size of any buffer used in less as far as I know (LBUFSIZE is 8192).

Instrumenting various places in the code shows that it usually happens in the forw_line_seg function, but at a random place in that function that varies each time it is run. And AFAICT forw_line_seg does not do anything that should cause the console mode to change. This nondeterministic behavior is suggestive that it's happening outside of less. I could work around the problem by resetting the console mode in every call to win32_kbhit or something like that, although that's pretty ugly.

@adoxa Jason, pinging you in case you have any ideas.

[avih]

Some more test cases which fail (executed in busybox sh, type is the cmd.exe command, the rest are sh):

cmd /c type regexp.c | cat | LESS= ./less  --mouse
cmd /c type regexp.c | printf %s\\n "$(cat)" | LESS= ./less  --mouse
cmd /c type regexp.c | (printf %s\\n "$(cat)"; sleep 1) | LESS= ./less  --mouse
cmd /c type regexp.c | (x=$(cat); sleep 1; printf %s\\n "$x") | LESS= ./less  --mouse

And two which succeed:

cmd /c type regexp.c | (sleep 1; LESS= ./less  --mouse)
cmd /c type regexp.c | (x=$(cat); sleep 1; printf %s\\n "$x") | (sleep 1; LESS= ./less  --mouse)

So it looks as if type does something which less doesn't like if it's running while less is running, but if less is launched after type terminates, then it works.

However, as I said, this also happens with source-highlight, and I don't see any terminal console API used at all. And rightfully so, especially when it outputs to a pipe...

It would be useful to be able to write a type-like minimal program in C to reproduce it. source-highlight is a huge beast and it won't be possible to make a minimal test case out of it.

[gwsw]

Another approach would be to try to write a minimal less-like program that exhibits the behavior when type or source-highlight is piped into it.

Is there anything like strace in Windows that would show what system calls type is calling? Most of the evidence points to type doing something that less doesn't expect, like changing the console mode. However the source-highlight failure doesn't fit with that theory, so it's puzzling.

[avih]

well, less-like minimal program which uses the mouse would be quite bigger than type-like program :)

[gwsw]

I notice the WIN32C version of pclose in less (ttyin.c lines 127-142) manually resets the console mode, and has the comment "Close the pipe, restoring the keyboard (CMD resets it, losing the mouse)". I think Jason wrote that code. This seems to hint at the idea that the console mode get changed when a pipe closes.

[gwsw]

well, less-like minimal program which uses the mouse would be quite bigger than type-like program :)

True, but we know exactly what less is doing, while we can only guess at what type is doing.

[avih]

The problem does not appear to ever happen with a 4096 byte input file

IIRC that's BUFSIZ on windows?

However the source-highlight failure doesn't fit with that theory, so it's puzzling.

Right, and we don't actually care about type specifically.

I'm noticing that with source-highlight it only happens with LESSOPEN. I don't recall if I reproduced it with normal pipe, and I can't now (but it still happens with LESSOPEN).

It's also possible that these are two issues (type/SH), even if they have the same symptoms.

For reference, I'm attaching my build of source-highlight: source-highlight.rel_3_1_9-8-ge4cf32d+4@avih--win64--2023-09-28.2.zip (just extract it someplace).

I'm using it like so (in cmd.exe shell):

set "LESSOPEN=| d:/path/to/bin/source-highlight.exe --failsafe --infer-lang -f esc --style-file=esc.style -i %s"
.\less.exe -R --mouse regexp.c

Initially the mouse works, but after G (or if launching with +Gg, or if scrolling with the mouse till the bottom) the mouse doesn't work.

Can you reproduce it?

[gwsw]

Yes, I can reproduce. I also note that it only happens when using a piped LESSOPEN. With a non-piped LESSOPEN there is no issue. I think this is all pointing to a problem where Windows changes the console mode when a program writing into a pipe terminates.

[avih]

This seems to hint at the idea that the console mode get changed when a pipe closes.

Right, maybe that would be the place to reset/reinit the console?

The source-highlight test case (only happens when reaching [near] the bottom) can definitely be "when the pipe closes".

But also, it doesn't happen with cat or source-highlight with normal pipe. How come only with LESSOPEN?

[avih]

But also, it doesn't happen with cat or source-highlight with normal pipe. How come only with LESSOPEN?

I think that's because LESSOPEN probably uses popen, but as far as I can tell pclose is not called when reaching EOF. Instead, as far as I can tell it's only called when less exits, after deinit.

So the terminal restoration doesn't happen when it should?

As for strace for windows, not that I'm aware of, but I was pointed to these: https://learn.microsoft.com/en-us/sysinternals/downloads/procmon http://www.rohitab.com/apimonitor

The first is fairly reputable, no idea about the second.

[avih]

I tried building v542 (where mouse support was added, with that terminal-reset-on-pclose) and v543, but I can't get the mouse working at all.

64 bit build crashes when using LESSOPEN, 32 bit works, and I also tried without LESSOPEN - I just can't get the mouse wheel scroll to work (when launched as less --mouse regexp.c when LESSOPEN is set or unset)

[gwsw]

Well, this (setting the console mode in every call to win32_kbhit) seems to fix it, but I'm not at all convinced that this is the right approach. There may also still be a tiny race, where win32_kbhit sets the console mode and then Windows resets it before PeekConsoleMode is called.

diff --git a/screen.c b/screen.c index 90f7d88..a738896 100644 --- a/screen.c +++ b/screen.c @@ -178,7 +178,8 @@ static int sy_fg_color; /* Color of system text (before less) */ static int sy_bg_color; public int sgr_mode; /* Honor ANSI sequences rather than using above */ #if MSDOS_COMPILER==WIN32C -static DWORD init_output_mode; /* The initial console output mode */ +public DWORD init_output_mode; /* The initial console output mode */ +public DWORD curr_output_mode; /* The current console output mode */ public int vt_enabled = -1; /* Is virtual terminal processing available? */ #endif #else @@ -1569,11 +1570,11 @@ static void win32_init_vt_term(void) if (vt_enabled == 0 || (vt_enabled == 1 && con_out == con_out_ours)) return; - GetConsoleMode(con_out, &output_mode); - vt_enabled = SetConsoleMode(con_out, - output_mode | ENABLE_VIRTUAL_TERMINAL_PROCESSING); + output_mode = curr_output_mode | ENABLE_VIRTUAL_TERMINAL_PROCESSING; + vt_enabled = SetConsoleMode(con_out, output_mode); if (vt_enabled) { + curr_output_mode = output_mode; auto_wrap = 0; ignaw = 1; } @@ -1699,8 +1700,8 @@ public void init_mouse(void) ltputs(sc_s_mousecap, sc_height, putchr); #else #if MSDOS_COMPILER==WIN32C - SetConsoleMode(tty, ENABLE_PROCESSED_INPUT | ENABLE_MOUSE_INPUT - | ENABLE_EXTENDED_FLAGS /* disable quick edit */); + curr_output_mode = ENABLE_PROCESSED_INPUT | ENABLE_MOUSE_INPUT | ENABLE_EXTENDED_FLAGS; /* disable quick edit */ + SetConsoleMode(tty, curr_output_mode); #endif #endif @@ -1716,8 +1717,8 @@ public void deinit_mouse(void) ltputs(sc_e_mousecap, sc_height, putchr); #else #if MSDOS_COMPILER==WIN32C - SetConsoleMode(tty, ENABLE_PROCESSED_INPUT | ENABLE_EXTENDED_FLAGS - | (console_mode & ENABLE_QUICK_EDIT_MODE)); + curr_output_mode = ENABLE_PROCESSED_INPUT | ENABLE_EXTENDED_FLAGS | (curr_output_mode & ENABLE_QUICK_EDIT_MODE); + SetConsoleMode(tty, curr_output_mode); #endif #endif } @@ -3034,6 +3035,7 @@ public int win32_kbhit(void) for (;;) { DWORD nread; + SetConsoleMode(tty, curr_output_mode); /* YUCK! */ PeekConsoleInputW(tty, &xip.ir, 1, &nread); if (nread == 0) return (FALSE); @@ -3102,4 +3104,3 @@ public void WIN32textout(char *text, int len) #endif } #endif - diff --git a/ttyin.c b/ttyin.c index e29bb48..e73e22c 100644 --- a/ttyin.c +++ b/ttyin.c @@ -23,8 +23,9 @@ #define _WIN32_WINNT 0x400 #endif #include -public DWORD console_mode; public HANDLE tty; +extern DWORD init_output_mode; +extern DWORD curr_output_mode; #else public int tty; #endif @@ -87,9 +88,9 @@ public void open_getchr(void) tty = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ, &sa, OPEN_EXISTING, 0L, NULL); - GetConsoleMode(tty, &console_mode); /* Make sure we get Ctrl+C events. */ - SetConsoleMode(tty, ENABLE_PROCESSED_INPUT | ENABLE_MOUSE_INPUT); + curr_output_mode = ENABLE_PROCESSED_INPUT | ENABLE_MOUSE_INPUT; + SetConsoleMode(tty, curr_output_mode); #else #if MSDOS_COMPILER extern int fd0; @@ -119,7 +120,7 @@ public void open_getchr(void) public void close_getchr(void) { #if MSDOS_COMPILER==WIN32C - SetConsoleMode(tty, console_mode); + SetConsoleMode(tty, init_output_mode); CloseHandle(tty); #endif } @@ -133,7 +134,7 @@ public int pclose(FILE *f) int result; result = _pclose(f); - SetConsoleMode(tty, ENABLE_PROCESSED_INPUT | ENABLE_MOUSE_INPUT); + SetConsoleMode(tty, curr_output_mode); return result; } #endif

[adoxa]

When cmd starts (including via popen) it gets the current mode, which it resets after every process it runs. Thus (presumably): cmd gets the current mode (without mouse); runs less; less turns on mouse; the pipe finishes; cmd closes, restoring the mode (turning off the mouse again). That's why the later sleep tests work - cmd closes before less runs.

[avih]

When cmd starts (including via popen) it gets the current mode, which it resets after every process it runs

Sounds plausible, and annoying.

Well, this (setting the console mode in every call to win32_kbhit) seems to fix it, but I'm not at all convinced that this is the right approach.

Agreed, maybe it should do that when piped input reaches EOF?

Regardless, two things:

You can quote code by placing it between two lines of triple-backticks, the first possibly followed by a syntax-highlight name (diff, sh, c etc), like so (but use backticks) (check the guidelines):

'''diff
<arbitrary multi line content>
'''

But also, it seems a fairly big diff. Why not something smaller, like so (tested to work):

diff --git a/screen.c b/screen.c
index b23c2d7..ddce114 100644
--- a/screen.c
+++ b/screen.c
@@ -3046,10 +3046,15 @@ static int win32_key_event(XINPUT_RECORD *xip)
 public int win32_kbhit(void)
 {
    XINPUT_RECORD xip;
+   DWORD mode;

    if (win32_queued_char())
        return (TRUE);

+   /* When an input pipe closes (normal or LESSOPEN), the mode may reset */
+   if (mousecap && GetConsoleMode(tty, &mode) && !(mode & ENABLE_MOUSE_INPUT))
+       SetConsoleMode(tty, mode | ENABLE_MOUSE_INPUT);
+
    for (;;)
    {
        DWORD nread;

or

diff --git a/screen.c b/screen.c
index b23c2d7..6abaaf9 100644
--- a/screen.c
+++ b/screen.c
@@ -3046,10 +3046,22 @@ static int win32_key_event(XINPUT_RECORD *xip)
 public int win32_kbhit(void)
 {
    XINPUT_RECORD xip;
+   DWORD mode;

    if (win32_queued_char())
        return (TRUE);

+   /* When an input pipe closes (normal or LESSOPEN), the mode may reset */
+   if ((mousecap || vt_enabled) && GetConsoleMode(tty, &mode))
+   {
+       DWORD m = mode
+               | (mousecap ? ENABLE_MOUSE_INPUT : 0)
+               | (vt_enabled ? ENABLE_VIRTUAL_TERMINAL_PROCESSING : 0);
+
+       if (m != mode)
+           SetConsoleMode(tty, m);
+   }
+
    for (;;)
    {
        DWORD nread;

As an anecdote, I instrumented it, and with type it actually sets the mode twice: once right on launch, and once when the bottom is reached. With LESSOPEN of source-highlight it's only set once - when reaching the bottom.

I think that's in line with the observation that with type it's broken right away sometimes.

[gwsw]

Yes I know about triple-backtick, but apparently it doesn't work with the < details> tag. I tried both nestings and neither worked.

In my change I did do some cleanup that's not strictly necessary to fix the problem. I don't have a strong feeling about it but slightly prefer my change.

Regarding doing the mode set only when piped input reaches EOF, that seems a bit trickier and riskier. How would I detect that the input is a pipe, rather than stdin or a fifo (not sure if Windows has fifos)? More to the point, I don't think Window's setting of the mode happens when less READS the EOF; I think it happens when the writer completes, which I don't think can be detected by less. If there's still buffered data in the pipe, less may not read EOF until long after Windows has set the mode.

[avih]

Yes I know about triple-backtick, but apparently it doesn't work with the < details> tag. I tried both nestings and neither worked.

IIRC you need an empty line after the opening details line, and/or empty line before the closing detail line. Also, there's a "preview" button to check how it looks before you post, and an "edit" button to modify it after you posted ;)

In my change I did do some cleanup that's not strictly necessary to fix the problem. I don't have a strong feeling about it but slightly prefer my change.

Right, I thought you might have done this, but it was not easy to read. I don't have a strong opinion either.

Regarding doing the mode set only when piped input reaches EOF, that seems a bit trickier and riskier. How would I detect that the input is a pipe, rather than stdin or a fifo (not sure if Windows has fifos)?

You can test (sb.st_mode & S_IFMT) == S_IFIFO after fstat(0, &sb) on Windows.

I don't think it there are filesystem fifos, but S_IFIFO detects a pipe.

More to the point, I don't think Window's setting of the mode happens when less READS the EOF; I think it happens when the writer completes, which I don't think can be detected by less. If there's still buffered data in the pipe, less may not read EOF until long after Windows has set the mode.

Yup. That's the real issue which I also realized after I suggested it.

[gwsw]

Ok, fixed via the discussed kludge in c7ffe20875920c2aa3a0945adfbdb33f45e72f19. Windows sucks.

[avih]

Ok, fixed via the discussed kludge in c7ffe20.

I don't know if it's right.

As far as I can tell, the new curr_console_mode is the combined input and output modes (even if documented as only the output mode).

However, according to ms docs the bits have different semantics when used with input/output console handles.

For instance ENABLE_VIRTUAL_TERMINAL_PROCESSING at the output handle (which is associated with vt_vnabled) is 0x4, which, with an input handle is ENABLE_ECHO_INPUT (which is why my second diff is incorrect - it wants to set both mouse and "vt output" on the input handle, but ends up setting echo input).

So I would think that independent input/output modes have to be maintained, and each used with the respective input/output console handle.

Also, while we haven't noticed issues with output modes getting reset when the piped command closes, it might be worth "fixing" the output mode as well, together with the input. Not 100% sure about that though, because the input handle was directly connected to where the command was executed, but the output handle is supposedly unrelated to the input pipe command. The right thing (TM) would be to test it, which we haven't done... (that I know of).

As for how to "fix" the mode, currently it sets it unconditionally on every $whatever.

My hunch is that it would be nicer to set it only if it doesn't match our expectation (what are those? only the set bits? or also the unset ones?). It shouldn't affect a good implementation, but a bad one might reset things unconditionally when the mode is set, even if it's the same as the previous mode.

So in retrospect, I think my first diff (if mouse is enabled but the mode doesn't have it, add mouse to the input mode) is the simplest and most correct solution: we only know that the mouse is broken, so if it should be enabled at the mode but isn't - enable it (and possibly move it into the for loop).

+   curr_console_mode = ENABLE_PROCESSED_INPUT | ENABLE_MOUSE_INPUT | ENABLE_EXTENDED_FLAGS; /* disable quick edit */

I don't think ENABLE_EXTENDED_FLAGS disables quick edit. In fact, I think it enables it. The docs of ENABLE_QUICK_EDIT_MODE are:

This flag enables the user to use the mouse to select and edit text. To enable this mode, use ENABLE_QUICK_EDIT_MODE | ENABLE_EXTENDED_FLAGS. To disable this mode, use ENABLE_EXTENDED_FLAGS without this flag.

And also above this table:

When a console is created, all input modes except ENABLE_WINDOW_INPUT and ENABLE_VIRTUAL_TERMINAL_INPUT are enabled by default.

Which, to me, reads as if both ENABLE_QUICK_EDIT_MODE and ENABLE_EXTENDED_FLAGS are set by default (and indeed quick edit works when opening a new console), and to disable quick edit one needs to unset the former (and set the latter).

And indeed, without --mouse, quick edit wasn't disabled before, and isn't disabled now (select to mark, enter to copy). With --mouse it was disabled before and still disabled now.

But then again, why do we want to disable quick edit at all? It's very useful, and personally It annoys me when it's disabled, because I frequently select-to-copy at the terminal, and when it's disabled one has to hold shift to select text.

Also, I didn't test it, but I suspect that, like xterm and other *nix terminals, selection/quick-edit gets disabled when the application enables mouse input.

So I think it's better to keep the default behavior here, without explicitly enabling or disabling quick edit.

EDIT: Ignore the extended flags comment. Removing ENABLE_EXTENDED_FLAGS (which is set only when the mouse is enabled) breaks the mouse. Not sure why, ENABLE_MOUSE_INPUT is not documented as requiring the extended flags... So I guess that's fine, but the comment should say that it's apparently needed for the mouse.

Windows sucks.

What doesn't...

Unrelated: I noticed that NEWS for changes between 633 and 644 includes "Mouse right-click jumps to position...", but it was added after 644 (at version.c it added as 645). I guess maybe "644" would change to the actual release version over time?

[gwsw]

Hm, yeah there seems to be some confusion of input and output modes. Right now win32_kbhit is setting ENABLE_MOUSE_INPUT on the output handle; I'm surprised this works as it seems like it should be interpreted as ENABLE_LVB_GRID_WORLDWIDE and leave the mouse disabled. I'll review this.

As far as the NEWS file, v645 is still "open" (the version number still has an "x" on the end), so things are expected to be in flux. Before an actual v645 release is built I will update the NEWS file (ready_to_release prevents the release build if that isn't done). Ideally I'd update it on the first 645x build; I'll try to do that in the future.

[avih]

Right, and at win32_kbhit, I think it would be better set the mode conditionally, like so:

            DWORD m;
            if (GetConsoleMode(tty, &m) && m != curr_console_mode)
                SetConsoleMode(tty, curr_console_mode);

I tested it, and it eliminates all the unneeded SetConsoleMode calls (which is basically 99.9999% of them).

[gwsw]

But why do you want to eliminate SetConsoleMode calls? Is SetConsoleMode slower than GetConsoleMode?

[avih]

It shouldn't affect a good implementation, but a bad one might reset things unconditionally when the mode is set, even if it's the same as the previous mode.

[gwsw]

But surely something in Windows wouldn't be implemented badly ... oh wait, never mind. :)

[avih]

Is SetConsoleMode slower than GetConsoleMode?

As an anecdote, I tested that on XP(VM)/7/10 and no, it's not slower :)

But personally I'd still feel better without setting the console mode 50 times per second during idle.

[gwsw]

Ok, take a look at 308cd52f57f66986283733104e5cff0899ff04ae. This has several fixes:

1. This keeps the input mode and output mode variables separate.
2. It fixes the fact that win32_init_vt_term was setting input bits on the output handle.
3. It fixes close_getchr so that it resets to the actual initial input mode; previously it was just setting it to (ENABLE_PROCESSED_INPUT | ENABLE_MOUSE_INPUT) regardless of the actual value at startup.
4. It fixes open_getchr so it doesn't set ENABLE_MOUSE_INPUT if we're not using --mouse.
5. It avoids calling SetConsoleMode unnecessarily in win32_kbhit as you suggested.

It does not yet set the output mode in win32_kbhit. As you say, I'm not sure that's necessary, although I suppose it's unlikely to cause problems if we did so.

[avih]

Looks much better. Thanks.

Two nits:

Now if the user doesn't enable quick edit at the console options, then less still enables it if the mouse is not enabled, which I think is wrong.

The Get/SetConsoleMode calls do have a return value which indicates success/failure - which is ignored except at the win32_kbhit.

I don't think it's likely to fail, but still.. but deciding what to do on failure can be a PITA. Not sure if even a warning is good, except if it's done at most once, and I don't know if it's worth the code.

But overall, much better.

[gwsw]

The quick edit issue is fixed in 3a4c4c9d884217cbdab0ccfbaf3f1c5a5d52bbe2.

I'm not sure about possible ConsoleMode failures. I can't really see anything very useful to do if they fail, except perhaps that if GetConsoleMode fails in get_term, we should set init_console_output_mode to some reasonable default.

[avih]

if the user doesn't enable quick edit at the console options, then less still enables it if the mouse is not enabled

The quick edit issue is fixed in 3a4c4c9

Is it fixed for you? It's not fixed for me as far as I can tell.

And looking at the code, while init/deinit mouse look OK, none of them is called when not using --mouse, which leaves us with the mode set at open_getchr, which was set one commit earlier, and is:

curr_console_input_mode = ENABLE_PROCESSED_INPUT | ENABLE_EXTENDED_FLAGS | ENABLE_QUICK_EDIT_MODE;

Which enables quick edit...

Am I missing something?

What say you about the following diff, on top of current master?

handle all input mode bits in one place

```diff diff --git a/screen.c b/screen.c index 767cee2..85166ef 100644 --- a/screen.c +++ b/screen.c @@ -180,6 +180,8 @@ public int sgr_mode; /* Honor ANSI sequences rather than using above #if MSDOS_COMPILER==WIN32C static DWORD init_console_output_mode; extern DWORD init_console_input_mode; +extern DWORD base_console_input_mode; +extern DWORD mouse_console_input_mode; extern DWORD curr_console_input_mode; public int vt_enabled = -1; /* Is virtual terminal processing available? */ #endif @@ -285,10 +287,6 @@ extern int hilite_search; #if MSDOS_COMPILER==WIN32C extern int wscroll; extern HANDLE tty; -#ifndef ENABLE_EXTENDED_FLAGS -#define ENABLE_EXTENDED_FLAGS 0x80 -#define ENABLE_QUICK_EDIT_MODE 0x40 -#endif #else extern int tty; #endif @@ -1699,7 +1697,7 @@ public void init_mouse(void) ltputs(sc_s_mousecap, sc_height, putchr); #else #if MSDOS_COMPILER==WIN32C - curr_console_input_mode = (curr_console_input_mode | ENABLE_MOUSE_INPUT) & ~(ENABLE_QUICK_EDIT_MODE & init_console_input_mode); + curr_console_input_mode = mouse_console_input_mode; SetConsoleMode(tty, curr_console_input_mode); #endif #endif @@ -1715,7 +1713,7 @@ public void deinit_mouse(void) ltputs(sc_e_mousecap, sc_height, putchr); #else #if MSDOS_COMPILER==WIN32C - curr_console_input_mode = (curr_console_input_mode & ~ENABLE_MOUSE_INPUT) | (ENABLE_QUICK_EDIT_MODE & init_console_input_mode); + curr_console_input_mode = base_console_input_mode; SetConsoleMode(tty, curr_console_input_mode); #endif #endif diff --git a/ttyin.c b/ttyin.c index e2e8456..bbd3e66 100644 --- a/ttyin.c +++ b/ttyin.c @@ -23,8 +23,20 @@ #define _WIN32_WINNT 0x400 #endif #include + +#ifndef ENABLE_EXTENDED_FLAGS +#define ENABLE_EXTENDED_FLAGS 0x80 +#define ENABLE_QUICK_EDIT_MODE 0x40 +#endif + +#ifndef ENABLE_VIRTUAL_TERMINAL_INPUT +#define ENABLE_VIRTUAL_TERMINAL_INPUT 0x0200 +#endif + public HANDLE tty; public DWORD init_console_input_mode; +public DWORD base_console_input_mode; +public DWORD mouse_console_input_mode; public DWORD curr_console_input_mode; #else public int tty; @@ -88,9 +100,24 @@ public void open_getchr(void) tty = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ, &sa, OPEN_EXISTING, 0L, NULL); - /* Make sure we get Ctrl+C events. */ + GetConsoleMode(tty, &init_console_input_mode); - curr_console_input_mode = ENABLE_PROCESSED_INPUT | ENABLE_EXTENDED_FLAGS | ENABLE_QUICK_EDIT_MODE; + + /* + * Make sure we get Ctrl+C events at our modes, without mouse by + * default, and without VT input. We don't care about other flags + */ + base_console_input_mode = (init_console_input_mode | ENABLE_PROCESSED_INPUT) + & ~(ENABLE_MOUSE_INPUT | ENABLE_VIRTUAL_TERMINAL_INPUT); + + /* + * enable mouse, and need to disable quick edit, or else it overrides + * the mouse (and extended flags is required with disabled quick edit) + */ + mouse_console_input_mode = (base_console_input_mode | ENABLE_MOUSE_INPUT | ENABLE_EXTENDED_FLAGS) + & ~ENABLE_QUICK_EDIT_MODE; + + curr_console_input_mode = base_console_input_mode; SetConsoleMode(tty, curr_console_input_mode); #else #if MSDOS_COMPILER ```

Removing ENABLE_EXTENDED_FLAGS (which is set only when the mouse is enabled) breaks the mouse. Not sure why, ENABLE_MOUSE_INPUT is not documented as requiring the extended flags...

But it is documented that to disable quick edit, one need to set the extended flags, and when the basic mode was without quick edit, adding extended flags indeed disabled quick edit.

The fact that quick edit overrides mouse input is not documented explicitly as far as I can tell, but it was mentioned here in 2007 on why it's not on by default:

At any rate, turning QuickEdit on by default means that the mouse stops working in all of these programs.

and also observed elsewhere (good historical overview, which mentions some unfixable gotchas):

Both tools can work with a mouse... . On Windows 7, that works fine. Not so much on Windows 10, because instead of sending mouse events to the application, Windows tries to select text in the console window.

Quick edit apparently existed since windows NT 3.1(!), but got enabled by default starting at win10, so existing apps which enable the mouse without disabling quick edit got hit by quick-edit enabled by default and breaking their mouse support...

[gwsw]

The QUICK_EDIT bit is not acting like I expect it to. If I uncheck quick edit in the Options tab of the Properties of my cmd window, I confirm that I can't select text with the mouse. Then if I run the program below that just opens a console and closes it, quick edit is now enabled on my console (I can select text and the checkbox is now checked in Properties). I guess this is part of what it means to have quick edit "enabled by default". But it seems really bad that just opening CON changes the mode on the real console. So how can less determine if quick edit is set on the console? If I call GetConsoleMode on this newly-opened handle, it says quick edit is enabled (mode is 0x1f7).

As far as I can tell, your proposed patch above has the same problem -- init_console_input_mode is set to 0x1f7 regardless of whether quick edit is enabled in the console.

#include <stdio.h>
#include <windows.h>

int main() {
    SECURITY_ATTRIBUTES sa;
    memset(&sa, 0, sizeof(SECURITY_ATTRIBUTES));
    sa.nLength = sizeof(SECURITY_ATTRIBUTES);
    sa.bInheritHandle = TRUE;
    HANDLE tty = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE,
            FILE_SHARE_READ, &sa,
            OPEN_EXISTING, 0L, NULL);
    CloseHandle(tty);
    return 0;
}

[avih]

Then if I run the program below that just opens a console and closes it, quick edit is now enabled on my console (I can select text and the checkbox is now checked in Properties). I guess this is part of what it means to have quick edit "enabled by default". But it seems really bad that just opening CON changes the mode on the real console

That does sound pretty bad.

So how can less determine if quick edit is set on the console? If I call GetConsoleMode on this newly-opened handle, it says quick edit is enabled (mode is 0x1f7).

As far as I can tell, your proposed patch above has the same problem -- init_console_input_mode is set to 0x1f7 regardless of whether quick edit is enabled in the console.

In my tests I also got once to a state where it changed the terminal checkbox, but then I closed that window and started from scratch in a new window and it was OK.

I think this might be part of the unfixable problems mentioned at the second link, because it's stateful, and you can't read the state accurately unless the extended flags bit is set, but if some app (like a previous instance of less) unset the extended bits flags, then it gets into bad territory until you close the window. Or at least that's roughly how I think of it.

In my tests with less itself with my last patch above with a new window, if quick edit was disabled, then it remained disabled both with and without mouse enabled in less, and if it was enabled, then it got corectly disabled when using the mouse, and then back to enabled when less exits, and if mouse was not used then it remained enabled the whole time.

I can test again with this patch all 4 combos (QE enabled/disabled at the terminal options, with/without --mouse). I'll post my results soon.

[avih]

with a new window, if quick edit was disabled, then it remained disabled both with and without mouse enabled in less, and if it was enabled, then it got corectly disabled when using the mouse, and then back to enabled when less exits, and if mouse was not used then it remained enabled the whole time.

Almost true, with the exception that I had to start a new window after changing the quick edit checkbox ("Defaults" - the default config, so that a new window will have the updated option).

The case where the window starts with quick edit enabled, and using --mouse, indeed disables QE - and it reflects at the checkbox of the current window - "Properries", and when less exits it gets enabled again, both while selecting and at the checkbox.

So it's very messy, but I don't think we can have control over it. The bottom line seems to be that to work correctly after changing the QE option manually, a new window is required which starts with the new option value, and then less works correctly with or without --mouse. I think I can live with that. Not that I have a choice...

Can you confirm this?

[gwsw]

I do get better behavior if I only change quick edit in Defaults and then open a new cmd window, rather than editing Properties. But the behavior is still not completely understandable to me; for example:

1. Start with quick edit disabled in Defaults. Open a new cmd window; quick edit is disabled as expected.
2. Edit Defaults. Enable quick edit. Close the defaults menu.
3. Close the cmd window.
4. Open a new cmd window.
5. Quick edit is NOT enabled. In Defaults it's still checked, but in Properties it is not checked, and text is not selectable. ???

I've made a correction in 1ce886241b80a89fa457c0367cf50c3bbbd9e7d4. I think this functions as well as can be done, given the weirdnesses of Windows around this feature.

[adoxa]

Your Command Prompt properties probably explicitly has QE off - check HKCU\Console\Command Prompt and/or %SystemRoot%_system32_cmd.exe.

[avih]

I've made a correction in 1ce8862. I think this functions as well as can be done

Well, this considers the QE bit while ignoring the extended flags bit, but when the EF is unset, the QE bit doesn't mean much, according to the MS docs and the 2nd link I posted earlier.

5. Properties it is not checked, and text is not selectable. ???

Yes, something fishy is going on, and Windows can also have a per-shortcut settings and who knows...

But anyway, we can only do what we can do, and that's to modify it as minimally as possible, and eventually restore it as best we can.

In our context that means to use the initial value with only the bits we need modified (don't set an absolute value) - like my patch does, and hope for the best...

[avih]

I don't think c53524ebf2693943f72095afae2fe6b32be92336 does anything:

- curr_console_input_mode = ENABLE_PROCESSED_INPUT | ENABLE_EXTENDED_FLAGS | (ENABLE_QUICK_EDIT_MODE & init_console_input_mode);
+ curr_console_input_mode |= ENABLE_PROCESSED_INPUT | ENABLE_EXTENDED_FLAGS | (ENABLE_QUICK_EDIT_MODE & init_console_input_mode);

The initial curr_console_input_mode is 0 anyway, so assigning an absolute value or adding these bits to 0 is identical. Did you mean = init_console_input_mode | ... instead of |= ... ?

And even if these bits were added to init_console_input_mode, why touching ENABLE_EXTENDED_FLAGS at all? You don't care about the quick edit value at this stage, so let these bits remain whatever the initial mode was.

Case in point, if the user doesn't use --mouse, then this might accidentally disable quick edit, because:

GetConsoleMode is even stranger. It may return ENABLE_EXTENDED_FLAGS when neither ENABLE_QUICK_EDIT_MODE nor ENABLE_INSERT_MODE is set, but it may also return none of the three flags even when ENABLE_QUICK_EDIT_MODE and/or ENABLE_INSERT_MODE is in fact set.

So if QE was enabled but the extended flags and QE bits are unset, then the line above would disable QE.

+ curr_console_input_mode &= ~(ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT);

Beyond the fact that this does nothing, because the previous line already ensured these bits are unset, why would you care about those bits anyway? They only affect ReadConsole and ReadFile - both of which are unused by less.

What you might have cared about in the basic mode, if the previous line was adding bits to init_console_input_mode, is to disable input VT, because I don't know if the less windows code knows how to deal with escape sequences for arrows keypress etc.

The VT input bit is off by default in a new console, but if less was launched by another application which enabled VT input, then less will initially have it enabled.

We also don't care about the initial mouse bit, because win32_mouse_event already ignores mouse events if --mouse was not used (and so my last patch didn't need to disable the mouse bit explicitly).

So I still think it's best that on startup just ensure that VT input if unset (again, assuming less doesn't handle it well), and that's it. All the other bits can remain identical to init_console_input_mode.

And when enabling the mouse, then the quick edit has to be disabled or else the mouse wont work, so here we'd need to unset quick edit, set extended flags, and set the mouse bit.

And if the mouse gets disabled after it was enabled, then get back to the basic mode by simply setting it. it's better than changing bits at curr_console_input_mode due to the extended flags shenanigans, and hope that extended flags was set, because otherwise all bets are off, and we're blind about the initial QE state.

Basically, my patch here except that we don't need to disable the mouse explicitly initially.

[gwsw]

You're right, I intended to set curr_console_input_mode = init_console_input_mode before doing the OR and AND.

As far as the ENABLE_ECHO_INPUT | ENABLE_LINE_INPUT bits, I misread the docs to say that they affect ReadConsoleInput rather than ReadConsole, so I think you're right that they don't need to be cleared.

The proper handling of the EXTENDED bit is unclear to me. Based solely on the Microsoft docs, it sounds like it would be safe to just leave EXTENDED and QE set to zero on startup, and they will remain unchanged. But it seems safer to explicitly set or clear QE, which according to the docs requires the EXTENDED flag in either case. I don't know how much trust to place in a random blog that contradicts the Microsoft docs. I suspect it's correct but obviously I have no way of testing ancient versions of Windows to confirm it.

[avih]

Based solely on the Microsoft docs, it sounds like it would be safe to just leave EXTENDED and QE set to zero on startup, and they will remain unchange

Not sure I get it. Do you mean to explicitly set these bits to zero, even if they had other initial values?

I also think this should keep the QE state, but do you think that setting the initial value would change them? I don't think there's evidence or info that setting the original QE and EF bits unmodified would change the QE state?

But also, based on that blog post, the docs don't tell the whole story.

Here's what it recommends:

... it should be apparent that using GetConsoleMode and SetConsoleMode 100% safely is not possible ...

... As long as the application never sets any of ENABLE_EXTENDED_FLAGS, ENABLE_QUICK_EDIT_MODE, and ENABLE_INSERT_MODE, all will be well ...

In general, applications should strive to preserve the ENABLE_EXTENDED_FLAGS bit whenever it’s set because that is the only way to make sure GetConsoleMode can fully read the console state.

So this is what my patch tried to do - touch the extended flags as little as possible, unless we have to (when the mouse is enabled), and then hope it was set initially (which does seems to be the case in my tests), because otherwise all bets are off - but still try to restore by setting the initial value we got, because maybe hopefully something is still able to realize that we want to restore the initial state (the underlaying implementation, based on heuristics or whatever).

I don't know how much trust to place in a random blog that contradicts the Microsoft docs

I think what it does is to tell a story which the docs omit, but I haven't noticed a contradiction with the docs. Did you? where?

For sure, that's only a third party info. But it does seem like some extensive digging was done, and that blog seems generally to the point (that's not the only blog post by the same author), it definitely left good impression on me.

I suspect it's correct but obviously I have no way of testing ancient versions of Windows to confirm it.

Same.

[gwsw]

Do you mean to explicitly set these bits to zero, even if they had other initial values?

Yes. According to the MS doc, you set EXT|QE to enable quick edit, and you set EXT to disable quick edit. That kind of implies that if you don't set either one, quick edit is unchanged, but I wouldn't want to rely on that without clearer documentation.

But yeah, I guess leaving both of them unchanged is probably the most conservative thing to do. I'll make another pass, probably tomorrow.

[gwsw]

Ok, 06b944e27b44ab14651ea6f92fe30b38b8e7e672 mostly implements your patch, without disabling mouse at startup.

[avih]

Thanks. Looks good as far as I can tell, but I did not yet put it into real world usage other than some anecdotal tests, as I intend to use it by default with pipe LESSOPEN. But that will happen soon, and I'll report if I notice any issues.

(I already know one - it's a bit annoying that there's always a warning to dismiss when pressing v to edit a file which was opened with LESSOPEN. Maybe an option to suppress the warning if the file exists?).

[avih]

I did not yet put it into real world usage other than some anecdotal tests, as I intend to use it by default with pipe LESSOPEN

Well, that didn't happen back then, but I did switch to LESSOPEN by default just now (previously I simulated LESSOPEN-like behavior using a script which piped the filter output into "less" manually).

So far I encountered one issue - fixed in #521.

I'll keep an eye open for more LESSOPEN related issues,.