Updated v2 of the layouts to use a subset of JSON.

I think the file comment should be less about how the code works and what the functions are called and more about how the format looks. It should list the keys and their meanings at least.

In layout_string_init it seems unnecessary to memset all 8K when you are just going to overwrite it. I'd just set write and be done with it.

In layout_string_copy it is inefficient to use strlcat when you know the write offset, because strlcat will need to scan the string again to find same. You can use memcpy since you know the space available, or snprintf.

Can you make layout_string_format a function instead of a macro? Macros like this are hard to read. You could make int layout_string_write(struct layout_string *ls, const char *fmt, ...);.

It would be nice to unify v1 and v2 so they both use the same struct layout_string buffer. Or alternatively I am OK with moving v1 into a layout-custom-old.c file and leaving it entirely unmodified except for code to change function names and to skip floating panes.

A lot of the functions are missing comments above the function definition. It should be all or none in a file (preferably all).

You can't do this:

sscanf(layout, "{\"V\":%d,\"L\":%n", &version, &n)

Because JSON fields are not ordered, so you could receive L before V, or with new fields we add later in between. Also spaces need to be ignored between tokens. It is perfectly valid for someone to have say { "L": {}, "V": 99 } or with newlines. I think we will need to build a separate parse step so we first run over the string and pull out the keys and values. This could either be generic for JSON (so build something which is just key-value and doesn't care about what keys there are), or it could understand the keys and build say struct layout_parse_input { int version; ... }.

Ah yeah, whitespace. I will write a tokenization step. I wanted to keep the versioning contained to the file, so I"ll adapt version 1 to use the same string building method as version 2. I'm not sure if it will change with the with the addition of the tokenization step, but I'll unify them. I was trying to do something too cute with layout_string_copy, where I called strlcat on the write pointer, which points to the end of string. It should just be a memcpy. Thank you for the feedback!

Oh, and as for the comments. It felt strange to duplicate what the code was doing with a comment at the top, but I'll change it to document the format, and document each function.

I have added a string tokenizer and a JSON parser that is tuned to v2 of the format. The struct naming isn't perfect. I hope I didn't go overboard. I wanted to write something that could be extended in the future with minimal friction. It needs a bit more testing, but the plumbing is all there and it works on common examples. What do you think?

I believe I addressed the rest of the feedback pertaining to constructing the layout strings, and I converted layout_append_v1 to use the new layout_string_write.

Oh, I should mention. There is possible follow-up work to do. At this point, some of the pane keys are ignored (last, active, zindex, and id). Some of these may be useful for reconstructing a layout. We can save and apply this context later from the evaluation step in a struct that hitches a ride though the call chain. Also, **cause can be passed though the tokenize, parse, and eval steps if we want better error messages. Both these points were out of scope for this PR.

EDIT1: I should also mention that the tokenizer does not handle escaped characters other than whitespace. Didn't seem necessary at this point.

I think I'm going to try and simplify the memory management of the parser. The general structure still stands though!

Don't return NULL if no client in format_cb_window_layout or you will break use of the format from a config file. Just assume the client is not a control client if not set.

OK some more comments:

Same as above for format_cb_window_visible_layout (handle ft->c == NULL).

Comments should be neutral and third person that is don't address the reader, so change Please work off of the current format.. I would just say "... deprecated at some point in future and should no longer be used.". I would expand the next paragraph though to list the fields out a bit more:

* The current (v2) format is JSON. The top level has two key:
*    "V": version number, currently 2;
*    "L": array of layout cells
* Each cell is an object with:
*    "w": cell width
*    "h": cell height
*    "x": horizontal position
*    "y": vertical position
*  If the cell is a node cell (with child cells), it additionally has:
*    "c": array of child cells
*  If the cell is a leaf cell (that is, containing a pane and no child cells), it additionally has:
*    "i": pane ID as %n
*    "l": index into last panes list, if not the active pane
*    "a": true if the active pane
*    "z": z-index, if a floating pane
*/

I would omit documenting the v1 format since we do not want people to use it, if you want to say it as a checksum and v2 does not that would be enough.

I don't think KEY_* defines buy us anything except another layer of indirection. I would remove them and just use "V" etc inline. Definitely the same for VAL_TYPE_*.

statics and enums and structs should still have layout_ prefix.

I think TOK_OPENCURLY would be better as OPENOBJECT and OPENBRACKET would be better as OPENARRAY, that is, name them after what they mean not the character type. That also avoids people getting confused with terminology because to many people brackets are () not [] (the latter would be "square brackets"), or "curly brackets" are "braces".

JSON integers can signed 64 bits so int64_t not int, it does not support unsigned integers though (somewhat annoyingly), so int64_t is enough.

I think we should support at least \" inside strings unless it is very difficult to do. I can't remember if \\ means \ but if it does we should support that too?

In layout_string_write, you do not need tmp, you can just drop the x and do:

        int len;
...
        len = vsnprintf(ls->write, remaining, fmt, ap);
        va_end(ap);
        if (len < 0 || len >= (int)remaining)
                return (-1);
        ls->write += len;

You know LAYOUT_STRING_MAX is small so no fear of overflowing INT_MAX here. This will \0 terminate it for you also.

I believe I have addressed everything you mentioned. I also got rid of all the string allocation in the parser and implemented views to use on the input. Made things a lot more simple.

Do you think it is worthwhile at this point to implement a struct layout_parse_ctx which would be returned from layout_construct instead of a struct layout_cell? The context would hold error strings, the layout root, and other data we may want in the future like z_index or last_panes data.

• Can you add /* $OpenBSD$ */ line at the top of json.c? (It will be expanded by CVS later.)

• jstrcmp -> json_strcmp but I would be getting rid of this, see below.

• I don't think json_node_type, json_string, json_node etc should be public in tmux.h. These should all be private and have appropriate accessors (json_get_string, json_first_key and similar).

• This would be more useful and easier to use if there was actual key lookup instead of having to walk all the keys. layout_parse_json_layout knows what keys it wants and what type each should be, so it should do something like if (!json_get_number(json, "w", &n)) ... error rather than walking all the keys. For optional fields add a json_has_key or similar. This means json_nodes should probably become an RB tree.

• I would drop struct json_string and either just allocate the string with xstrndup, or set a reasonable upper limit such as 512 and have char[] in json_node. This is not performance critical so copying is not going to make any difference. If you really want to keep it then just have it inside json.c and have json_get_string do the copy so the caller can rely on having a C string.

• Can you put an empty line before each fail: label?

I would probably keep json_node_type as public and have an API like:

struct json_node *json_find(struct json_node *jn, const char *key);
enum json_node_type json_get_type(struct json_node *jn);
struct json_node *json_array_first(struct json_node *jn);
struct json_node *json_array_next(struct json_node *jn);
int json_get_string(struct json_node *jn, const char **s);
// get_number, get_boolean, get_object

You could probably live without json_get_type since you know what type you want and just rely on get_string and friends to fail or succeed...

Awesome, thanks for the feedback. This all makes sense, ill start implementing. I would like your opinion on my reasoning for the string view and if you think it is useful. The primary reason i started liking the string view is for error reporting. Since it references the input, the surrounding context of the error can be given to the user. We can also store a pointer to the input along with the allocated string. Big picture, is it worthwhile to supply 'N' characters of surrounding context to the user on an error?

I think it is probably more infrastructure than it is worth for this. But you could still do that if you want because when there is an error, you have the context at that point, no? I just mean don't store it as a pointer into the string (or if you do, don't expose that externally - copy it before you do - so that the caller has a normal string and does not have to worry about it being special).

