OSC 8 on Windows not working well by default #522
Comments
|
I just added OSC support in my new SGR parser, which ignores ("swallows") all OSC sequences, except OSC-8 where it can add underline to the link text (and the URI is hidden/ignored as part of the OSC sequence). It's not too many lines of code (adds 3 new states to the parser), and it doesn't make the parser more complex, so it's probably best to leave it to the SGR processor on windows, and not handle it specially elsewhere in "less" (beyond the existing special handling). As a side note, I know we said we'll add the new windows SGR parser after the release, but it's been months since then, and we still don't have a release, so if you want, I can open a PR to enhance the SGR processor with the following:
I've been using this new parser (sans the new OSC[-8] states) for probably about a year now, and did not notice issues with it, so let me know if you want me to open a PR. |
|
Sure, go ahead and file a PR for the new SGR processor. I'm trying to get the beta closed but #515 and #521 have delayed that. I will release 657 as a beta today and if no new issues arise I expect to do the production release in a week or two. I have thought a little about what to do with terminals that don't support OSC8. You're correct that there's no terminfo flag for this. Previously when only SGR (ESC...m) sequences were supported, the -R flag was sufficient to enable/disable the pass thru. Now that OSC8 is supported, perhaps there should be another flag or an environment variable for finer control. That would go in the next release of course. |
You mean you'll try to merge it before the release? or file it and it can be handled soon - after the release?
Not sure about that. I think even terminals which don't support it still know to ignore OSC sequences which they don't identify? AFAIK, the generic form is At which case, it would look like plain text in such terminals (and in "less"). I think that's by design of how OSC-8 works (maybe not the greatest design, because the URI is lost in such case inside the ignored OSC-sequence, and leaves only the link-text visible). It might be OK to underline it in such case (or use some other customizable attribute with But, if you feel like it, then maybe allow also replacing it with some attribute, but IMO not by default. Also, in my new OSC handling, I decided to simplify it and do one of two things:
This simplifies the state machine from detecting OSC and also OSC-8 (and whether or not it sets the link or resets it) to only detect OSC sequences. |
No, I think that would be too big a change for the current release. I was thinking it would be merged after the release. I'm currently experimenting with some changes that will go in after the release so having a PR with the code to examine would be useful now.
Yes, I think most terminals ignore OSC sequences they don't support. I was thinking there may be some terminals which don't do that because they're very old or too rudimentary (like the current SGR support in less!) but perhaps it's not really an issue. BTW, ECMA-48 (section 8.3.86) says an OSC sequence is simply "ESC ]", terminated by "ESC ". AFAICT, the "NUM ;" part that comes after the ] in many OSCs is an extension of the spec. The use of BEL instead of ESC\ to terminate is also an extension of the spec. |
OK. You can see a near final version here and the OSC support added here, just replacing the current code instead of disabling it with
Right. It's 8.3.89 and the termination is ST, which is Either way, in my code it's detected by |
|
FYI, v657 is not tagged. I think you typically tag versions? |
|
Thanks. I did make a tag when I built the release, but sometimes I forget that |
|
By the way, I see this in version.c: If that refers to #521 then unless you know better, I don't think it's only with LESSOPEN. It's a buffer overflow in fexpand, which I think doesn't depend on LESSOPEN, It probably only overflow if the malloc block size is the same as the file name sans the The implication depends on what gets overwritten, and it can be basically anything, depending on allocation order etc. I only noticed it on windows with LESSOPEN, but I'm sure it can result in other bugs under different circumstances. Also, it's a relatively recent regression, so it probably doesn't affect versions more than a month or so old. |
|
I've merged output.c from avih/less to the post659 branch where I'm currently doing development (change 22ac9f0). Is that the only file that's needed? I've done minimal testing so far. Let me know if you see any problems. |
|
I didn't notice any problems, and that's the final logical functionality I intend to have with this change. But I have locally some refinements and changes that I didn't yet push: mainly using a more scientific RGB to 16 colors conversion, which works better. In this case I wanted to PR a set of 3-4 incremental commits, each explaining what and why is changed. So when work resumes at the main branch, can you skip 22ac9f0, and instead use the PR (which I intent to open in the next few days)? Once we get that merged, which means This means first to remove all the difference between win10 and other dos/windows to to a single statement "on DOS and windows before 10, 256 and true colors are converted to 16 colors". This should be trivial. And then decide which differences between *nix and windows we want to keep - which I think mainly comes down to whether the custom content rendering of underline should stay, which personally I still find useful. Another useful thing to do later is improving the default behavior on VT terminals, i.e. supporting 256 and true colors with -R by default (currently the default - without -Da, is to convert to 16 colors), which would be trivial (like |
|
I'm guessing post659 would be merged soon to master. Since you mentioned you won't merge commit 22ac9f0, this is a reminder, and also a question how would you do that, technically? I'm guessing I've not touched it recently, but the main issue this commit has (other than the minor changes I mentioned above) is that it overrides the latest "make 256/true colors work out of the box on windows 10", and I'm not yet sure how this approach can work with the new parser, which is one of the reasons I didn't open another PR yet. |
|
Yes, I expect to finalize the 66x release soon and after that do the merge of post659 to master. I guess I was expecting that you would open a PR for your new parser and that would overwrite the 22ac9f0 change. What are your plans for the new parser at this point? |
Functionality wise, same as now on win10+ (256/true colors working out of the box), plus where 256/true colors are not supported (or maybe with some flag), convert 256/true colors to 8/16. Technically-wise, not sure yet. The new parser doesn't lend itself nicely to ignore the input for "unknown" sequences, so maybe it would still parse and identify all input, but will know what to do with 256/true colors depending on terminal support. Timeline and git wise, hopefully soon, and after there is a base version to work on without commit 22ac9f0 getting in the way. I.e. it should be not enter master somehow, which is why I asked how you're gonna do that. |
|
LGTM. Thanks. (I didn't check that it leaves no traces for that commit, but I trust you that the revert is full). But why not instead skip that commit when you merge it into master (e.g. rebase -i and delete it in post659 or in some temp branch before it's merged to master), so that there are no traces that it ever existed (because on master, it indeed didn't). |
|
The master branch has been largely frozen for few months since the 661 release, and further few months even before that to avoid breaking changes before a release, which makes it about 6 months without being able to push to master anything more than small fixups. Maybe with future releases, instead of opening a post-659-like branch for non-fixup commits, just push non-fixup commits to master, and if there's a need to a fixup release, then open a fixup branch, e.g. v661.1 etc ? And maybe similarly branch v661 when it nears release, so that master can keep getting meaningful changes at all times, while the release/fixups branches are the ones which remain conservative? or the 661 branch can also accumulate fixup after the release towards 661.1 etc. This will also not require contributors to know that they need to base their code on post-xxx branch rather than on master, etc. Maybe use slightly different branch names to avoid name clashing with tags. e.g. maybe the branch names are without Maybe also release tags can be vXXX-rel or some such to distinguish from in-development tags which don't become releases. EDIT: I see that there's already v661-rel. nice. |
Currently, by default, the internal SGR processor on Windows does not identify OSC 8 correctly, and renders incorrect text (shows both the URL and text, etc) on Windows prior to 10.
On windows 10+ it does work:
-Da- OSC 8 does render correctly, as this bypassed the internal SGR processor and goes directly to the terminal.-Da, the internal SGR processor gives up on any ESC sequence which isn't CSI, and passthrough the rest of the buffer unmodified to the terminal, so it works (because VT is enabled by default on Windows 10+).Adding OSC-8 support to the internal SGR processor is not impossible, and it might be a good solution, however,
since "less" already knows to parse OSC-8, I was wondering if it could be translated to underline the link text on terminals/setups which can't handle OSC -8.
One such case would be Windows if VT is not enabled or if
-Dais not used[*]. Other cases could be controlled by some option to indicate that the terminal doesn't support it (I presume there's no terminfo flag for OSC-8 support?).I didn't yet look at the code, but wanted to hear your thoughts about this.
[*] including the case where
-Dais not enabled, because passthrough-to-the-end-of-the-buffer can include SGR sequences after the OSC-8 - which the internal SGR processor would not track during passthrough, which can result in rendering bugs.The text was updated successfully, but these errors were encountered: