Fix kitty keyboard protocol parsing of ctrl-o

[krobelus]

The third commit wants to add more logic and a unit test related to decoding
terminal escape sequences and the kitty keyboard protocol. It's probably
not right to keep all of this in lib/util.c, so extract a new module.

I called it "terminal" as first approximation - the name could be workshopped,
especially since we have a lot of terminal-specific code all around. We also
have lib/tty/tty.h though that's slightly different.

---

Commit 6e4510b (subshell: recognize CSI u encoding for ctrl-o, 2024-10-09)
made us parse sequences like \e[111;5u (meaning ctrl-o). As reported in
https://github.com/fish-shell/fish-shell/issues/10640#issuecomment-2698350151,
the kitty keyboard protocol specifies various other sequences that also
mean ctrl-o. Specifically, when numlock is active.

Instead of matching raw byte values, let's add teach our ECMA-48 CSI parser to
decode parameter bytes and interpret them according to the kitty keyboard protocol.

Tested with fish version 4:

1. Turn on numlock
2. SHELL=/bin/fish mc
3. ctrl-o
4. Enter
5. ctrl-o -- confirmed that this one is recognized now

Note that capslock still disables ctrl-o. We could change that easily
(and for consistency also allow ctrl-O).

While at it, add some unit tests (using a fish prompt as input data).

[zyv]

Hey, thanks for a quality contribution featuring... a test!!!11 I'm not sure if you're the first ever, but you get a gold star anyway ⭐ 🌟 ⭐

I'm really unfamiliar with the code and have little idea about the subject, hopefully @aborodin can say something. I only left formal comments that can be easily addressed. Also, please fix the build on Solaris.

  Making check in lib
  make  terminal
    CC       terminal.o
  terminal.c: In function ‘test_strip_ctrl_codes’:
  terminal.c:56:1: error: ‘main’ is normally a non-static function [-Werror=main]
     56 | main (void)
        | ^~~~
  terminal.c:70:1: error: expected declaration or statement at end of input
     70 | }
        | ^
  At top level:
  terminal.c:56:1: error: ‘main’ defined but not used [-Werror=unused-function]
     56 | main (void)
        | ^~~~

I guess you missed the END_TEST macro.

@asl could you please report whether this helps?

Note that capslock still disables ctrl-o. We could change that easily
(and for consistency also allow ctrl-O).

I'm not sure if we generally recognize shifted stuff, and if we do, how it works. I'm afraid to open this can of worms. There is some information in #2150 ... @ossilator and @egmontkob know more about this.

P.S. How amazing was it to enjoy the privilege of using GitHub for issues and a pull-request submission interface?

I know I probably can't impress people with this, but you're the one who experienced our venerable Trac... oh man, that was a hell of a lot of work. I need some encouragement 😅

[ossilator]

