-
Notifications
You must be signed in to change notification settings - Fork 253
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
fix(open): linux error spawning new obsidian process #346
Conversation
I can also only make this change for Linux, if you'd prefer. if this_os == util.OSType.Linux then
vim.fn.jobstart(cmd .. " " .. vim.fn.shellescape(table.concat(args, " ")), { detach = true })
else
run_job(cmd, args)
end |
@benelan I wonder if it has to do with running in a shell env vs no shell. I think the plenary |
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.
Hey @benelan, thanks for this!
lua/obsidian/commands/open.lua
Outdated
@@ -81,5 +80,6 @@ return function(client, data) | |||
|
|||
assert(cmd) | |||
assert(args) | |||
run_job(cmd, args) | |||
|
|||
vim.fn.jobstart(cmd .. " " .. vim.fn.shellescape(table.concat(args, " ")), { detach = true }) |
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.
Apparently nvim recommends using vim.system()
from Lua instead of vim.fn.jobstart()
(see :help jobstart
). I'm not entirely sure why but I guess I'd rather follow that recommendation then not.
Also would be good to report errors like run_job()
will.
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.
Yeah I checked and unfortunately vim.system()
isn't available in v0.9
, so I used vim.fn.jobstart()
for backwards compatibility. Should I throw a conditional in there to check the nvim version and use vim.system()
if it is available?
I'll add in some error reporting either way.
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.
Huh, okay in that case let's stick with vim.fn.jobstart
. It looks like you can pass a callback there to capture stderr.
Hmm yeah that's possible. I'll experiment with passing a list to jobstart to see if that breaks it or not. |
Alright, I added the logging! Opening obsidian with $ xdg-open "obsidian://open?vault=notes&file=monorepos.md" 2>&1 >/dev/null
[104469:0126/132841.090072:ERROR:node_bindings.cc(300)] Most NODE_OPTIONs are not supported in packaged apps. See documentation for more details.
(obsidian:104469): Gtk-WARNING **: 13:28:41.206: Theme parsing error: gtk.css:6691:68: Invalid name of pseudo-class
[104469:0126/132841.419062:ERROR:nss_util.cc(357)] After loading Root Certs, loaded==false: NSS error code: -8018 I used on_exit instead and log if the code isn't 0. I also tested passing a list to jobstart and it still worked. Here is the vim command for reference: :exe jobstart(["xdg-open", "obsidian://open?vault=notes&file=monorepos.md"], {"detach": v:true}) |
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.
Awesome! Looks good. Just one more minor request and then we can get this merged!
Done, thanks for the review! |
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.
LGTM!
The
ObsidianOpen
command failed to open the Obsidian app on Linux if it wasn't already running. After some troubleshooting I determined it is something related to the async module.Nothing jumped out at me in the plenary job initialization code, but using
vim.fn.jobstart()
,vim.system()
, andvim.uv.spawn()
all work. I went with jobstart for backwards compatibility. Let me know if I need to make any other changes, or feel free to adopt this patch however you see fit. Thanks for the great plugin!Fix #319