iTerm2 ticket: https://gitlab.com/gnachman/iterm2/issues/3606

Instead of a special looking cursor, another approach could be to send a signal, and let g-t handle it the way it wants to (e.g. icon in tab bar).

Recognising no-echo mode is also needed for bug 726176.

I wonder if there are valid reasons for now having a signal (like SIGWINCH for size) for when the termios settings change... would be nice to have that, but I'm not diving into the kernel :-)

To detect no-echo it's probably good enough to check on every key press.

(In reply to Christian Persch from comment #3)

> but I'm not diving into the kernel :-)

me neither :)

> To detect no-echo it's probably good enough to check on every key press.

In that case you'd not immediately get this cursor after the password prompt, only after the first keypress. Checking at keypress is useful for IME. So both.

Created attachment 311915 [details] [review]
Demo for scroll lock

Here's a demo. I hope I managed to get right the logic that watches keyboard.

An app can release scroll lock by disabling IXON at any time; we don't recognize if it's re-enabled shortly afterwards, but checking for it at every keypress makes the logic a bit more robust.

tty_ioctl(4) -> TIOCPKT seems to be relevant, maybe it's the key to reliable get notified of scroll lock state changes, but at this point I don't understand a single word of its docs :)

Created attachment 311917 [details] [review]
Demo for scroll lock, v2

Created attachment 311937 [details] [review]
Demo for scroll lock, v3

Switching to the tty_ioctl(4) -> TIOCPKT method. This one seems to be way simpler and 100% robust.

The manpage is very poorly worded. Basically what happens is that for each read() call, we get an extra byte at the beginning of the buffer, notifying us about any ^S/^Q event (if any). Subsequent bytes (if any) are normal input bytes.

Review of attachment 311937 [details] [review]:

Great idea! :-)

Could you please separate this into 2 patches: one to introduce the TIOCPKT mode, and one that uses it for the cursor changes? And I think we should also add a signal (or property) to VteTerminal so that gnome-terminal could indicate this in the UI (need to think how exactly, maybe an overlay in a corner, or sth...).

Also, I think you need to add G_IO_PRI to the IO conditions in g_io_watch_add_full() in _vte_terminal_connect_pty_read() (which adds the FD to the set of FDs to watch for exceptions in select(2), as the man page for that ioctl says). And I'm not sure, but probably also handle G_IO_PRI without G_IO_IN in vte_terminal_io_read() ?

::: src/pty.cc
@@ +763,3 @@
+        /* tty_ioctl(4) -> every read() gives an extra byte at the beginning
+         * notifying us of stop/start (^S/^Q) events. */
+        ioctl(fd, TIOCPKT, &one);

Just to be sure, let's check the return value and set @error accordingly.

::: src/vte.cc
@@ +4259,3 @@
+                                                        _vte_debug_print(VTE_DEBUG_IO, "Output stopped (^S)\n");
+                                                        terminal->pvt->scroll_locked = TRUE;
+                                                        _vte_invalidate_cursor_once(terminal, FALSE);

Just call into a static vte_terminal_set_scroll_locked() function here, I'd say.

@@ +10021,3 @@
+                vte_terminal_fill_rectangle(terminal, &bg,
+                                            x + width - width / 3, y, width / 3, height);
+                return;

Rather unusual look... :-)  Add a comment describing what it means? Also this fits best with BLOCK cursor, what about the other modes? No need to block on this, we can improve the look later.

::: src/vteinternal.hh
@@ +100,3 @@
         guchar data[VTE_INPUT_CHUNK_SIZE - 2 * sizeof(void *)];
 };
 

Ingenious :-)

However, I'm not sure this is guaranteed to work (for all compilers)? Would be good to add a static assertion that offsetof(dataminusone) + 1 == offsetof(data) ?

Also you probably need to add "- 1" to the sizeof of data now, since I think the -2*sizeof(void*) is so that the whole vte_incoming_chunk has sizeof divisible by 2*sizeof(void*).

@@ +406,3 @@
         guint vscroll_policy : 1;
+
+        gboolean scroll_locked;

