Overwrite line metadata #109
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
triarius
approved these changes
Jul 26, 2023
| } | ||
| p.screen.setLineMetadata(bkNamespace, data) |
| strings.Join([]string{ | ||
| `<?bk t="234"?>hello world!`, | ||
| `<?bk t="456"?>another line!`, | ||
| }, "\n"), |
There was a problem hiding this comment.
It's just a test, so it's moot, but I wonder if the compiler in lines this? At least with the alternative using +, you have this garantee: https://go.dev/ref/spec#Constant_expressions
There was a problem hiding this comment.
I wasn't 100% sure, so I compiled a test binary and went to crack it open with Ghidra, except I don't have it installed, and it relies on the JDK. Which reminded me that Java (at some point) compiled repeated string concatenations with + into a StringBuilder under the hood.
Anyway, I expect the answer is no. Certainly it doesn't create a single string constant.
| } | ||
|
|
||
| for i := xStart; i <= xEnd && i < len(line.nodes); i++ { | ||
| line.nodes[i] = emptyNode |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What timestamp should a rewritten line have?
I argue we should replace old line metadata with new metadata. This will make timestamps look more sensible in
ESC [1A ESC [K-heavy output.For example, when faced with updating the existing screen (pseudo-rendered):
with a sequence like
ESC [1A ESC [K ESC _bk;t=200 BEL Completed! LF, we currently getwhich is nonsensical from a timestamp perspective (
Completed!actually happened at t=200).The current approach does have a benefit: assuming the timestamps are non-decreasing, it is somewhat possible to derive the "duration" of a line (assuming each line is printed at the very start of a chunk of processing). In the example above,
Commencing build...can be inferred to take Δt = 1; ifCompleted!has t=200, thenCommencing build...'s inferred Δt = 200, when 199 of that was actually spent starting with package enumeration. But to do this effectively requires making that assumption.I also tweaked the style of
setLineMetadata, and implemented an optimisation forclear.