Ticket #5047, #5048: Separate mc.keydef file, updated syntax

[egmontkob]

Proposed changes

Move the escape sequences from the global "mc.lib" and user "ini" file to new global and user "mc.keydef".

Saner escaping, nicer look.

Removed "terminal:" prefix.

• Resolves: Move the input escape sequences to a separate file #5047
• Resolves: Simplify escaping of input escape sequences #5048

Checklist

👉 Our coding style can be found here: https://midnight-commander.org/coding-style/ 👈

• I have referenced the issue(s) resolved by this PR (if any)
• I have signed-off my contribution with git commit --amend -s
• Lint and unit tests pass locally with my changes (make indent && make check)
• I have added tests that prove my fix is effective or that my feature works
• I have added the necessary documentation (if appropriate)

[egmontkob]

This is a cleanup before finalizing the real work in #4632 & friends.

Documentation (manpage etc.) is yet to be updated, but I'd prefer to hear a round of feedback first.

[mc-worker]

A quick view.

[mc-worker]

Let's return GString itself.

[egmontkob]

Let's return GString itself.

Is there a generic guideline here? Should we always prefer GString to char* as retval? How about input parameters?

Or should the API depend on how the internals were the most convenient to implement: if we already have a GString then return that?

I was tempted to change the input of this function, as well as the retval of the decode_controls() function to GString, for the reason that theoretically a terminal's escape sequence may contain a NUL byte (although in practice I really don't think it does, except for Ctrl+@ a.k.a. Ctrl+space which is the single NUL byte, handled elsewhere in the code).

For the retval of this function, as well as the input parameter of the decoding counterpart, the semantics is C-style string: cannot contain embedded NUL byte. What's the point of changing the return type to GString? Also, for consistency, shouldn't we then change the input parameter of the decoder too, making it possibly more cumbersome for the caller? Or have 3 GStrings and 1 char*?

Would love to learn more about the preference in general, and its reasons; thanks! :)

[egmontkob]

Added a new first commit, moving from keymap to mcconfig a method that I previously copy-pasted into keydef.

I haven't yet addressed the char * / GString issue for decode/encode, I'd like to see a consistent full picture which of the 2+2 input parameters and retvals should be changed, please help me here :)

[mc-worker]

I haven't yet addressed the char * / GString issue for decode/encode,

You do use a GString object within a function. Then you destroy it in parts in two different places: the container and some its members in the function, the char array in the caller function. It would be logical to destroy the entire object at once. And if you need the length of the string in the caller function, you have to call strlen() instead of use a lenmember of GString.

[egmontkob]

You do use a GString object within a function.

I happen to use it, in both the decode and encode function, for no particular reason. Each of them could have very easily been written without GString. Maybe if I had written the code a day earlier or a day later, or in a slightly different mood, I would have gone with plain char *.

I do not write the implementation first, and then tailor the interface to that, this wouldn't make sense to me. I first create the interface, with whatever types that I believe make the most sense, which in this case was char *, then I come up with an implementation, which in this case happened to use GString helpers.

Then you destroy it in parts in two different places: the container and some its members in the function, the char array in the caller function. It would be logical to destroy the entire object at once.

Destroying at two parts is a standard practice, this is explicitly what g_string_free (..., FALSE) was designed for. It's used in mc's codebase around 40 times. There's nothing wrong with it.

And if you need the length of the string in the caller function, you have to call strlen() instead of use a lenmember of GString.

I don't think I use the length in the caller. But even if I did...

I know, for some reason, mc has an extremely strong aversion against running strlen() twice on a string. We're talking about teeny-weeny strings here, very few of them, accessed very rarely (typically only at mc's startup). Maybe we can shave off a few nanoseconds. But that's not necessarily true either.

Accessing data via a GString * requires one more indirection than via a char *, possibly accessing memory at two very different places, which might be worse cache-wise than accessing one neighborhood twice. So it's not clear whether the performance would increase or decrease, having access to len and not having to measure it again. (But, again, we actually don't need the length.) But it's absolutely certainly super negligible, it's the worst use of our time trying to optimize it.

What we need to optimize for is code cleanness, consistency, and quick development flow.

For the decode function, a return value of GString * might convey the incorrect message of supporting the NUL byte. It does not support it. I upgraded all the char * to GString * that are related to learning keys, reading keys from the config file, and it still didn't work for me. (The byte 0x01 Ctrl-A, or ESC followed by this byte also didn't work for me as an "escape sequence" assigned to a key.) There's more work to be done, and presumably exactly zero need for it, so I'd rather not do it now.

And how far would you go with your logic? E.g. mc_config_get_escape_sequence_list() returns a list of strings as char **, now one question for sure is whether to rather return GString **, another question is that if I happen to internally go with GArray then should I return that, just so that the caller has immediate access to the number of strings returned?

I honestly really don't like this philosophy of adjusting the interface: the return value (and you didn't respond: what about the input parameters?) to the implementation, rather than designing the interface first. I really don't like trying to micro-optimize the most insignificant bit, let alone with no information whether it would actually speed up things or not if the caller cared about the string length, let alone the caller now doesn't even care. I really don't like being so afraid of maybe (but in fact: not) calling strlen() an additional few dozens of times per mc startup.

I'm tempted to say that if you firmly insist on returning GString * if it's created in the function body anyway, my proposed solution would be to rewrite the body of those functions not to use GString :)

It might look like I'm just spending 15 minutes to type this comment instead of changing what you asked for. Behind it is much more unpublished work actually trying to adjust the code, and if we're going with GString anyway then trying to add support for embedded NUL bytes, which would also require to change the interface of mc_config_get/set_escape_sequence_list() from char ** to GString **, for which I used GArray as an internal helper, but then again back to the question: shouldn't I return that GArray just in case the caller finds it useful...? and do it in reasonable, well-polished commits in some meaningful order... just to realize during the process that e.g. unittests became uglier, and somehow the whole code just doesn't feel as clean to me as the char * version was. (Surely, I could restart and address your particular immediate feedback only...)

I'd really like to stick to the char * version for now, and focus on more important things... there's a lot left to do with keyboard handling, this PR is only about 1/4 - 1/3 of the way there.

[egmontkob]

How do you like the direction of the latest push?

Since I've worked on it a lot, I thought I'd upload it.

The encode_controls() / decode_controls() methods now support embedded NUL in the raw sequence.

encode_controls() takes a char * string and a ssize_t len parameter, the latter potentially being -1 for C-terminated strings; a pretty standard practice seen in GLib and elsewhere so that you don't have to create a GString if you don't have one. Currently there are two callers of this method: mc_config_set_escape_sequence_list() which does have a GString and the unittest which doesn't. I can modify to take GString if you prefer that.

decode_controls() returns a GString * because here the return value is the one potentially containing an embedded NUL.

The retval of encode_controls() and the input param of decode_controls() are still a char * because here we don't need embedded NUL. I still don't like the idea of encode_controls() returning a GString * just because it happens to have one internally, but if you really insist then I'll do it to be able to move forward with the rest. Also please then advise about the input param of decode_controls(), should that remain a char *?

Much of the internals have been reworked to carry a possible internal NUL of the escape sequence, although it doesn't work, it would need further investigation which is not my focus right now. That being said, if we ever happen to need that, the current underlying work might become useful.

Please advise whether I should continue this latest version, or the one before (with char * everywhere), and also if you really insist on converting both methods' output params to GString *.