I'd rather put that up into the 	/* PTY handling data. */ section with all the other IO stuff, since it's not really related to the *widget* scrolling?

(In reply to Christian Persch from comment #8)

> Could you please separate this into 2 patches: one to introduce the TIOCPKT

Sure! Btw it's time to figure out: do we want to change the cursor at all, or just let g-t do something?

The tab bar is a great place to show a symbol or icon, with a hover tooltip or whatever; just what to do when there's no tab bar? Then we're restricted to plain text in the window title.

> Also, I think you need to add G_IO_PRI to the IO conditions in
> g_io_watch_add_full() in _vte_terminal_connect_pty_read() (which adds the FD
> to the set of FDs to watch for exceptions in select(2), as the man page for
> that ioctl says). And I'm not sure, but probably also handle G_IO_PRI
> without G_IO_IN in vte_terminal_io_read() ?

This one's black magic to me.

BTW we don't need to watch for exceptions: we receive the scroll lock change as an input byte.

> Just to be sure, let's check the return value and set @error accordingly.

Sure.

> Just call into a static vte_terminal_set_scroll_locked() function here, I'd
> say.

Yup, indeed. The patch is still in "proof of concept" state.

> Rather unusual look... :-)  Add a comment describing what it means?

I didn't mean it to be final. It was just something I could implement in a minute. BTW I like that it resembles the pause button. I'm open to any design, as well as leaving the cursor unchanged and doing something in the chrome.

> Also
> this fits best with BLOCK cursor, what about the other modes? No need to
> block on this, we can improve the look later.

Just for reference: on Mac Terminal.app the cursor on password prompt becomes that rectangle with the hole in the middle; no matter if the normal cursor look is rectangle, underscore or i-beam.

> ::: src/vteinternal.hh
> @@ +100,3 @@
>          guchar data[VTE_INPUT_CHUNK_SIZE - 2 * sizeof(void *)];
>  };
>  
> 
> Ingenious :-)

Or ugly, depends... I guess I saw this (or a similar) pattern in GArray or something similar.

I'm thinking that renaming data to data_internal with a one bigger size, and making data = &data_internal[1] is probably a cleaner approach.

Or... we could just forget about it totally. We're single-threaded, and we restore that previous byte anyway, so it's not a problem if we corrupt 'len' or anything else. The chances of leaving the process's addressable memory is zero. (Okay I don't mean to seriously do this, but I'm quite sure it'd work safely.)

> +        gboolean scroll_locked;
> 
> I'd rather put that up into the 	/* PTY handling data. */ section with all
> the other IO stuff, since it's not really related to the *widget* scrolling?

Again: sure right, I haven't cleaned this code yet, I just put it at the end of the struct. I'll do a proper cleanup - but first I'd like to prototype up the password prompt thing as well.

(In reply to Egmont Koblinger from comment #9)
> (In reply to Christian Persch from comment #8)
> 
> > Could you please separate this into 2 patches: one to introduce the TIOCPKT
> 
> Sure! Btw it's time to figure out: do we want to change the cursor at all,
> or just let g-t do something?

Both, I think.

> The tab bar is a great place to show a symbol or icon, with a hover tooltip
> or whatever; just what to do when there's no tab bar? Then we're restricted
> to plain text in the window title.

If we switch to using a headerbar in g-t, there would be space for some notification emblem for this. Let's discuss that in a different (g-t) bug, though :-)
 
> > Also, I think you need to add G_IO_PRI to the IO conditions in
> > g_io_watch_add_full() in _vte_terminal_connect_pty_read() (which adds the FD
> > to the set of FDs to watch for exceptions in select(2), as the man page for
> > that ioctl says). And I'm not sure, but probably also handle G_IO_PRI
> > without G_IO_IN in vte_terminal_io_read() ?
> 
> This one's black magic to me.
> 
> BTW we don't need to watch for exceptions: we receive the scroll lock change
> as an input byte.

Yes, but I *think* (possibly wrongly :-) ) that without G_IO_PRI we don't get woken up if that status byte is the *only* byte to read. So probably just add PRIO to the flags when adding the watch, and then change the condition in vte_terminal_io_read to if (condition & (G_IO_IN | G_IO_PRI)) ?

> > ::: src/vteinternal.hh
> > @@ +100,3 @@
> >          guchar data[VTE_INPUT_CHUNK_SIZE - 2 * sizeof(void *)];
> >  };
> >  
> > 
> > Ingenious :-)
> 
> Or ugly, depends... I guess I saw this (or a similar) pattern in GArray or
> something similar.
> 
> I'm thinking that renaming data to data_internal with a one bigger size, and
> making data = &data_internal[1] is probably a cleaner approach.
> 
> Or... we could just forget about it totally. We're single-threaded, and we
> restore that previous byte anyway, so it's not a problem if we corrupt 'len'
> or anything else. The chances of leaving the process's addressable memory is
> zero. (Okay I don't mean to seriously do this, but I'm quite sure it'd work
> safely.)

