-
Notifications
You must be signed in to change notification settings - Fork 23
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
fixes for Velocity on Windows (switching from 'start' technique to 'cmd /c') #47
Conversation
…md /c' technique)
src/dash.ts
Outdated
win32: 'start dash-plugin:// && start', | ||
// Same technique as Silverlake Software's "Search Docsets" extension, | ||
// which is written by Velocity's developer and is tested to work with current Velocity on W10. | ||
win32: 'cmd.exe /c start "" ', |
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.
@salembeats have we tested this with Zeal?
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.
I haven't wanted to try it out on Zeal on my main machine because installing Zeal breaks Velocity's integration for Dash URLs. It should work, though. I'll test it on a VM in a minute.
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.
src/dash.ts
Outdated
win32: uri.replace('&', '^&'), | ||
}[this.OS] || '"' + uri + '"' | ||
); | ||
return ({}[this.OS] || '"' + uri + '"'); |
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.
should we simplify with this instead?
return '"' + uri + '"';
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.
Either way works. I kept it open like this just in case any OS-specific formatting is needed again for some reason.
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.
If I were really going all-out into my own personal coding style, and didn't want to keep the (currently no-op) switch expression, I'd probably use backticks:
return `"${uri}"`;
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.
(... especially since the eyes start to blur on the adjacent single/double quotes... )
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.
yes, using your solution to use string interpolation is much better. 👍please change it
for OS specific, let's just remove it since we don't use it anymore, we can just refer to it in git history eventually if we need it again.
Perhaps we can get rid of getOSSpecificURI
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.
Done and done.
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.
Simplified the code a bit, then looked at the new merge conflicts and changed it to be compatible with the new "exact" options.
@salembeats we still have a conflict file here 😃 |
Yeah, it's been about a month, so that's not a surprise. Should be good now. |
Let me double-check a few things and make sure types are right and all of that. |
There we go, good to go. Class is much more concise. |
looks good |
No description provided.