Buildkite timestamps only at start of line, via screenLine.metadata

[pda]

As discussed in #96 and #97, Buildkite timestamps (<ESC>_bk;t=123456<BEL>) were corrupting virtual terminal X position when moving the cursor backwards through them (“CUB”: <ESC>[<distance>D). This is because they occupied a position on the terminal intermediate representation X,Y grid, and parsing/appending them incremented the internal X-position state. Buildkite timestamps should generally only appear at the start of lines, where this isn't a problem, but certain ANSI sequences cause buildkite/agent to place them mid-line.

This pull request introduces a “line metadata” structure, supporting namespaced key:value data attached to entire lines. For example the Buildkite APC <ESC>_bk;t=1234<BEL> places t→1234 under the bk namespace. Subsequent _bk;t APCs on the same line are ignored. The bk namespace is then rendered as <?bk t="1234"?> prior to the line's terminal data, so it's always at the start of the line.

Some refactoring to achieve that:

• _bk data is no longer represented as a subtype of struct element, but as metadata map[string]map[string]string on a new struct screenLine which wraps the bare []node that previously represented a line.
• A new screen.setnxLineMetadata() method, with a Redis-inspired name indicating that it only sets keys that don't already exist.
• screen.getCurrentLineForWriting() *screenLine replaces the previous growScreenHeight() and growLineWidth() since those were always used in conjunction to prepare for writing data that might be out of existing bounds, and this was also required prior to adding _bk;t metadata to a new line.
• rendering <?bk …?> to HTML is separated out of the rendering of struct element because it's no longer one of those.

This PR is currently based on some other more minor refactoring that was extracted from a previous approach that I don't want to merge.

Related:

• Fixes Bug with rendering characters in Gradle progress output #96
• Closes Buildkite timestamps are zero-width via persistent elements #97
• Refactor before fixing zero-width bk APC timestamps #98
• Prefixer for ANSI timestamps overzealous on escape-code-new-lines agent#1738

[moskyb]

This LGTM other than some minor readability and stylistic quibbles. Awesome work!

[moskyb]

I'm not 100% sure if we have another use for i here, but if not we could replace it with either of these:

Suggested change

	keys := make([]string, len(data))
	// Make a list of the map's keys
	i := 0
	for key := range data {
	keys[i] = key
	i++
	}
	sort.Strings(keys)
	keys := maps.Keys(data)
	sort.Strings(keys)

or alternatively, if we don't want to use the experimental maps module:

Suggested change

	keys := make([]string, len(data))
	// Make a list of the map's keys
	i := 0
	for key := range data {
	keys[i] = key
	i++
	}
	sort.Strings(keys)
	keys := make([]string, 0, len(data))
	for key := range data {
	keys := append(keys, key)
	}
	sort.Strings(keys)

[pda]

Yeah interesting. TIL about https://pkg.go.dev/golang.org/x/exp/maps but I think I'll avoid bringing that in while shifting this existing code. Hopefully x/exp/maps is in stdlib by the time we look at this again and we'll use it then :)

Re. eliminating the i++ etc, looks like the existing code was following the pattern described in https://stackoverflow.com/a/27848197 which is “slightly [less] concise, but slightly [more] efficient … You already know how big it's going to be so you don't even need to use append”.

I'll just let sleeping dogs lie 🙈

[moskyb]

we could probably do this a little more rigorously by doing:

Suggested change

	value := strings.Replace(data[key], `"`, """, -1)
	value := html.EscapeString(data[key])

... buuuuut, looking into it, that method uses and HTML escape code for quotes (") rather than an entity name ("), so if we can't deal with that on the backedn then we should probably keep as is.

[pda]

👍🏼 I think the numeric entity would be fine, and escaping the other 4 chars (<, >, & and ' in addition to ") would probably be beneficial in cases where we include those chars in the input.

However, another let-sleeping-dogs-lie for this existing code that I'm moving 😅 🐶💤🛌

[moskyb]

🙌