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.

Sign up for GitHub

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

Jump to bottom

Unreliable with non-standard shell #18

Closed
bugabinga opened this issue Feb 27, 2024 · 5 comments · May be fixed by #19
Closed

Unreliable with non-standard shell #18

bugabinga opened this issue Feb 27, 2024 · 5 comments · May be fixed by #19

Comments

@bugabinga
Copy link

I like to use a non-standard shell for vim.opt.shell (Nushell). It happens to work on Linux, but does not on Windows (Nushell is cross-platform).

I think the issue is that query_command in init.lua is a shell script, that assumes the default shells per platform.

I would like to refactor the code to:

  1. change query_command to a table, that always represents one command + arguments (no shell features allowed, e.g. pipes)
  2. change vim.fn.jobstart to vim.system, which is not dependent on the shell option in Neovim.

Thoughts?
@f-person
Copy link
Owner

Hi! The refactoring of query_command indeed seems nice and makes sense, but

change vim.fn.jobstart to vim.system

I'm not sure about this one, since the plugin relies on vim.fn.jobstart and I don't know how exactly vim.system works, but this is also open to discussion :)
I'll try to check out the vim.system API and come back with a reply on that ;)

@nekowinston
Copy link
Collaborator

nekowinston commented Feb 27, 2024 •
edited
Loading

My only concern is that vim.system is very recent (0.10, nightly exclusive at the time of writing). An effort to make query_command shell agnostic sounds like a good idea.

@bugabinga
Copy link
Author

I noticed, that the help file for jobstart states:

		If {cmd} is a List it runs directly (no 'shell').

That means, changing query_command to a table and adapting the parsing of the query result could be enough, right?

@nekowinston
Copy link
Collaborator

Yes. The only shell pipe we're using is in WSL

auto-dark-mode.nvim/lua/auto-dark-mode/init.lua

Line 100 in 76e8d40

'reg.exe Query "HKCU\\SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Themes\\Personalize" /v AppsUseLightTheme | findstr.exe "AppsUseLightTheme"'

which I think should be able to be moved into lua native code in

auto-dark-mode.nvim/lua/auto-dark-mode/init.lua

Lines 24 to 39 in 76e8d40

local function parse_query_response(res)
if system == "Linux" then
-- https://github.com/flatpak/xdg-desktop-portal/blob/c0f0eb103effdcf3701a1bf53f12fe953fbf0b75/data/org.freedesktop.impl.portal.Settings.xml#L32-L46
-- 0: no preference
-- 1: dark
-- 2: light
return string.match(res, "uint32 1") ~= nil
elseif system == "Darwin" then
return res == "Dark"
elseif system == "Windows_NT" or system == "WSL" then
-- AppsUseLightTheme REG_DWORD 0x0 : dark
-- AppsUseLightTheme REG_DWORD 0x1 : light
return string.match(res, "1") == nil
end
return false
end

bugabinga pushed a commit to bugabinga/auto-dark-mode.nvim that referenced this issue Feb 27, 2024
fix f-person#18 by making query_command shell-agnostic
22da14d 
`query_command` is now of type `table` so that `vim.fn.jobstart` does
not use `shell`.
@bugabinga bugabinga mentioned this issue Feb 27, 2024
Open
@bugabinga bugabinga mentioned this issue Mar 6, 2024
Closed
@nekowinston
Copy link
Collaborator

nekowinston commented Mar 7, 2024 •
edited
Loading

Sorry, I got held up on further reviewing the PR, I'll try to have another look on the weekend. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@bugabinga @f-person @nekowinston