-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
initial draft of VT function support spec #1884
Conversation
Perhaps to preserve tabulation we could use ✅❎ emoji instead of markdown checkboxen? I realize that on my phone the X is in green just like the check is, but perhaps GitHub has something different on offer? ❌ |
Well, my thought was that when this is completed, it should be embedded in an open issue as a specification, representing "VT support" -- that way we (developers) can simply check a box when the feature is implemented. It might serve as a master list of implemented sequences. Does this make sense? |
Ah, yeah, that seems reasonable! |
The plan here was to also include the columns where we first introduced it to the conhost and what level of actual test/fuzz/etc coverage and support it has, right? |
@miniksa Yes, absolutely, but I let's get the "minimum viable product" out there first and then we can dig into the extras. You say what "level" -- do you see test/fuzz/coverage for an individual sequence as a non-binary evaluation? |
I mean, in theory, it shouldn't be GREEN or GOOD unless all of the things are covered. But in practice, I'm going to guess that the (UnitTested&&Fuzzed&&FeatureTested) is going to be FALSE for nearly every row in your table. So it might be nice to show that some would be TRUE if only we'd fix the Fuzzed portion of the matrix. Might have to be a manual or manually assisted process... |
Ah, I think I'm going to drop the spec/checkbox task list trickery. Not supporting them inside tables in github markdown really prevents me from coming up with functional and informative layout. There are too many columns now and without a tabular layout, it'll be a bit too dense to visually parse easily. I'll take a stab at doing it like @DHowett-MSFT says, with emoji. Ultimately if it's going to be driven from code, then the checkboxes are not that neccessary. |
REBASE HELL. WHO IS THE IDIOT WHO GAVE ME MY GIT LICENSE |
4c93bf0
to
8088f4a
Compare
VT Function SupportTable of Contents
Code Extension FunctionsControl Coding
Character Coding
Graphic Character Sets
Terminal Management FunctionsIdentification, status, and Initialization
Emulations
Set-Up
Display Coordinate System and AddressingActive Position and Cursor
Margins and Scrolling
Cursor Movement
Horizontal Tabulation
Page Size and Arrangement
Page Movement
Status Display
Right to Left
Window Management
Visual Attributes and Renditions
Line Renditions
Character Renditions
Audible Indicators
Editing FunctionsDEC Private
OLTP FeaturesRectangular Area Operations
Data Integrity
Macros
Saving and Restoring Terminal StateCursor Save Buffer
Terminal State Interrogation
Keyboard Processing Functions
Soft Key Mapping (UDK)
Soft Fonts (DRCS)
Printing
Terminal Communication and Synchronization
Text Locator Extension
Session Management Extension
Documented Exceptions
Generated on 08/06/2019 23:13:39 |
a3437d4
to
a7bad84
Compare
Hey @oising -- is this good to have the draft tag ripped off? Or does it need a couple more rounds? |
@DHowett-MSFT Oh, jeez, it's still marked draft? Let me give it a once over this evening and I'll get back to you. I think it needs to be released into the wild and let people shoot it or feed it. |
Thanks! 😄 |
@DHowett-MSFT what do you think anyway, should it go into the build pipeline to generate an asset for the release? |
I think that's a very cool idea. In a pinch, if we do not feel a pressing need to publish that asset alongside the release, we can drop it in or adjacent to the wiki too |
@DHowett-MSFT yeah, it would good to at least be able to tag it as a point in time reference to the identically tagged source |
Ok, I've refamiliarized myself with my frame of mind, and I've got a tweak or two to implement. I'm on it! |
I'll be at this forever... it's been too long already. Have at it, and give me some specific things to fix up. Would be nice to get this into a devops pipeline or something. |
he lives! |
I had a insanely busy couple of months on a new project, and then fell and gave myself a moderate concussion before the new year. Only easing back into screens and stuff now... sigh. |
I suspect you already know this, but in case you aren't aware, a number of the links in the symbol column don't work. I'm assuming they were all just generated automatically from the name, but not every control sequence has its own page in the VT510 manual. For example, I think the best you could do for the control characters would be to link to the tables in chapter 4 (here and here). Also, just from a brief scan of the list, I think there are quite a lot of sequences that I would have considered implemented, but aren't ticked in the conhost column. Would it be helpful if I made a list, or should I wait until this is merged and submit a PR? Edit: I've only realised now that the documentation in the comment above isn't an actual file being committed, so suggesting changes to that isn't going to help much. 🤦♂️ The ps script is beyond me, but I guess it's magically generated by scanning the source code somehow? I can understand it may not have been run in a while, so won't have picked up some of the recently added sequences, but the fact it's missing basic control chars like |
I've now had a chance to try out the script on my local build, and it's definitely still missing operations that we do support, but many of them can quite easily be fixed just by updating the comments. There are operations that are aliases, which just need to have the alias name added to the comments (e.g. Then there is
Finally there are the controls that we do support, but don't pass through the dispatch interface, so they're never going to be picked up by this script. This includes Anyway, is it worth my putting together a little PR to update the comments for the operations that can easily be fixed? Or would it be better if that was included as part of this PR? |
ECMA-35 and ECMA-48 are better references to work from for the stuff that is not DEC private. For starters, they explain the actual structure of control sequences (with parameter, intermediate, and final characters) and indeed what "private" means. There are ITU standards from the 1990s that deal with SGR indexed and direct colour control sequences, moreover. This StackExchange answer should help with reference doco. |
OK, I've started putting together a PR to update the To start with, I think we need to update the CSV to give each of the I don't know if we also want to keep I'm not sure this is a workable long-term solution though. If/when we support additional reports, they may be broken out into separate methods, but they're unlikely to be added to the The fact that the various mode operations are in the interface seems like a mistake to me - technically they should also be private. The only reason I can think they might be there is for testing purposes, but even that seems like a hack. That said, this isn't a problem we need to solve now, so we can worry about it later. I just thought it worth mentioning. The other thing I noticed when going through the list, is there are quite a lot of reporting sequences that are essentially listed as two entries. For example, we have the |
… VT commands that are supported. (#4752) ## Summary of the Pull Request Most of the methods in the `ITermDispatch` interface have a comment following them that indicates the VT function that they implement. These comments are then used by the script in PR #1884 to generate a table of supported VT functions. This PR updates some of those comments, to more accurately reflect the functions that are actually supported. ## References PR #1884 ## PR Checklist * [ ] Closes #xxx * [x] CLA signed. * [x] No new tests. * [x] No new docs. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #1884 ## Detailed Description of the Pull Request / Additional comments In some cases there are methods that implement multiple VT functions which are essentially aliases. Originally the comments listed only one of the functions, so I've now updated them to list both. This includes `HPA` as an alias of `CHA`, and `HVP` as an alias of `CUP`. Similarly, some control characters are implemented in terms of another VT function, but only the main function was listed in the comment. Again I've now updated the comments to list both the main function and any related control characters. This includes `BS` (sharing the same method as `CUB`), `HT` (the same method as `CHT`), and `LF`, `FF`, and `VT` (the same method as `IND` and `NEL`). Then there were some minor corrections. The `DeviceAttributes` method was commented as `DA`, but it really should be `DA1`. `DesignateCharset` was simply commented as _DesignateCharset_, when it should be `SCS`. The `DECSCNM` comment was missing a space, so it wasn't picked up by the script. And the `SetColumns` comment mistakenly included `DECSCPP`, but we don't actually support that. Finally there is the `DeviceStatusReport` method, which potentially covers a wide range of different reports. But for now we only support the _Cursor Position Report_, so I've commented it as `DSR, DSR-CPR` to more clearly indicate our level of support. In the long term we'll probably need a better way of handling these reports though. ## Validation Steps Performed I've run the script from PR #1884 and confirmed that the output is now a more accurate reflection of our actual VT support.
Hey @oising - sorry we've let this one sit for so long! I'm not sure why this got lost in February, maybe we all panicked over a certain global pandemic or something. I think this looks just about ready to merge - do you want to just give it a quick once-over to make sure it's up to date? I know we discussed having this as a build artifact, but that sounds like more engineering effort and I wouldn't want to hold this up too much longer. Maybe we should just run the script once for the "current state" of the support, and throw that in the docs folder, and update it as needed? The only other thing I'd look for is a quick "this is how you update the doc" oneliner, just so little old me who doesn't know powershell can update the doc without thinking about it 😉 |
I've gone through two new laptops since I started this :P But since I don't need to compile anything to test this, it shouldn't be a problem...
Sure -- let's just get it in there, and iterate later.
Will do. Now I'm about to rebase 769 commits from a branch that has changed name (master -> main) -- I wonder how that will go. EDIT: it went fine :) |
conhost and windows terminal renamed files; updated script to do summary output only, also quiet mode and option file path for output (else stdout) added -quiet and fixed -summaryonly parameters added hyperlink directly to soruce implementation for conhost and terminal seqs Update doc/reference/master-sequence-list.csv Co-Authored-By: James Holderness <[email protected]> Apply suggestions from code review Co-Authored-By: James Holderness <[email protected]>
6a52aab
to
afece10
Compare
New misspellings found, please review:
To accept these changes, run the following commands
✏️ Contributor please read thisBy default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.
If the listed items are:
See the 🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉
|
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.
lets ship it and iterate on it in post. We've got the commandline posted in chat, so I'm less worried about that. Let's just get the spellcheck bot happy 😄
The spellcheck thing, is that a you job or a me job? |
Typically that's the responsibility of the contributor, but I don't think the CI will block an external PR over it. If this gets the signoffs before the spellcheck fixes, then I'll just take care of 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.
Lovely. Lovely lovely lovely. Thank you. This is gonna be the lowest numbered PR merged in a long time--thanks for hanging out with us. 😄
@zadjii-msft since I'm your second signoff, let's do the spellbot and merge!
(I'm just gonna fix the spellbot in post, because I end up breaking other people's PR's literally 100% of the time I try pushing to their branches) |
Nice! It'll be so much easier to submit incremental improvements now.... |
I'm making this a draft PR while I continue to parse out data, format and add value (like sync scripts etc)
TODO:
adaptDispatch.hpp
terminalDispatch.hpp
sequences.csv
sequences.csv
to represent sub operations (e.g. SGR ops supported)