Okay, this is ready for review again. I have added a few additional functions to the api that combine functionality and return references to static values.

OK some comments:

• control_window_layout_changed_cb should not change from control_notify_write to control_write.

• layout_dump ignores floating panes for v1 clients, but it looks like layout_parse still requires the number of panes to match (it calls window_count_panes(w, 1) which includes floating panes). This means a v1 client can't apply the same layout string (or a modified one) if there are floating panes.

• I is validated with strtol but this will allow negative pane IDs or values more than INT_MAX. TBH since I is unused I would just ignore it for now.

• Similarly, i is checked for range but what if I use the same index twice? Or leave some out?

• I do not like the JSON API returning pointers to static storage:

  • Why can json_find_string not just return field->str? If it can't, it should store a cached copy to return in the node on the first call and free it with the node.
  • json_find_number should definitely not return a pointer like this. It should return success/failure and store the number in caller-provided storage, so be like int json_find_number(struct json_node *jn, const char *key, int64_t *n).
  • Same for booleans.
  • I would just get rid of cause from these in the name of making the API simpler, whether the key is not found or is not valid is pretty irrelevant, no?

• INPUT_MAX can be removed if you change json_find_string, but make TOKENS_MAX a number so everyone doesn't need to go and work out what 1<<14 is.

• TOKENS_MAX seems way too big. Make it 1000 or something?

I believe I have addressed most things, but I have a few points of discussion:

1. the json_find_* api is meant to be convenience functions for when you know that a key should exist and what its value type is with error reporting built in with NULL available as a cause input if it isn't needed for a call. I think this is probably the most likely use case for interacting with json in the codebase, so it made sense to me to package it all together. There is much less vertical clutter because of it. This contract ensures that "i" required. If you don't find this compelling, should i scrap it and use json_find and json_get_* calls instead?
2. TOKENS_MAX is directly related to the input string size because in worst case each token is a single character (2-3 for the id in each pane). Given the new format's verbosity, the capacity for panes in the layout has also shrunk. It is probably reasonable to have it at 1000 or something, but it is just something to keep in mind. I put LAYOUT_STRING_MAX back to 8192 and TOKENS_MAX to 1000.

Indexes are now being validated for uniqueness, but not strict sequential ordering because of the bottom right deletion fiting. Let me know how it looks!

A few things:

Try this:

$ for i in `seq 30`; do tmux splitw \; selectl tiled; done
$ tmux selectl "$(tmux display -p '#{window_layout}')"
tokenization error: ...

Also this looks wrong - the floating pane ends up in a weird state... is it meant to be tiled at this point?

$ ./tmux -Lx new -d \; new-pane -x100 -y30 \; a
$ ./tmux -C display -p '#{window_layout}'
%begin 1787565429 463 0
7b70,106x85,0,0[106x85,0,0,0]
%end 1787565429 463 0
%exit
$ ./tmux selectl '7b70,106x85,0,0[106x85,0,0,0]'

The example in tmux.1 doesn't work:

$ tmux select-layout '{"V":2,"L":{"t":"h","w":159,"h":48,"x":0,"y":0,"c":[{"t":"p","w":79,"h":48,"x":0,"y":0,"l":0,"i":0,"I":"%0"},{"t":"p","w":79,"h":48,"x":80,"y":0,"a":true,"i":1","I":"%2"}]}}'
invalid number: 1","I":"...: {"V":2,"L":{"t":"h","w":159,"h":48,"x":0,"y":0,"c":[{"t":"p","w":79,"h":48,"x":0,"y":0,"l":0,"i":0,"I":"%0"},{"t":"p","w":79,"h":48,"x":80,"y":0,"a":true,"i":1","I":"%2"}]}}