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
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
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?
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
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.
Re: [hackers] [st][PATCH] Add support for DSR response "OK" escape sequence
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.
Regards,
Re: [hackers] [st][PATCH] Add support for DSR response "OK" escape sequence
On 6.2.2023 1.39, Adam Price 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.
+ break;
+ case 6: /* Report Cursor Position (CPR) `;R` */
len = snprintf(buf, sizeof(buf), "\033[%i;%iR",
term.c.y+1, term.c.x+1);
--
Santtu
[hackers] [st][PATCH] 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
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