I think we can just use it like this, with the static assertion, and revisit this issue IF anyone complains about his compiler breaking the assertion.

Created attachment 311988 [details] [review]
Demo, v4

Add PoC support for password (icanon + no echo) detection.

(Code quality is still very poor.)

(In reply to Christian Persch from comment #3)
> but I'm not diving into the kernel :-)

Turns out there's (since linux 2.6.something) TIOCPKG_IOCTL flag that is set in the control byte whenever the termios have changed. However, this only happens if the (old or new) termios have the EXTPROC flag set (in c_lflag), which has other consequences in how the tty driver works. Need to check what exactly changes, and if this is compatible with all terminal apps…

(In reply to Christian Persch from comment #12)

> Turns out there's (since linux 2.6.something) TIOCPKG_IOCTL flag that is set

(typo: it's TIOCPKT_IOCTL)

> in the control byte whenever the termios have changed. However, this only
> happens if the (old or new) termios have the EXTPROC flag set (in c_lflag),

According to https://www.daemon-systems.org/man/pty.4.html you need to do an ioctl(TIOCEXT) on the master, which makes perfect sense – alas, this is *BSD.

For Linux, you're right, EXTPROC needs to be set in c_lflag; problem being that an app can switch it off. Of course we can catch it and force it back on and hope we can do it eventually without a race condition, and hope that the app can't watch on termios change and revert it, ending up in an endless loop... but I'm not happy with this design at all.

Another problem is that setting EXTPROC breaks ^C for me, it becomes a literal 0x03 byte (similarly to -isig, although cooked mode echoes as a 0x03 byte rather than the 2-byte "^C" literal).

So I'm afraid we can't use it :(

It would be nice if someone implemented the BSD approach on Linux, probably it's a piece of cake for someone a bit experienced with kernel programming.

I don't think TIOCEXT ioctl is right either, I think (reading BSD kernel source code) that it still enables this undesirable behaviour.

What we need is a way to just always get the TIOCPKT_IOCTL whether EXTPROC is in the flags or not. That could take different forms:

* Just always send it, disregarding EXTPROC. Don't see how that could break anything, but…

* Add a new ioctl to enable this specifically.

* Use the value passed to TIOCPKT ioctl. It seems *any* nonzero value turns it on, so could also pass a flag in there so that it turns this on when set.

In either case, it's a simple patch. If only we had someone to handle kernel matters for us :-)

Since it's unlikely to happen in the foreseeable future, I think I'll go ahead with the current approach. (We get explicit ^S/^Q notifications; for password mode we poll the termios every now and then.)

We should take a look at rlwrap's implementation. (How come I haven't heard about this tool up until now??) It's source contains I_PUSH which seems to be the Solaris equivalent of TIOCPKT -- no sign of TIOCPKT though. I guess it's also polling the state, but maybe it knows the magic we're looking for.

I don't think there's any magic, since I looked at the PTY kernel code [https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/pty.c] and the only way it ever emits TIOCPKT_IOCTL is when EXTPROC is turned on :-(

So until/unless we can get the kernel fixed, we need polling :-/

I also checked the kernel code -- what I meant with rlwrap is that maybe it knows about a totally different approach. I doubt it, but maybe.

I really love having a different cursor for scroll lock; it has already come handy a couple of times. Don't ask me why I'm pressing ^S, I don't know :)

Do you have any ideas on cursor shapes?

For password I guess we could go for Mac Terminal's choice. For scroll lock I like the idea of having something along the standard Stop or Pause symbols. And then there's read-only mode.

(In reply to Egmont Koblinger from comment #19)
> Do you have any ideas on cursor shapes?

Not really... maybe solid block cursor with 'pause' symbol (2 vert bars) inside? 

> For password I guess we could go for Mac Terminal's choice. For scroll lock
> I like the idea of having something along the standard Stop or Pause
> symbols. And then there's read-only mode.

Sounds good for bigger fonts; maybe not that much for my 6x13. Will give a try (or feel free to if you wish).

Created attachment 314685 [details] [review]
Demo, v5

Fix bug 757445.

Created attachment 316265 [details] [review]
remaining bits

I pushed the parts of this that enable packet mode. Attaching the remaining bits ported to master.

*** Bug 773791 has been marked as a duplicate of this bug. ***

Tilix feature request for ^S/^Q reporting via API: https://github.com/gnunn1/tilix/issues/1593.

I guess reporting alone won't be enought, you'll probably also want API to stop/start directly (e.g. restart from the "stopped" notification).

Can VTE do that technically?

I'm wondering if there's a direct ioctl for that, or if VTE would need to check the stty settings and synthesize the "start" character (which is subject to race condition in the extremely unlikely case that this character is getting modified in that tiny time frame – I'm fine with ignoring this).

What if there's no start character configured? We're screwed. Or we could set up one (taking care to avoid conflicts), emit that, and then revert the setting. Again somewhat raceable, and pretty cumbersome.

It's also a complicated story with all the other stty settings, e.g. when starting up Midnight Commander I still see stop=^S and start=^Q both in the outer and inner terminal, yet mc receives ^S and ^Q, so they must be passed through because of some other stty option, not sure which one.

I'm wondering if Tilix could also do whatever VTE would do...

Or, who knows, maybe Tilix doesn't want to display a "restart" button, but rather educate the user and tell them which key to press. Which could again be looked up from stty, but now here's a much bigger window for that information to no longer be up to date. But again I'm fine with ignoring it.

Shall we add a convenience API for something that Tilix can also figure out itself? If so, should it be for reporting the "start" character, or for restarting the stream, or both?

I guess we should ask gnunn1 how he'd imagine this feature, rather than us designing something that doesn't happen to meet his expectations :)

Maybe we can just report the current state, and Tilix would display "press the restart key (typically ^Q)..." and this would be Good Enough(TM).

> Can VTE do that technically?

I think so, yes. tcflow(3) / TCXONC ioctl.

BTW, we want this for g-t too, of course.

(In reply to Christian Persch from comment #28)

> I think so, yes. tcflow(3) / TCXONC ioctl.

Yay!

(Note: tcflow(3) says "transmits a START character", we should check if it still enables the flow if no start character is defined.)

(Nit: I'm wondering why the lower-level ioctl_tty(2) manpage defines the behavior as "equivalent to" the higher-level tcflow(3) library method, it's weird to me.)

Do you think it's beneficial to have a convenience wrapper in VTE, or should g-t/tilix just directly execute this call? I'm fine with the latter.

Shall we also go for a different cursor shape in the mean time, or let's forget that approach (preserve that for password input)? I'm fine abandoning that approach here due to our lack of graphical design skills :)

> BTW, we want this for g-t too, of course.

Of course! :)

> we should check if it still enables the flow if no start character...

Oh, I guess that's the difference between TCOON and TCION, the first one directly restarts the output, the second one restarts it indirectly via the start character. So we'd go with TCOON. Amazing!

> Do you think it's beneficial to have a convenience wrapper in VTE, 

Definitly yes. IMO the wrapper UI should never have to go behind vte's back and mess with the terminal PTY's FD directly.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vte/-/issues/2228.