Re: [hackers] [st][PATCH] Add support for DSR response "OK" escape sequence
On Tue, Feb 7, 2023 at 2:39 PM Hiltjo Posthuma wrote:
>
> On Tue, Feb 07, 2023 at 11:20:36AM -0500, Adam Price wrote:
> > On Tue, Feb 7, 2023 at 11:17 AM Roberto E. Vargas Caballero
> > wrote:
> > >
> > > On Tue, Feb 07, 2023 at 10:54:57AM -0500, Adam Price wrote:
> > > > On Mon, Feb 6, 2023 at 12:06 PM Roberto E. Vargas Caballero
> > > > wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Mon, Feb 06, 2023 at 08:45:27AM +0200, Santtu Lakkala wrote:
> > > > > > > tsetattr(csiescseq.arg, csiescseq.narg);
> > > > > > > break;
> > > > > > > - case 'n': /* DSR – Device Status Report (cursor position) */
> > > > > > > - if (csiescseq.arg[0] == 6) {
> > > > > > > + case 'n': /* DSR – Device Status Report */
> > > > > > > + switch (csiescseq.arg[0]) {
> > > > > > > + case 5: /* Status Report "OK" `0n` */
> > > > > > > + ttywrite("\033[0n", sizeof("\033[0n"), 0);
> > > > > >
> > > > > > This will write a NUL byte to the tty, which doesn't seem
> > > > > > intentional.
> > > > >
> > > > > Indeed, but it should not have any difference because '\0' is a
> > > > > control
> > > > > character that in this situation is ignored by the terminal. Anyway it
> > > > > should be avoided.
> > > >
> > > > Ah right, of course. Thank you to you two for pointing that out. I
> > > > should use
> > > > strlen() instead of sizeof().
> > > >
> > > > I will send an updated patch here shortly.
> > > >
> > >
> > > No, sizeof()-1. There is no reason to call strlen when you know the size.
> >
> > In this particular case, strlen() and sizeof() - 1 give the same result, no?
> >
>
> In a nutshell (discarding more pedantics :)):
>
> strlen() is done at run-time, it counts until a NUL byte.
> sizeof() is done at compile time and counts the size of the data structure.
>
> Otherwise the result is the same here.
> strlen() would most probably be optimized by the modern compiler too.
>
> I agree with Roberto that the best code pattern in this case is using
> sizeof()-1.
>
> I hope this helps,
>
> --
> Kind regards,
> Hiltjo
>
It does.
I appreciate the help and the explanation.
Re: [hackers] [st][PATCH] Add support for DSR response "OK" escape sequence
Thanks all! -- Adam
Re: [hackers] [st][PATCH] Add support for DSR response "OK" escape sequence
On Tue, Feb 7, 2023 at 11:17 AM Roberto E. Vargas Caballero
wrote:
>
> On Tue, Feb 07, 2023 at 10:54:57AM -0500, Adam Price wrote:
> > On Mon, Feb 6, 2023 at 12:06 PM Roberto E. Vargas Caballero
> > wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Feb 06, 2023 at 08:45:27AM +0200, Santtu Lakkala wrote:
> > > > > tsetattr(csiescseq.arg, csiescseq.narg);
> > > > > break;
> > > > > - case 'n': /* DSR – Device Status Report (cursor position) */
> > > > > - if (csiescseq.arg[0] == 6) {
> > > > > + case 'n': /* DSR – Device Status Report */
> > > > > + switch (csiescseq.arg[0]) {
> > > > > + case 5: /* Status Report "OK" `0n` */
> > > > > + ttywrite("\033[0n", sizeof("\033[0n"), 0);
> > > >
> > > > This will write a NUL byte to the tty, which doesn't seem intentional.
> > >
> > > Indeed, but it should not have any difference because '\0' is a control
> > > character that in this situation is ignored by the terminal. Anyway it
> > > should be avoided.
> >
> > Ah right, of course. Thank you to you two for pointing that out. I should
> > use
> > strlen() instead of sizeof().
> >
> > I will send an updated patch here shortly.
> >
>
> No, sizeof()-1. There is no reason to call strlen when you know the size.
In this particular case, strlen() and sizeof() - 1 give the same result, no?
[hackers] [st] ignore C1 control characters in UTF-8 mode || Hiltjo Posthuma
commit 211964d56ee00a7d46e251cbc150afb79138ae37
Author: Hiltjo Posthuma
AuthorDate: Tue Feb 7 20:00:59 2023 +0100
Commit: Hiltjo Posthuma
CommitDate: Tue Feb 7 20:00:59 2023 +0100
ignore C1 control characters in UTF-8 mode
Ignore processing and printing C1 control characters in UTF-8 mode.
These are in the range: 0x80 - 0x9f.
By default in st the mode is set to UTF-8.
This matches more the behaviour of xterm with the options -u8 or +u8 also.
Also see the xterm resource "allowC1Printable".
Let me know if this breaks something, in most cases I don't think so.
As usual a very good reference is:
https://invisible-island.net/xterm/ctlseqs/ctlseqs.html
diff --git a/st.c b/st.c
index 49357cc..134e724 100644
--- a/st.c
+++ b/st.c
@@ -2422,6 +2422,9 @@ check_control_code:
* they must not cause conflicts with sequences.
*/
if (control) {
+ /* in UTF-8 mode ignore handling C1 control characters */
+ if (IS_SET(MODE_UTF8) && ISCONTROLC1(u))
+ return;
tcontrolcode(u);
/*
* control codes are not shown ever
Re: [hackers] [st][PATCH] Add support for DSR response "OK" escape sequence
On Sun, Feb 05, 2023 at 06:39:24PM -0500, Adam Price wrote:
> ---
> VT100 defines an escape sequence [1] called Device Status Report (DSR). When
> the DSR sequence received is `csi 5n`, an "OK" response `csi 0n` is returned.
> This patch adds that "OK" response.
>
> I encountered this missing sequence when I noticed that fzf [2] would clobber
> my prompt whenever completing a find.
>
> To test that ST doesn't currently respond to `csi 5n`, use fzf's shell
> extension in ST's repo to complete the path for a file.
>
> my-fancy-prompt $ vim **
>
> st.c
>
> Select a file with , and notice that fzf clobbers some or all of your
> prompt.
>
> After applying this patch, do the same test as above and notice that fzf has
> no
> longer clobbered your prompt by placing the file name in the correct position
> in your command.
>
> my-fancy-prompt $ vim **
>
> my-fancy prompt $ vim st.c
>
> Thank you for considering my first patch submission.
>
> [1] https://www.xfree86.org/current/ctlseqs.html#VT100%20Mode
> [2] https://github.com/junegunn/fzf
>
>
> st.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/st.c b/st.c
> index 34c27ad..21a53cc 100644
> --- a/st.c
> +++ b/st.c
> @@ -1769,11 +1769,18 @@ csihandle(void)
> case 'm': /* SGR -- Terminal attribute (color) */
> tsetattr(csiescseq.arg, csiescseq.narg);
> break;
> - case 'n': /* DSR – Device Status Report (cursor position) */
> - if (csiescseq.arg[0] == 6) {
> + case 'n': /* DSR – Device Status Report */
> + switch (csiescseq.arg[0]) {
> + case 5: /* Status Report "OK" `0n` */
> + ttywrite("\033[0n", sizeof("\033[0n"), 0);
> + break;
> + case 6: /* Report Cursor Position (CPR) `;R` */
> len = snprintf(buf, sizeof(buf), "\033[%i;%iR",
> term.c.y+1, term.c.x+1);
> ttywrite(buf, len, 0);
> + break;
> + default:
> + goto unknown;
> }
> break;
> case 'r': /* DECSTBM -- Set Scrolling Region */
> --
> 2.39.1
>
>
Hi Adam,
Thanks for the patch,
I've slightly adapted the patch with input from the mailinglist and commited it.
Also thanks for the reviews people!
--
Kind regards,
Hiltjo
[hackers] [st] Add support for DSR response "OK" escape sequence || Adam Price
commit f17abd25b376c292f783062ecf821453eaa9cc4c
Author: Adam Price
AuthorDate: Tue Feb 7 19:54:29 2023 +0100
Commit: Hiltjo Posthuma
CommitDate: Tue Feb 7 19:57:34 2023 +0100
Add support for DSR response "OK" escape sequence
"VT100 defines an escape sequence [1] called Device Status Report (DSR).
When
the DSR sequence received is `csi 5n`, an "OK" response `csi 0n` is
returned.
This patch adds that "OK" response.
I encountered this missing sequence when I noticed that fzf [2] would
clobber
my prompt whenever completing a find.
To test that ST doesn't currently respond to `csi 5n`, use fzf's shell
extension in ST's repo to complete the path for a file.
my-fancy-prompt $ vim **
st.c
Select a file with , and notice that fzf clobbers some or all of your
prompt.
After applying this patch, do the same test as above and notice that fzf
has no
longer clobbered your prompt by placing the file name in the correct
position
in your command.
my-fancy-prompt $ vim **
my-fancy prompt $ vim st.c
Thank you for considering my first patch submission.
[1] https://www.xfree86.org/current/ctlseqs.html#VT100%20Mode
[2] https://github.com/junegunn/fzf
"
Patch slightly adapted with input from the mailinglist,
diff --git a/st.c b/st.c
index 34c27ad..49357cc 100644
--- a/st.c
+++ b/st.c
@@ -1769,11 +1769,18 @@ csihandle(void)
case 'm': /* SGR -- Terminal attribute (color) */
tsetattr(csiescseq.arg, csiescseq.narg);
break;
- case 'n': /* DSR â Device Status Report (cursor position) */
- if (csiescseq.arg[0] == 6) {
+ case 'n': /* DSR -- Device Status Report */
+ switch (csiescseq.arg[0]) {
+ case 5: /* Status Report "OK" `0n` */
+ ttywrite("\033[0n", sizeof("\033[0n") - 1, 0);
+ break;
+ case 6: /* Report Cursor Position (CPR) ";R" */
len = snprintf(buf, sizeof(buf), "\033[%i;%iR",
- term.c.y+1, term.c.x+1);
+ term.c.y+1, term.c.x+1);
ttywrite(buf, len, 0);
+ break;
+ default:
+ goto unknown;
}
break;
case 'r': /* DECSTBM -- Set Scrolling Region */
Re: [hackers] [st][PATCH] Add support for DSR response "OK" escape sequence
On Tue, Feb 07, 2023 at 10:54:57AM -0500, Adam Price wrote:
> On Mon, Feb 6, 2023 at 12:06 PM Roberto E. Vargas Caballero
> wrote:
> >
> > Hi,
> >
> > On Mon, Feb 06, 2023 at 08:45:27AM +0200, Santtu Lakkala wrote:
> > > > tsetattr(csiescseq.arg, csiescseq.narg);
> > > > break;
> > > > - case 'n': /* DSR – Device Status Report (cursor position) */
> > > > - if (csiescseq.arg[0] == 6) {
> > > > + case 'n': /* DSR – Device Status Report */
> > > > + switch (csiescseq.arg[0]) {
> > > > + case 5: /* Status Report "OK" `0n` */
> > > > + ttywrite("\033[0n", sizeof("\033[0n"), 0);
> > >
> > > This will write a NUL byte to the tty, which doesn't seem intentional.
> >
> > Indeed, but it should not have any difference because '\0' is a control
> > character that in this situation is ignored by the terminal. Anyway it
> > should be avoided.
>
> Ah right, of course. Thank you to you two for pointing that out. I should use
> strlen() instead of sizeof().
>
> I will send an updated patch here shortly.
>
No, sizeof()-1. There is no reason to call strlen when you know the size.
Regards.
Re: [hackers] [st][PATCH] Add support for DSR response "OK" escape sequence
On Mon, Feb 6, 2023 at 12:06 PM Roberto E. Vargas Caballero
wrote:
>
> Hi,
>
> On Mon, Feb 06, 2023 at 08:45:27AM +0200, Santtu Lakkala wrote:
> > > tsetattr(csiescseq.arg, csiescseq.narg);
> > > break;
> > > - case 'n': /* DSR – Device Status Report (cursor position) */
> > > - if (csiescseq.arg[0] == 6) {
> > > + case 'n': /* DSR – Device Status Report */
> > > + switch (csiescseq.arg[0]) {
> > > + case 5: /* Status Report "OK" `0n` */
> > > + ttywrite("\033[0n", sizeof("\033[0n"), 0);
> >
> > This will write a NUL byte to the tty, which doesn't seem intentional.
>
> Indeed, but it should not have any difference because '\0' is a control
> character that in this situation is ignored by the terminal. Anyway it
> should be avoided.
Ah right, of course. Thank you to you two for pointing that out. I should use
strlen() instead of sizeof().
I will send an updated patch here shortly.
