Improve OSC 9;9 parsing logic & add tests#8934
Conversation
|
I'll admit. I have no idea the I purpose including this in servicing release because the bug will frustrate oh-my-posh users since oh-my-posh by default will trigger the failure described in #8930 . |
| else | ||
| { | ||
| // If we fail to find the surrouding quotation marks, we'll give the path a try anyway. | ||
| // ConEmu also does this. |
There was a problem hiding this comment.
This is why I thought I was doing it right. ConEmu won't blame users for forgetting “. I think we should follow,
| const auto path = til::at(parts, 1); | ||
| // The path should be surrounded with '"' according to the documentation of ConEmu. | ||
| // An example: 9;"D:/" | ||
| if (path.at(0) == L'"' && path.at(path.size() - 1) == L'"' && path.size() >= 3) |
There was a problem hiding this comment.
Note that this is actually making OSC 9;9 even more Win32-specific. Since " is an invalid dir name on WIndows, but is a valid dir name on Linux.
There was a problem hiding this comment.
To be specific, when we see a path provided by OSC 9;9 , for example, "D:" on Windows, the actual path is guaranteed to be D: because neither " nor : is valid character in dir names on Windows. But on Linux, you can create a dir that's literally named "D:" by mkdir "\"D:\"".
I can't blame ConEmu for designing the sequence this way since it's a Win32 native application at the very beginning. Just to point it out.
Also CC @Maximus5 for awareness.
Update: on second thought, maybe I'm a bit paranoid about this. The CWD should always be an absolute path, right? There's almost no chance an absolute path would both start and end with " to cause the confusion. I sincerely hope the confusion I described only exist in my imagination.
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. 😉
|
|
|
||
| // These OSC 9;9 sequences will result in invalid CWD. We shouldn't crash on these. | ||
| stateMachine.ProcessString(L"\x1b]9;9;\"\x9c"); | ||
| VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"\""); |
There was a problem hiding this comment.
In the future I think we should consider this an invalid sequence, especially after we figure out the way to identify WSL profiles and make OSC 7 work. Right now I'm not sure how to do with it.
zadjii-msft
left a comment
There was a problem hiding this comment.
Yea I think there are enough test cases for this now 😋
DHowett
left a comment
There was a problem hiding this comment.
Thanks! This is excellent as always.
|
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
🎉 Handy links: |
@skyline75489 I'm implementing this for oh-my-posh V3, if I get this correctly omitting the |
|
@JanDeDobbeleer Yeah that is correct. Just keep doing what v2 does is OK. Thanks for the great work by the way :) |
This PR fixes the parsing of OSC 9;9 sequences with path surrounded by
quotation marks.
Original OSC 9;9 PR: #8330
Unit test added. Manually tested with oh-my-posh.
Closes #8930