Skip to content
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

Understand (and discard) SGR subparameters #180

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jfly
Copy link

@jfly jfly commented Oct 8, 2024

This completes #178. pyte now understands :-delimited subparameters, but doesn't actually do anything with them (which is better than the previous behavior, where they'd get spewed to the screen buffer.

I've split the work here into 2 separate commits, each with some explanation in the commit message, I recommend reading them independently.

I've also laid the groundwork for actually using these subparameters, which I'm tracking in #179

jfly added 2 commits October 8, 2024 12:15
Now we do the math as we find each digit, rather than at the end after
we have collected each digit.

It's possible this is faster than extending a str and doing an
`int(...)` conversion at the end (I haven't profiled it), but that's not
why I made this change. I made this change to support an upcoming change
to support SGR parameter "context", which is when a SGR CSI parameter
is actually a list of integers: <selectel#178>.
It's nicer to be able to collect the integers as we go, rather than
having to process them all at the end.
This fixes selectel#178.

Before, we assumed that CSI parameters are only integers. However, that
caused us to barf in weird ways when presented with CSI parameters that
contain `:`-delimited subparameters.

This update to the parsing code causes us to parse those subparameters,
and then immediately discard them. That may seem kind of weird, but I'm
laying the groundwork for a followup change to the SGR handling code to
actually be aware of subparameters
(selectel#179). I just didn't want to
muddy this diff with that change as well.
@jfly
Copy link
Author

jfly commented Oct 14, 2024

@superbobry, do you have any time to take a look at this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant