-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Improve OSC 9;9 parsing logic & add tests #8934
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Yea I think there are enough test cases for this now 😋
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.
Thanks!
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.
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