[#1 reason why GH reviews suck: one can't inline-comment on commit messages (which is such an obvious and easily fixable omission that one can't avoid thinking that this is intentional). did i already mention gerrithub ...? 😁 ]

the 3rd commit message (and the PR description) say "add teach".

from a cursory look, the kitty kbd proto implements everything i "asked for" in #2150, so: yay.

capslock/shiftlock should definitely not suppress control sequences.
as for shift ... i guess it's fine if this does, because:

• "nobody" should be doing that (intentionally), as it's pointless
• terminals may hijack these combos for their own purposes anyway (konsole does)

[krobelus]

Hey, thanks for a quality contribution featuring... a test!!!11 I'm
not sure if you're the first ever

there's a lot of boilerplate to adding tests I guess.
(I think the foot terminal defines a neat UNITTEST macro.)

I'm really unfamiliar with the code and have little idea about the
subject,

it's mainly about implementing section 5.4 (Control sequences) from
ECMA-48 and the kitty keyboard protocol, for the sole purpose of
letting us decode ctrl-o reliably.

hopefully @aborodin can say something. I only left formal
comments that can be easily addressed. Also, please fix the build
on Solaris.

I guess you missed the END_TEST macro.

@asl could you please report whether this helps?

I think this problem was only seen by @Crashdummyy.

Note I've added another commit that will cause fish to not enable kitty
protocol unless known safe. So you'd need to test without the last commit.

Note that capslock still disables ctrl-o. We could change that easily
(and for consistency also allow ctrl-O).

I'm not sure if we generally recognize shifted stuff, and if we
do, how it works. I'm afraid to open this can of worms. There is
some information in #2150 ... @ossilator and @egmontkob know more
about this.

This patch does add some parsing code that could be used in future to
fix key bindings when inside mc (which would need mc to explictly
request the kitty keyboard protocol).
But for now it's only fixing a preexisting issue.

P.S. How amazing was it to enjoy the privilege of using GitHub for
issues and a pull-request submission interface?

I know I probably can't impress people with this, but you're the
one who experienced our venerable Trac... oh man, that was a hell
of a lot of work. I need some encouragement 😅

I've been watching that from afar, seems pretty nice. GitHub API docs
are quite good IIRC.
Having to upload patches in the browser was definitely suboptimal.

[#1 reason why GH reviews suck: one can't inline-comment on commit
messages (which is such an obvious and easily fixable omission that
one can't avoid thinking that this is intentional).

Yeah, I think github wants to keep people on their website, so they
boycott commit messages (the github cofounder said "it's too difficult
to find commit messages" -- well, guess who's too blame). Github also
encourages people to create messy commit histories. I prefer the
mailing list style. But whatever gets people to contribute.

did i already mention gerrithub ...? 😁 ]

the 3rd commit message (and the PR description) say "add teach".

from a cursory look, the kitty kbd proto implements everything i "asked for" in #2150, so: yay.

capslock/shiftlock should definitely not suppress control sequences.

ok I made it so c-o still works when capslock is on.
c-s-o works too because from a user perspective that's exactly the same thing as c-o with capslock on.
Would need a good reason to treat them differently.

as for shift ... i guess it's fine if this does, because:

"nobody" should be doing that (intentionally), as it's pointless
terminals may hijack these combos for their own purposes anyway (konsole does)

Konsole also hijacks alt-f, that would be very annoying to use for me.

---

Extracted the test to separate commit.

range-diff

1:  182ba30ab ! 1:  20e6d84f9 Extract escape-sequence parsing logic into new module
    @@ Metadata
      ## Commit message ##
         Extract escape-sequence parsing logic into new module
     
    -    A following commit wants to add more logic and a unit test related to decoding
    -    terminal escape sequences and the kitty keyboard protocol.  It's probably
    -    not right to keep all of this in lib/util.c, so extract a new module.
    +    Following commits want to add more logic (and tests) for decoding terminal
    +    escape sequences, and implement a subset of the kitty keyboard protocol.
    +    It's probably not right to keep all of this in lib/util.c, so extract a
    +    new module.
     
    -    I called it "terminal" as first approximation - the name could be workshopped,
    -    especially since we have a lot of terminal-specific code all around. We also
    -    have lib/tty/tty.h though that's slightly different.
    +    I called it "terminal" because it helps mc act as terminal.
    +
    +    We also have lib/tty/tty.h though that's for talking to the actual terminal
    +    that mc is running inside.
     
         Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
     
      ## lib/Makefile.am ##
     @@ lib/Makefile.am: SUBLIB_includes = \
    + 	widget.h
    + 
      SRC_mc_utils = \
    ++	terminal.c terminal.h \
      	utilunix.c \
      	unixcompat.h \
    -+	terminal.c terminal.h \
      	util.c util.h
    - 
    - 
     
      ## lib/terminal.c (new) ##
     @@
    ++/*
    ++   Terminal emulation.
    ++
    ++   Copyright (C) 2025
    ++   Free Software Foundation, Inc.
    ++
    ++   This file is part of the Midnight Commander.
    ++   Authors:
    ++   Miguel de Icaza, 1994, 1995, 1996
    ++   Johannes Altmanninger, 2025
    ++
    ++   The Midnight Commander is free software: you can redistribute it
    ++   and/or modify it under the terms of the GNU General Public License as
    ++   published by the Free Software Foundation, either version 3 of the License,
    ++   or (at your option) any later version.
    ++
    ++   The Midnight Commander is distributed in the hope that it will be useful,
    ++   but WITHOUT ANY WARRANTY; without even the implied warranty of
    ++   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
    ++   GNU General Public License for more details.
    ++
    ++   You should have received a copy of the GNU General Public License
    ++   along with this program.  If not, see <https://www.gnu.org/licenses/>.
    ++ */
    ++
    ++/** \file terminal.c
    ++ *  \brief Source: terminal emulation.
    ++ *  \author Johannes Altmanninger
    ++ *  \date 2025
    ++ *
    ++ *  Subshells running inside Midnight Commander may assume they run inside
    ++ *  a terminal. This module helps us act like a real terminal in relevant
    ++ *  aspects.
    ++ */
    ++
     +#include <config.h>
     +
    -+#include "lib/terminal.h"
    ++#include <assert.h>
    ++
     +#include "lib/util.h"
     +#include "lib/strutil.h"
     +
    ++#include "lib/terminal.h"
    ++
     +/* --------------------------------------------------------------------------------------------- */
     +/**
     + * Remove all control sequences from the argument string.  We define
    @@ lib/terminal.c (new)
     +                while (*r != '\0' && (*r < 0x40 || *r > 0x7E))
     +                    ++r;
     +            }
    -+            else if (*r == ']')
    ++            if (*r == ']')
     +            {
     +                /*
     +                 * Skip xterm's OSC (Operating System Command)
    @@ lib/terminal.c (new)
     +        {
     +            char *n;
     +
    -+            n = str_get_next_char (r);
    ++            n = str_cget_next_char (r);
     +            if (str_isprint (r))
     +            {
     +                memmove (w, r, n - r);
    @@ lib/terminal.c (new)
     +
     +/* --------------------------------------------------------------------------------------------- */
     
    - ## lib/terminal.h (new) ##
    -@@
    -+/** \file terminal.h
    -+ *  \brief Header: terminal command decoding. See also tty.h
    -+ */
    -+
    -+#ifndef MC_TERMINAL_H
    -+#define MC_TERMINAL_H
    -+
    -+#include <sys/types.h>
    -+#include <inttypes.h>  // uintmax_t
    -+
    -+#include "lib/global.h"  // include <glib.h>
    -+
    -+char *strip_ctrl_codes (char *s);
    -+
    -+/* Replaces "\\E" and "\\e" with "\033". Replaces "^" + [a-z] with
    -+ * ((char) 1 + (c - 'a')). The same goes for "^" + [A-Z].
    -+ * Returns a newly allocated string. */
    -+char *convert_controls (const char *s);
    -+
    -+#endif
    -
      ## lib/util.c ##
     @@ lib/util.c: skip_numbers (const char *s)
          return su;
    @@ src/subshell/common.c
      #include "lib/unixcompat.h"
      #include "lib/tty/tty.h"  // LINES
      #include "lib/tty/key.h"  // XCTRL
    -
    - ## tests/src/Makefile.am ##
    -@@
    - PACKAGE_STRING = "/src"
    - 
    --SUBDIRS = . filemanager vfs
    -+SUBDIRS = . filemanager lib vfs
    - 
    - if USE_INTERNAL_EDIT
    - SUBDIRS += editor
-:  --------- > 2:  19b0cc97f Test for stripping control codes
2:  d1f2607f5 ! 3:  64140668c const-correct strip_ctrl_codes
    @@ lib/terminal.c: strip_ctrl_codes (char *s)
     -            char *n;
     +            const char *n;
      
    --            n = str_get_next_char (r);
    -+            n = str_cget_next_char (r);
    +             n = str_cget_next_char (r);
                  if (str_isprint (r))
    -             {
    -                 memmove (w, r, n - r);
3:  00eb67a8e ! 4:  cf671cfe2 Fix kitty keyboard protocol parsing of ctrl-o
    @@ Commit message
         the kitty keyboard protocol specifies various other sequences that also
         mean ctrl-o. Specifically, when numlock is active.
     
    -    Instead of matching raw byte values, let's add teach our ECMA-48 CSI parser to
    -    decode parameter bytes and interpret them according to the kitty keyboard protocol.
    +    Instead of matching raw byte values, let's teach our ECMA-48 CSI parser to
    +    decode parameter bytes and interpret them according to the kitty keyboard
    +    protocol.
     
    -    Tested with fish version 4:
    +    Tested with fish version 4 on kitty:
     
                 1. Turn on numlock
                 2. SHELL=/bin/fish src/mc
    @@ Commit message
                 4. Enter
                 5. ctrl-o -- confirmed that this one is recognized now
     
    -    Note that capslock still disables ctrl-o. We could change that easily
    -    (and for consistency also allow ctrl-O).
    -
    -    While at it, add some unit tests (using a fish prompt as input data).
    +    Make ctrl-O do the same thing I guess, to make switching easier if capslock
    +    is in effect.
     
         Signed-off-by: Johannes Altmanninger <aclopte@gmail.com>
     
    - ## configure.ac ##
    -@@ configure.ac: tests/lib/widget/Makefile
    - tests/src/Makefile
    - tests/src/filemanager/Makefile
    - tests/src/editor/Makefile
    -+tests/src/lib/Makefile
    - tests/src/editor/test-data.txt
    - tests/src/vfs/Makefile
    - tests/src/vfs/extfs/Makefile
    -
      ## lib/terminal.c ##
     @@
    - #include <config.h>
    - 
    -+#include <assert.h>
    -+
      #include "lib/terminal.h"
    - #include "lib/util.h"
    - #include "lib/strutil.h"
      
      /* --------------------------------------------------------------------------------------------- */
     +
    @@ lib/terminal.c
     + *
     + * On success, *sptr will point to one-past the end of the sequence.
     + * On failure, *sptr will point to the first invalid byte.
    -+ *
    -+ * Here's the format in a sort of pidgin BNF:
       *
     - * control-seq = Esc non-'['
     - *             | Esc '[' (parameter-byte)* (intermediate-byte)* final-byte
    ++ * Here's the format in a sort of pidgin BNF:
    ++ *
     + * CSI-command = Esc '[' parameters (intermediate-byte)* final-byte
     + * parameters = [0-9;:]+
     + *            | [<=>?] (parameter-byte)* # private mode
    @@ lib/terminal.c
     +    while (0)
     +
     +    char private_mode = '\0';
    -+    assert (c != '\0');
    ++    g_assert (c != '\0');
     +    if (strchr ("<=>?", c) != NULL)
     +    {
     +        private_mode = c;
    @@ lib/terminal.c
     +
     +    // parameter bytes
     +    size_t param_count = 0;
    -+    if (private_mode)
    ++    if (private_mode != '\0')
     +        while (c >= 0x30 && c <= 0x3F)
     +            NEXT_CHAR;
     +    else
     +    {
    -+        if (out)
    ++        if (out != NULL)
     +            // N.B. Empty parameter strings are allowed. For our current use,
     +            // defaulting to zero happens to work.
     +            memset (out->params, 0, sizeof (out->params));
    @@ lib/terminal.c
     +                if (param_count == 0)
     +                    param_count = 1;
     +                if (tmp * 10 < tmp)
    -+                    goto invalid_sequence;
    ++                    goto invalid_sequence;  // overflow
     +                tmp *= 10;
    ++                if (tmp + c - '0' < tmp)
    ++                    goto invalid_sequence;  // overflow
     +                tmp += c - '0';
    -+                if (out)
    ++                if (out != NULL)
     +                    out->params[param_count - 1][sub_index] = tmp;
     +            }
    -+            else if (c == ':' && ++sub_index < ARRAY_LEN (out->params[0]))
    ++            else if (c == ':' && ++sub_index < G_N_ELEMENTS (out->params[0]))
     +                tmp = 0;
    -+            else if (c == ';' && ++param_count <= ARRAY_LEN (out->params))
    ++            else if (c == ';' && ++param_count <= G_N_ELEMENTS (out->params))
     +                tmp = sub_index = 0;
     +            else
     +                goto invalid_sequence;
    @@ lib/terminal.c
     +        goto invalid_sequence;
     +    ++s;
     +    ok = TRUE;
    -+    if (out)
    ++    if (out != NULL)
     +    {
     +        out->private_mode = private_mode;
     +        out->param_count = param_count;
    @@ lib/terminal.c
     +    return ok;
     +}
     +
    -+/* ---------------------------------------------------------------------------------------------
    -+ */
    ++/* --------------------------------------------------------------------------------------------- */
     +/**
     + * Remove all control sequences (CSI, OSC) from the argument string.
       *
    @@ lib/terminal.c: strip_ctrl_codes (char *s)
     +                // We're already past the sequence, no need to increment.
     +                continue;
                  }
    -             else if (*r == ']')
    +             if (*r == ']')
                  {
     @@ lib/terminal.c: strip_ctrl_codes (char *s)
      }
    @@ lib/terminal.c: convert_controls (const char *p)
     -
     -/* --------------------------------------------------------------------------------------------- */
     
    - ## lib/terminal.h ##
    + ## lib/terminal.h (new) ##
     @@
    - 
    - #include "lib/global.h"  // include <glib.h>
    - 
    ++/** \file terminal.h
    ++ *  \brief Header: terminal emulation logic.
    ++ */
    ++
    ++#ifndef MC_TERMINAL_H
    ++#define MC_TERMINAL_H
    ++
    ++#include <sys/types.h>
    ++#include <inttypes.h>  // uint32_t
    ++
    ++#include "lib/global.h"  // include <glib.h>
    ++
     +struct csi_command_t
     +{
     +    char private_mode;
    @@ lib/terminal.h
     +};
     +gboolean parse_csi (struct csi_command_t *out, const char **sptr, const char *end);
     +
    - char *strip_ctrl_codes (char *s);
    - 
    - /* Replaces "\\E" and "\\e" with "\033". Replaces "^" + [a-z] with
    -
    - ## lib/util.h ##
    -@@
    - #define _GL_CMP(n1, n2) (((n1) > (n2)) - ((n1) < (n2)))
    - 
    - /* Difference or zero */
    --#define DOZ(a, b) ((a) > (b) ? (a) - (b) : 0)
    -+#define DOZ(a, b)        ((a) > (b) ? (a) - (b) : 0)
    ++char *strip_ctrl_codes (char *s);
     +
    -+#define ARRAY_LEN(array) (sizeof (array) / sizeof ((array)[0]))
    - 
    - /* flags for shell_execute */
    - #define EXECUTE_INTERNAL (1 << 0)
    ++/* Replaces "\\E" and "\\e" with "\033". Replaces "^" + [a-z] with
    ++ * ((char) 1 + (c - 'a')). The same goes for "^" + [A-Z].
    ++ * Returns a newly allocated string. */
    ++char *convert_controls (const char *s);
    ++
    ++#endif
     
      ## src/subshell/common.c ##
     @@
    @@ src/subshell/common.c: set_prompt_string (void)
          setup_cmdline ();
      }
      
    ++enum kitty_keyboard_protocol
    ++{
    ++    modifier_shift = 0x01,
    ++    modifier_ctrl = 0x04,
    ++    modifier_caps_lock = 0x40,
    ++    modifier_num_lock = 0x80,
    ++
    ++    modifier_shifted = modifier_shift | modifier_caps_lock,
    ++
    ++    event_type_press = 1,
    ++    event_type_repeat = 2
    ++};
    ++
     +static gboolean
     +peek_subshell_switch_key (const char *buffer, size_t len)
     +{
    @@ src/subshell/common.c: set_prompt_string (void)
     +    if (buffer[0] == (XCTRL ('o') & 255))
     +        return TRUE;
     +
    -+    // Also check if ctrl-o is encoded as specified by the kitty keyboard protocol.
    ++    // Also check if ctrl-o is encoded via the kitty keyboard protocol.
     +    if (len == 1)
     +        return FALSE;
     +    if (buffer[0] != ESC_CHAR || buffer[1] != '[')  // CSI
    @@ src/subshell/common.c: set_prompt_string (void)
     +    uint32_t modifiers = csi.params[1][0] - 1;
     +    const uint32_t event = csi.params[1][1];
     +
    -+    enum
    -+    {
    -+        shift = 0x01,
    -+        ctrl = 0x04,
    -+        caps_lock = 0x40,
    -+        num_lock = 0x80,
    -+
    -+        evt_press = 1,
    -+        evt_repeat = 2
    -+    };
    -+
     +    // Apply any shift modifier.
    -+    // Note that if both shift and caps set, there should be no shifted key.
    -+    if ((modifiers & (shift | caps_lock)) && shifted_key)
    -+    {
    ++    // Note that if both shift and caps lock are set, there should be no shifted key.
    ++    if ((modifiers & modifier_shifted) != 0 && shifted_key)
     +        codepoint = shifted_key;
    -+        modifiers &= ~shift;
    -+    }
    ++    modifiers &= ~modifier_shifted;
     +
    -+    if (codepoint != 'o' || (modifiers & ~(caps_lock | num_lock)) != ctrl)
    -+        return FALSE;
    -+    if (event && event != evt_press && event != evt_repeat)
    ++    if ((codepoint != 'o' && codepoint != 'O') || (modifiers & ~modifier_num_lock) != modifier_ctrl)
     +        return FALSE;
    -+    return TRUE;
    ++    return event == 0 || event == event_type_press || event == event_type_repeat;
     +}
     +
      /* --------------------------------------------------------------------------------------------- */
    @@ src/subshell/common.c: feed_subshell (int how, gboolean fail_on_error)
                          write_all (mc_global.tty.subshell_pty, pty_buffer, i);
      
     
    - ## tests/src/lib/.gitignore (new) ##
    -@@
    -+terminal
    -
    - ## tests/src/lib/Makefile.am (new) ##
    -@@
    -+PACKAGE_STRING = "/src/lib"
    -+
    -+AM_CPPFLAGS = \
    -+	-DTEST_SHARE_DIR=\"$(abs_srcdir)\" \
    -+	$(GLIB_CFLAGS) \
    -+	-I$(top_srcdir) \
    -+	@CHECK_CFLAGS@
    -+
    -+LIBS = @CHECK_LIBS@ \
    -+	$(top_builddir)/src/libinternal.la \
    -+	$(top_builddir)/lib/libmc.la
    -+
    -+if ENABLE_MCLIB
    -+LIBS += $(GLIB_LIBS)
    -+endif
    -+
    -+TESTS = \
    -+	terminal
    -+
    -+check_PROGRAMS = $(TESTS)
    -+
    -+edit_complete_word_cmd_SOURCES = \
    -+	terminal.c
    -
    - ## tests/src/lib/terminal.c (new) ##
    -@@
    -+#include <string.h>
    -+#include <assert.h>
    -+
    -+#include <config.h>
    -+#include "lib/global.h"  // include <glib.h>
    -+#include "lib/terminal.h"
    -+#include "lib/strutil.h"
    -+
    -+#define TEST_SUITE_NAME "/src/lib/terminal"
    -+
    -+#include "tests/mctest.h"
    -+
    -+/* --------------------------------------------------------------------------------------------- */
    -+
    -+static void
    -+setup (void)
    -+{
    -+    str_init_strings (NULL);
    -+}
    -+
    -+static void
    -+teardown (void)
    -+{
    -+    str_uninit_strings ();
    -+}
    -+
    -+/* --------------------------------------------------------------------------------------------- */
    -+
    + ## tests/lib/terminal.c ##
    +@@ tests/lib/terminal.c: teardown (void)
    + 
    + /* --------------------------------------------------------------------------------------------- */
    + 
     +START_TEST (test_parse_csi)
     +{
     +    const char *s = &"\x1b[=5uRest"[2];
    @@ tests/src/lib/terminal.c (new)
     +
     +/* --------------------------------------------------------------------------------------------- */
     +
    -+START_TEST (test_strip_ctrl_codes)
    -+{
    -+    char *s = strdup (
    -+        "\033]0;~\a\033[30m\033(B\033[m\033]133;A;special_key=1\a$ "
    -+        "\033[K\033[?2004h\033[>4;1m\033[=5u\033=\033[?2004l\033[>4;0m\033[=0u\033>\033[?2004h\033["
    -+        ">4;1m\033[=5u\033=\033[?2004l\033[>4;0m\033[=0u\033>\033[?2004h\033[>4;1m\033[=5u\033=");
    -+    char *actual = strip_ctrl_codes (s);
    -+    const char *expected = "$ ";
    -+    ck_assert_str_eq (actual, expected);
    -+    free (s);
    -+}
    -+
    -+/* --------------------------------------------------------------------------------------------- */
    -+
    -+int
    -+main (void)
    -+{
    -+    TCase *tc_core;
    -+
    -+    tc_core = tcase_create ("Core");
    -+
    -+    tcase_add_checked_fixture (tc_core, setup, teardown);
    -+
    -+    // Add new tests here: ***************
    + START_TEST (test_strip_ctrl_codes)
    + {
    +     char *s = strdup (
    +@@ tests/lib/terminal.c: main (void)
    +     tcase_add_checked_fixture (tc_core, setup, teardown);
    + 
    +     // Add new tests here: ***************
     +    tcase_add_test (tc_core, test_parse_csi);
    -+    tcase_add_test (tc_core, test_strip_ctrl_codes);
    -+    // ***********************************
    -+
    -+    return mctest_run_all (tc_core);
    -+}
    -+
    -+/* --------------------------------------------------------------------------------------------- */
    +     tcase_add_test (tc_core, test_strip_ctrl_codes);
    +     // ***********************************
    + 
-:  --------- > 5:  79c6a1968 subshell: tell fish that we actually support the kitty keyboard protocol now

[egmontkob]

egmontkob know more about this

No, unfortunately I don't. I'm not familiar with Kitty's keyboard protocol at all. So I'll sit out this discussion.

[zyv]

there's a lot of boilerplate to adding tests I guess.
(I think the foot terminal defines a neat UNITTEST macro.)

Well, you didn't just add a test, you added a whole new test suite. I don't see much difference between our current START_TEST / END_TEST and Foot's UNITTEST macro.

Foot seems to put their tests directly into the source files instead of having a separate tree of suites and tests. I can see how this makes it easier to add tests. It's not that it's an uncontroversial decision, though.

I think the biggest problem is that we have so few tests that it's basically not about finding a suite and adding your little new test to it, it's about writing a whole new suite from scratch. But that is a chicken and egg problem. If we don't add more suites and tests, there will never be enough of them to make it easy to add a new test to an existing suite.

However, my impression so far has been that most contributors either object on principle, calling it useless bureaucracy, or expect maintainers to write tests for them. That's why I'm so glad you don't seem to exhibit either of those behaviors.

I've been watching that from afar, seems pretty nice. GitHub API docs
are quite good IIRC.

Living in Germany, I've seen a lot worse. They do have their own quirks that you have to learn by doing though...

It seems that the REST API has taken a back seat and all new stuff goes into GraphQL, but this is somehow not really made explicit anywhere, so you basically start doing something with REST and bump into missing things, which eventually leads to throwing the whole thing out and starting from scratch with GraphQL.

On top of that, their import API has never left "unofficial" status and remains REST. While it's (thankfully) not completely broken yet, it means you don't get a decent client for it (had to code my own) and it also doesn't support new features like resolutions, issue types, etc.

But yeah, I am very happy with the results so far :)

Yeah, I think github wants to keep people on their website, so they
boycott commit messages (the github cofounder said "it's too difficult
to find commit messages" -- well, guess who's too blame).

I think it's more of a (self-reinforcing) cultural lack of demand these days than a conspiracy against commit messages... They are doing things that enterprise customers are screaming about. Most of the recent improvements to Issues and Projects are well done, in my opinion.

Github also encourages people to create messy commit histories.

But (unlike for commit messages) they have the tools for those who care to make it work. As you can see (well, probably not, since you don't have the commit bit), I have made it so that the PRs cannot be merged unless they are up-to-date on master and properly rebased (for which I have implemented a /rebase command).

So I hope we can keep our working style of committing fixups (instead of mostly force-pushing series of patches) and then rebasing and merging with one command. One big advantage I see with fixup is that it's naturally easy to see which comments have been fixed and which have not. With a force-push (yes, sometimes it's unavoidable) I would have to go through the whole patch again.

[zyv]

I added a few fixes. Otherwise I'm happy with the code as it is (but see below).

ok I made it so c-o still works when capslock is on.

I think I can agree with that...

c-s-o works too because from a user perspective that's
exactly the same thing as c-o with capslock on.
Would need a good reason to treat them differently.

... but this definitely sounds wrong to me. In the GUI programs I use, the shortcuts with Shift definitely have a different meaning. That is, Ctrl-O and Ctrl-Shift-O are definitely different, and both work regardless of whether CapsLock is on or not.

Could you please change this back? I don't want to break anything, so I won't do it myself.

range-diff

How did you do that? This could be useful to learn.

[zyv]

Since you care about commit messages, in 5dc7aef:

I guess we could avoid that but I don't think this
is not a big loss, especially because since fish 4.0.1 will enable it again.

The double negation is wrong.

[ossilator]

c-s-o works too
because from a user perspective that's exactly the same thing as c-o with capslock on.

absolutely not.
as a (not really) proof, consider that capslock doesn't lock the digit row, so it's not at all like actually pressing shift (unlike shiftlock on for example the german layout).
but more to the point, the keyboard being in a particular logical state is quite different from physically holding down a modifier, and specifically for the purpose of emitting control sequences, it would be highly unexpected for capslock to have an effect.

One big advantage I see with fixup is that it's naturally easy to see which comments have been fixed and which have not. With a force-push (yes, sometimes it's unavoidable) I would have to go through the whole patch again.

well, to me this sounds just backwards, like typical workaround thinking for bad tooling. squashing the fixups is a potentially error-prone process (the necessary reordering may cause conflicts), so it shouldn't be delayed. and of course gerrit has an answer to that: diffs between patchsets (versions of a change, or in practice, actual commits), also called inter-diffs, are a rather central element of the workflow.

seriously, just import mc to gerrithub (i would do that myself, but you'd need to give me full workflow permissions for the GH project) and import the pending PRs (after squashing the fixups), and see how it looks in practice. i've yet to see a developer with high standards who has looked back.
(it occurs to me that this would have been a good argument for converting the tickets with associated branches to PRs.)

[krobelus]

That is, Ctrl-O and Ctrl-Shift-O are definitely different, and both
work regardless of whether CapsLock is on or not.

Ok that makes sense, done.

range-diff
How did you do that? This could be useful to learn.

It's a "diff of patches" (or "diff of diffs"), useful after force
push. For example

git range-diff bc213f6ff...1cf6b5814
git range-diff origin/some-branch@{1}...origin/some-branch # show the diff since the last fetch

I think it takes some time to get accustomed to it,
but it's often the easiest way to find what changed.

[krobelus]

I have made it so that the PRs cannot be merged unless they are
up-to-date on master and properly rebased (for which I have implemented
a /rebase command).

Note that CI tests should typically run on a merge commit implicitly created
by the CI (not sure what's the default in github), meaning that an up-to-date
version will be tested, even if the PR branch is old.

After that suceeds, a gh pr merge --rebase <PR-URL> should result in the
same Git tree unless something else merged in the meantime.

So I hope we can keep our working style of committing fixups (instead
of mostly force-pushing series of patches) and then rebasing and
merging with one command.

I have no horse in this race but I'm strongly in favor of cleaning
up history early and often.

I don't think it's right to optimize for the hypothetical reviewer
that has good enough memory of a previous version of a patch series to
be able to benefit from multiple out-of-context commits added on top.

One big advantage I see with fixup is
that it's naturally easy to see which comments have been fixed and
which have not. With a force-push (yes, sometimes it's unavoidable)
I would have to go through the whole patch again.

I think that can be fine momentarily / depending on people but I don't think
it's a good idea in general. Usually a PR only has one person contributing
substantial changes but multiple reviewers who don't care about historical
artifacts.

We should optimize for the reader -- which includes the author; when I send
a new version of nontrivial patch series, I usually try to read through the
entire series again. This doesn't really work if commits are not ordered
properly. If I check every line in-order, I'll confused by errors that are fixed in
a later commit.

Solving the tooling problem with git range-diff (or jj interdiff) should
cover many use cases.

I like how the Git project does it (see https://public-inbox.org/git/)
but obviously they have a different setup (only one maintainer is
allowed to merge, so things must be optimized for the reviewer).
One thing to note is that they include in each resubmission of the same patch series
the range-diff and prose descriptions of what changed.

So it's definitely possible to have both a simple diff to the previous
version but still keep the history atomic and monotonic.

well, to me this sounds just backwards, like typical workaround
thinking for bad tooling. squashing the fixups is a potentially
error-prone process (the necessary reordering may cause conflicts),
so it shouldn't be delayed.

In parts it's also Git itself; I think Jujutsu does a great job at
nudging beginners in a good direction.

and of course gerrit has an answer to
that: diffs between patchsets (versions of a change, or in practice,
actual commits), also called inter-diffs, are a rather central
element of the workflow.

i've yet to see a developer with
high standards who has looked back.

I'm not really involved in mc but I'd love to try such a system that
encourages this.

I think it's absolutely the right choice to offer github (for 95%
of the population) but it can be nice to allow alternatives.

[zyv]

One big advantage I see with fixup is that it's naturally easy to see which comments have been fixed and which have not. With a force-push (yes, sometimes it's unavoidable) I would have to go through the whole patch again.

well, to me this sounds just backwards, like typical workaround thinking for bad tooling. squashing the fixups is a potentially error-prone process (the necessary reordering may cause conflicts), so it shouldn't be delayed. [...]

I think it depends on how much development you do, how big the reviews are, how many comments of what kind you get, etc.

In my previous endaviours, it was either a few "big" issues with the PR, or a reasonable number of small things to fix before the merge.

For big issues, force-pushing was fine, because I could take a fresh look at the whole new patch, and minor comments could be left until the end.

For small issues, the fixup was very convenient. You leave 5 comments, get 5 fixups of a few lines each, tick them off, start an automatic rebase process that is almost guaranteed to end without conflict (unless a wrong commit was fixed or something), and that's it.

Polishing series of patches for large projects like the kernel or Qt is a completely different story. Of course, fixups are an inadequate tool for this. But these projects have more than 1-2 people reviewing them, the reviewers have time, the contributors have time and motivation to do anything at all, learning non-trivial tools is definitely worth the benefits, etc.

I can't see how that applies to mc in its current form, with its current team and contributors. Maybe if I were working on the kernel, git or Qt I'd feel differently, because that's what I do anyway, but I'm not, not even close.

So PRs with fixups seem to me to be the sweet spot right now in terms of being better than nothing, but very light. And of course even that seems to be too much for all the previous submitters I know of.

seriously, just import mc to gerrithub (i would do that myself, but you'd need to give me full workflow permissions for the GH project) and import the pending PRs (after squashing the fixups), and see how it looks in practice. i've yet to see a developer with high standards who has looked back. (it occurs to me that this would have been a good argument for converting the tickets with associated branches to PRs.)

Can't you just fork mc to your account, set it up and make a demo if you're willing to put the work in? Why do you have to do it with the main organisation?

[zyv]

I have made it so that the PRs cannot be merged unless they are
up-to-date on master and properly rebased (for which I have implemented
a /rebase command).

Note that CI tests should typically run on a merge commit implicitly created by the CI (not sure what's the default in github), meaning that an up-to-date version will be tested, even if the PR branch is old.

After that suceeds, a gh pr merge --rebase <PR-URL> should result in the same Git tree unless something else merged in the meantime.

That's exactly how I set it up. Moreover, if something else has been merged in the meantime, the merge will be aborted.

So I hope we can keep our working style of committing fixups (instead
of mostly force-pushing series of patches) and then rebasing and
merging with one command.

I have no horse in this race but I'm strongly in favor of cleaning up history early and often.

I don't think it's right to optimize for the hypothetical reviewer that has good enough memory of a previous version of a patch series to be able to benefit from multiple out-of-context commits added on top.

Well, I think if you have a 1.5 reviewer and close to zero contributors, it's absolutely right to optimize for what the reviewers find convenient.

I tried to explain this above, but in the end I don't think it really matters. I'm not against other workflows, and I'm open to learning and changing mine, but otherwise I prefer what I think is optimal for me under the current circumstances.

One big advantage I see with fixup is
that it's naturally easy to see which comments have been fixed and
which have not. With a force-push (yes, sometimes it's unavoidable)
I would have to go through the whole patch again.

I think that can be fine momentarily / depending on people but I don't think it's a good idea in general. Usually a PR only has one person contributing substantial changes but multiple reviewers who don't care about historical artifacts.

Yeah, that's exactly the opposite of what we have here :)

Solving the tooling problem with git range-diff (or jj interdiff) should cover many use cases.

I will take a look at range-diff, thanks! Also, jj has been on my list for a long time, but I could never find the time to check it out... even more shameful now that I got to know the authors by chance, and I actually think very highly of the project.

I like how the Git project does it (see https://public-inbox.org/git/) but obviously they have a different setup (only one maintainer is allowed to merge, so things must be optimized for the reviewer). One thing to note is that they include in each resubmission of the same patch series the range-diff and prose descriptions of what changed.

So it's definitely possible to have both a simple diff to the previous version but still keep the history atomic and monotonic.

Looks a lot like the kernel workflow... somewhat unsurprising ;)

[zyv]

Thank you, I'm happy when CI and Andrew are happy!

common.c: In function 'peek_subshell_switch_key':
common.c:814:20: error: unused variable 'shifted_key' [-Werror=unused-variable]
  814 |     const uint32_t shifted_key = csi.params[0][1];
      |                    ^~~~~~~~~~~

[krobelus]

For small issues, the fixup was very convenient. You leave 5 comments, get 5 fixups of a few lines each, tick them off

I think range-diff allows the same workflow, and it also 1. guarantees that fixups are ordered 2. shows the fixups in their original context (so can tell if the surrounding code is new or not) 3. shows changes to commit messages

But these projects have more than 1-2 people reviewing them, the reviewers have time, the contributors have time and motivation to do anything at all, learning non-trivial tools is definitely worth the benefits, etc.

I'm happy to follow any workflow that helps. But to give a concrete example for why I prefer squashing early even if I don't expect anyone else to read my code. For my most recent push, I wanted to do another pass over my changes. Specifically I wanted to read all changes except for the boring/noisy code-movement commit, and also excluding anything that was merged in from master on this branch. I think that's a reasonable want (also post-merge, in case we found a problem with these changes) -- so I'd strongly prefer to not squash the code-movement commit with logic changes. But the Git command to do so gets a bit complicated, mainly because it needs to get rid of the interleaved merge commit by creating a a fake merge: code_movement_commit=20e6d84f9dde2949d4840dfcf4e180f433c9a465 git diff $(git merge-tree $(git log -1 --merges --format=%H)^2 $code_movement_commit) and of course a single "diff" invocation doesn't work well if there are multiple nontrivial patches

[ossilator]

I think it depends on how much development you do, how big the reviews are, how many comments of what kind you get, etc. [...]

honestly, you're overthinking it. project and contribution size don't matter at all. once you get into the gerrit workflow, you just want to use it for everything, because it's so natural and efficient.

for contributors it's a bit of a hurdle, because they need to get into history polishing in the first place. but my perspective is that this is actually a useful people filter.

Can't you just fork mc to your account, set it up and make a demo if you're willing to put the work in? Why do you have to do it with the main organisation?

i can, but it seems counter-productive to me. if you actually take a look at my "clone" and then want to use it, then you'd have to replicate my steps and your experiments (which should be an actual live review to be realistic), which is a total waste. conversely, if you do the experiment yourself and find it to be garbage, then nothing is lost by doing it in the main org, as it can be just deleted afterwards.

[zyv]

I think it depends on how much development you do, how big the reviews are, how many comments of what kind you get, etc. [...]

honestly, you're overthinking it. project and contribution size don't matter at all. once you get into the gerrit workflow, you just want to use it for everything, because it's so natural and efficient.

for contributors it's a bit of a hurdle, because they need to get into history polishing in the first place. but my perspective is that this is actually a useful people filter.

Can't you just fork mc to your account, set it up and make a demo if you're willing to put the work in? Why do you have to do it with the main organisation?

i can, but it seems counter-productive to me. if you actually take a look at my "clone" and then want to use it, then you'd have to replicate my steps and your experiments (which should be an actual live review to be realistic), which is a total waste. conversely, if you do the experiment yourself and find it to be garbage, then nothing is lost by doing it in the main org, as it can be just deleted afterwards.

I found a project that seems to use Gerrithub and read up on their process:

https://github.com/cue-lang/cue/blob/master/CONTRIBUTING.md

Now I understand better what you are saying. I'm afraid the people filter aspect has filtered me out at the moment. I kind of see the appeal, but the amount of overhead seems too crazy at the moment.

Maybe I'll come back to it one day when I've solved more of what I think are pressing problems...

[zyv]

@krobelus Thanks for your contribution, it was a wild ride!