Skip to content
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

Move open-in-vscode-via-url to a separate command #7

Merged
merged 9 commits into from
Jun 24, 2022

Conversation

ptim
Copy link
Contributor

@ptim ptim commented Jun 21, 2022

Follow-up to #5 (replaces faulty #6)

Hi @NomarCub ! Many apologies for the very slow reply...

The changes somehow appear more sweeping than I originally intended, but I think it's for the best - the commits are granular, so if anything rubs you up the wrong way (eg - hope you don't mind the dev helpers), let me know and I can adjust / remove!

I tried to make some modifications before merging, but fucked something up, so I merged anyway. Do you mind opening another PR with some adjustments?

On Windows, due to the dialog that comes up with URL, the second window.open never manifests. This also means that you can't specify to not open the file currently active in Obsidian.

Aha - I don't have a windows machine to test on, but I would have thought that this might cause a failure on the first invocation, but would work on subsequent invocations after the user had satisfied the dialog... Are you finding that you have to address a dialog every time?

I've made a toggle for "Open file", so users should be able to find a config that works well for them.

For me it'd make sense if the useWorkspace was a checkable setting, transparent to the user.

Gotcha... as I was reviewing this, I thought it was cleaner to have two separate commands; in this way it's transparent to the user in that they can set shortcuts as desired 👍

For me, it's as simple as 'set my keyboard command to open-in-vscode-via-url', then the defaults are good. Changes I made to CLI command have been reverted, and it remains bound to the Button (this could be a setting if you think it's important).

It'd also be nice if you could use the {{}} variables in the the URL workspace path setting.

Done! (for {{vaultpath}} - the URL protocol is not as flexible as the CLI, so filepath doesn't make sense in this context, I think, as we have a toggle for this purpose)

Hope that helps - let me know what you think, Tim

screenshot - 2022-06-21 - test vault - Obsidian v0 15 2 - 2022-06-21 at 21 48

@NomarCub
Copy link
Owner

Hi! Thanks for the fixes, I like the new structure too. If there's something to change in the future with the dev features I might ask you for help.

Two more things:

  • I'd like to be able to set the ribbon to the new command too. So instead of true and false, there should be 3 options, now for no ribbon and one each for the commands.
  • The HACK part is well, hacky 😄 On Windows you get two prompts. If VSCode was not open before and if you didn't press yes in time, VSCode makes a popup saying there's another instance of Code running but not responding. There should be an option to not fire the second window.open(), in order to get back the old behaviour, I think that's enough.

@ptim
Copy link
Contributor Author

ptim commented Jun 24, 2022

I'd like to be able to set the ribbon to the new command too. So instead of true and false, there should be 3 options, now for no ribbon and one each for the commands.

Certainly.. here we go (I think an additional toggle is the cleanest config):

screenshot - 2022-06-24 - test vault - Obsidian v0 15 2 - 2022-06-24 at 10 04

The HACK part is well, hacky 😄

Yeah! As I mentioned, I've upvoted the VSCode feature request that would resolve this issue - would encourage all interested parties to do so! FR: Allow a vscode://file to open in the window that has the workspace opened · Issue #150078 · microsoft/vscode

On Windows you get two prompts. If VSCode was not open before and if you didn't press yes in time, VSCode makes a popup saying there's another instance of Code running but not responding.

Bummer! In the land of workarounds, this sounds like a documentation issue.

After a bit of reading, it seems that all of this is due to a recent change to VSCode: URL protocol: confirmation dialog · Issue #95670 · microsoft/vscode which addresses a security concern on windows: Prompt users when opening vscode://file/... URIs · Issue #95252 · microsoft/vscode. I've added some docs to the readme and settings 👍

This kind of thing was a reason for separating the two commands - one workflow might be: initially open the vscode using all the configurability of the code command, then for subsequent openings, use the URL method.

Again, the reason I'm invested in the URL method is because I switch back and forth regularly, and there's really discernible difference between the methods in terms of speed: URL is just above perceptible speed for me (thinking of that 200ms rule of thumb... haven't measured it, but it's quick!), vs over a second of waiting time for the CLI invocation.

There should be an option to not fire the second window.open(), in order to get back the old behaviour, I think that's enough.

The "Open File" toggle determines the second window.open - leaving this off is the same as old behaviour 👍

@ptim
Copy link
Contributor Author

ptim commented Jun 24, 2022

PS: here's a hack / workaround for windows users: microsoft/vscode#95670 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants