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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for custom parsing of APC, SOS and PM sequences. #115
base: master
Are you sure you want to change the base?
Add support for custom parsing of APC, SOS and PM sequences. #115
Changes from 1 commit
c515809
907eed3
4d31609
4277fbc
37413f0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is a bit silly because "some other terminals" includes Alacritty. This is mostly an extension to how other sequences like OSCs are handled, which also support the bell terminator and do so already in Alacritty. So this might need some rephrasing to not sound strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was referring to SOS/PM/APC sequences, which are not yet supported by alacritty. The current behavior (treating them as the anywhere state) doesn't seem to support termination with BEL, so this would be a behavior change for alacrity, wouldn't it?
But since as you've said, this is how OSC sequences already behave, so I think we should change this behavior to be more consistent.
As stated above, I added these comments only to explain my reasoning, I didn't intend them to be included in the code in the final version of the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0x18/0x1A is just a general-purpose reset into ground from anywhere, the transition is unrelated to its origin state. There's not really any reason for this comment to exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit confused, since not all terminals seem to do that (at least when handling SOS/PM/APC), but I'll remove the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'll refer to https://vt100.net/emu/dec_ansi_parser for "general" guidance on Alacritty's parser here (even though it doesn't handle opaque strings). It's not really a SOS/PM/APC, but more of an "anywhere" thing. As such a SOS/PM/APC-specific comment is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems misleading. Escape resetting ongoing escape sequences from anywhere is a de-facto standard implemented by many terminal emulators. It might not be explicitly called for in older specifications, but is not incorrect behavior either, it's mostly an implementation detail really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry if I got my intentions wrong. The comments are not meant to be here to stay. They are mostly documenting my thoughts while looking for feedback to this pull request. I have little experience with de-facto terminal implementations, so I can only refer to the specifications (which are a bit vague).
If escape sequences resetting the state is common de-facto behavior, then I would be happy to keep it this way, since it keeps the implementation clean and simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this comment on a method call that dispatches bytes indiscriminately? It might be more appropriate on the match arm instead, but even then I question whether the
0x80
to0xFF
range can be considered "valid characters", considering they're not printable characters (so effectively the same as any other byte).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for that. This is a leftover comment from a previous implementation attempt that I gave up on and forgot to remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than sorting these by start/put/end, I'd prefer grouping them by sos/apc/pm. I think it makes more sense since the grouping into start/put/end is somewhat arbitrary considering they aren't really interconnected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I'll do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment applies for the other functions. Calling these "valid characters" could be misleading to consumers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the naming
_dispatch
is consistent with the rest of our methods. The existing_dispatch
functions generally represent the dispatch of an entire escape sequence in full, while this function just represents the dispatch of a single byte in the sequence.I think this is much closer to the way esc works in VTE, where it is split in
hook
,put
, andunhook
.Other parts of this patch already make use of the
_put
nomenclature, so I think it's better to be consistent and rename these functions tosos/pm/apc_put
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right, considering the method on the internal
OpaqueDispatch
trait is calledopaque_put
and then it ends up callingsos/apc/pm_dispatch()
...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly this trait and its implementations is the main issue I have with this patch. It should work fine since it all just gets inlined and optimized out essentially, but it's still a whole lot of boilerplate.
I'm not sure I have any better ideas for now, but at the very least this trait needs a comment explaining that it's just a helper for dispatching over the opaque string escapes and in practice is inlined everywhere to function as static dispatch without conditional indirection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This trait was the best solution I could think of until now, which means it is the least terrible one. I'll add a comment.