mpvnet-player / mpv.net

🎞 mpv.net is a media player for Windows with a modern GUI.
GNU General Public License v2.0
3.49k stars 162 forks source link

`${path}` doesn't escape `$` signs for PowerShell's "Show current file in File Explorer" command #580

Closed Sneakpeakcss closed 11 months ago

Sneakpeakcss commented 11 months ago

Describe the bug

The command to open the file in explorer:


run powershell -command "explorer.exe '/select,' ( \"${path}\" -replace '/', '\\' )"

breaks the moment there's $ sign in the path. I've tried to replace it , but that didn't work for me. It seems like the moment ${path} is replaced, it already cut everything after $.

To Reproduce

Opening: C:\test\test test\tes.mp4               :heavy_check_mark: C:\test\test test\test123.mp4      :heavy_check_mark: C:\test\test test\tes$ t123.mp4  :heavy_check_mark: (space after $) C:\test\test test\tes$t123.mp4  :x: (opens This PC directory, or a 'matching' file if it exists, in this case C:\test\test test\tes.mp4  )

Steps to reproduce the behavior:

  1. Play a video with a $ sign in the path and press e

Expected behavior $ should probably be escaped (`$) before passing the command to PowerShell.

Additional context Add any other context about the problem here.

  1. mpv.net v7
  2. Windows 22H2
stax76 commented 11 months ago

Do you often have such files?

Sneakpeakcss commented 11 months ago

Not that often, but the files that i work with have predefined names, so from time to time a dollar sign shows up in them. It also causes problems in folder names, so it's not only exclusive to files. I'll check if i can find the mentioned Lua script later, though it would be good to know its name or original url to it.

stax76 commented 11 months ago

The Lua script was in the wiki, but the link was dead, so I removed it from the wiki, the dead link was:

https://github.com/ayghub/open-dir

Sneakpeakcss commented 11 months ago

Seems like it was completely lost, few sites managed to cache the repo, but not the script itself. [edit] Though, looking at the wiki revisions, it was the author of the script who removed it first and then re-added it only 3 weeks ago. I wonder if he made it private and forgot to change it afterwards.

It was simpler to remake it than digging through obscure parts of internet:

openInExplorer.lua

e           script-message-to openInExplorer open-in-explorer  #menu: Tools > Show current file in File Explorer

PowerShell:

local function open_in_explorer()
    local path = mp.get_property("path")
    if path ~= nil then
        local ps_code = [[
            explorer.exe /select,"__path__"
        ]]
        local path = string.gsub(path, "[%(%)%.%+%-%*%?%[%]%^%$%%]", "%%%1")
        local ps_path = string.gsub(path, "([$`])", "`%1")      -- escape dollar/backtick signs for PowerShell
        local ps_code = string.gsub(ps_code, "__path__", ps_path)
        mp.command_native({
            name = "subprocess",
            playback_only = false,
            args = { 'powershell', '-NoProfile', '-Command', ps_code },
        })
    end
end

mp.add_key_binding(nil, "open-in-explorer", open_in_explorer)

However, at this point it's probably better to stop using PowerShell for this:

direct:

local function open_in_explorer()
    local path = mp.get_property("path")
    if path ~= nil then
        mp.command_native({
            name = "subprocess",
            playback_only = false,
            args = { 'explorer', '/select,',  path },
        })
    end
end

mp.add_key_binding(nil, "open-in-explorer", open_in_explorer)

Both manage to open ( ) % . + - [ ^ $ ' ` .mp4 and tt(tt)tt%tt.tt+tt-tt[tt^tt$tt'tt`tt .mp4 without any issues, though the second one is slightly faster:

test so i'm not sure if anyone would need the PowerShell version at that point.


Seems like this issue might be closed then, unless you want to keep it open for one reason or another.

stax76 commented 11 months ago

PowerShell is great, but starts very slow, compared to cmd, bash, nushell, Lua, Python etc.

I removed the PowerShell one-liner from the defaults because of the $ character limitation/bug and because there aren't any Lua scripts included with mpv.net. It should be fine having only a minimal default feature set, people who need extra features can install a script. Including Lua scripts is a possibility for the future, but there are more important tasks, like internationalization.

With some modification, I added the code to my private.lua script.

-- e  script-binding  fooscript/open-in-explorer
mp.add_key_binding(nil, "open-in-explorer", function ()
    local path = mp.get_property("path")
    if path == nil or string.find(path, "://") then return end
    path = string.gsub(path, "/", "\\")
    mp.command_native({
        name = "subprocess",
        playback_only = false,
        args = { 'explorer', '/select,',  path },
    })
end)

If you don't have a mpv-scripts repo, you can create a GitHub Gist. And add it to the mpv wiki. I think it's very useful, I use it frequently for music files. Thanks for working on a solution.

Sneakpeakcss commented 11 months ago

That’s a nifty way to include the keybind. I was so focused on the main $ issue that i didn’t think much about the scripts structure, or even why bother to do it in PS at all, until it was already done.

However, even at the beginning i noticed and thought about the forward slash replacement, but isn't this technically redundant, since this script is exclusive to Windows?

I remember googling a bit and finding some general references to "network paths", but i don't use any of that, so i wasn't able to check how mpv handles them. That one made me drop my initial idea of using path:match("^%a:\\") just to check for volume letter and leave it at that.

Are there any situations on Windows where mpv provides path with / ? Because if so, i would rather fix my local version before even trying to add it to wiki (i already managed to shoot myself in the foot for not managing protocol paths):

mp.add_key_binding(nil, "open-in-explorer", function()
    local path = mp.get_property("path")
    if path ~= nil and not path:match("^%a[%a%d-_]+://") then
        mp.command_native({
            name = "subprocess",
            playback_only = false,
            args = { 'explorer', '/select,',  path },
        })
    else
        mp.osd_message("Invalid path: " .. (path or "No path provided"))
    end
end)

Bonus feedback message to avoid mashing the key 20 times in a row, just to be sure…

  When it comes to playing video files, i barely managed to properly transition from mpc-hc to mpv, and i didn't drop that idea completely mostly because of mpv.net and uosc ( not to mention that Lua doesn’t require compiling, so it was stupidly easy for someone who doesn’t program at all to butcher other people’s code when required), so at this point it's hard for me to even imagine transitioning from foobar2000 with JScript Panel 3/ColumnsUI for music files. Mpv seems very "resistant" when it comes to things like proper playlist managment.

stax76 commented 11 months ago

However, even at the beginning i noticed and thought about the forward slash replacement, but isn't this technically redundant, since this script is exclusive to Windows?

In this case particular case, backslashes are necessary, you can test it with the following PS commands:

explorer '/select,' C:\Windows/explorer.exe # this does not work

explorer '/select,' C:\Windows\explorer.exe # this works

It was mentioned in a discussion I remembered, probably here.

Either way, normalizing is probably better, it's unclear if Explorer and Win32 stores the path normalized, probably it does, but if it doesn't, it could cause issues, for instance with not commonly used explorer features or shell extensions.

Are there any situations on Windows where mpv provides path with / ?

Yes, mpv doesn't normalize in many cases. Maybe add a comment, 'Explorer requires normalization in this case', so it's clear for everybody.

For music, I used MediaMonkey and MusicBee before migrating to mpv/mpv.net. There are music playlists on YouTube with all the music I liked in my lifetime, almost 1000 songs.

https://www.youtube.com/@stax76/playlists

Sneakpeakcss commented 11 months ago

For music, I used MediaMonkey and MusicBee before migrating to mpv/mpv.net. There are music playlists on YouTube with all the music I liked in my lifetime, almost 1000 songs.

https://www.youtube.com/@stax76/playlists

Nice, some of them reminded me of Emancipator or The Glitch Mob when it comes to chill/electronic music.

In this case particular case, backslashes are necessary, you can test it with the following PS commands:

explorer '/select,' C:\Windows/explorer.exe # this does not work

explorer '/select,' C:\Windows\explorer.exe # this works

Yea, that was the main reason i started thinking about this. I wasn't able find examples to test it and see if mpv doesn't just replace them automatically when providing path on windows, and now i found this, so apparently this: file://C:/test/test.mp4 is a valid path in mpv.

image

That's a good enough example that proves the point, so there’s no reason not to include it:

mp.add_key_binding(nil, "open-in-explorer", function()
    local path = mp.get_property("path")
    if path ~= nil and not path:match("^%a[%a%d-_]+://") then
        path = string.gsub(path, "/", "\\")
        mp.command_native({
            name = "subprocess",
            playback_only = false,
            args = { 'explorer', '/select,',  path },
        })
    else
        mp.osd_message("Invalid path: " .. (path or "No path provided"))
    end
end)

Even without this, at least it didn't just error out like it happened when i tried using it on youtube link before handling those cases, so that's nice.

Sneakpeakcss commented 11 months ago

This didn't take long... new-old issues coming back in force, but this time even stronger. $ isn't an issue anymore, but apparently a comma in a path that doesn't contain any spaces also had problems from the beginning:

C:\test\test2\aa,aa.mp4 this will open "This PC" directory C:\test\test2\,aaaa.mp4 this will open "test" directory and select test2 folder C:\test\test2\..,aaaa.mp4 uh huh… Amazing way of navigating directories

Everything starts working fine the moment there's a space anywhere in the path.

I don't know how mpv executes that command when the subprocess is "direct" like in the script above, but i can only assume it's similar if not the same as in cmd version, so if we believe ProcessExplorer this is what's being passed from mpv:

PowerShell: args = { 'powershell', '-NoProfile', '-Command', ps_code },

"powershell" -NoProfile -Command "  explorer.exe /select,\"C:\test\test2\,aaaa.mp4\"  "  #❌ opens 'test' directory and selects 'test2' folder
"powershell" -NoProfile -Command "  explorer.exe /select,\"C:\test\test2\aa,aa.mp4\"  "  #❌ opens 'This PC' directory
"powershell" -NoProfile -Command "  explorer.exe /select,\"C:\test\test2\aa ,aa.mp4\"  " #✔️ space in path makes it work

cmd: args = { 'cmd', '/c', 'explorer', '/select,', path },

"cmd" /c explorer /select, C:\test\test2\,aaaa.mp4    ❌ opens 'test' directory and selects 'test2' folder
"cmd" /c explorer /select, C:\test\test2\aa,aa.mp4    ❌ opens 'This PC' directory
"cmd" /c explorer /select, "C:\test\test2\aa ,aa.mp4" ✔️ space in path makes it work

This works in all cases when done manually from command prompt:

explorer.exe /select,"C:\test\test2\aa,aa.mp4"  ✔️ works in every single case

Well, that's promising. Since mpv doesn't pass any quotation marks in the problematic cases lets just do it in the script and see what mpv has to say about it: args = { 'cmd', '/c', 'explorer', '/select,', '"' .. path .. '"' },

"cmd" /c explorer /select, "\"C:\test\test2\aa,aa.mp4\""

Amazing… It doesn't work.  

PowerShell version is even more mystical when it comes to getting explorer.exe /select, to work, even in the window manually:

Start-Process -FilePath explorer.exe -ArgumentList "/select,`"C:\test\test2\aa,aa.mp4`""  #✔️ works in every single case

This is the only way i managed to get it to work, and that's from the PS window directly…

This seems to work:

mp.add_key_binding(nil, "open-in-explorer", function()
    local path = mp.get_property("path")
    if path ~= nil and not path:match("^%a[%a%d-_]+://") then
        local ps_code = [[
            Start-Process -FilePath explorer.exe -ArgumentList "/select,`"__path__`""
        ]]
        local path = string.gsub(path, "/", "\\")
        local path = string.gsub(path, "[%(%)%.%+%-%*%?%[%]%^%$%%]", "%%%1")
        local ps_path = string.gsub(path, "([$`])", "`%1")      -- escape dollar/backtick signs for PowerShell
        local ps_code = string.gsub(ps_code, "__path__", ps_path)
        mp.command_native({
            name = "subprocess",
            playback_only = false,
            args = { 'powershell', '-NoProfile', '-Command', ps_code },
        })
    else
        mp.osd_message("Invalid path: " .. (path or "No path provided"))
    end
end)

beatingRecords

We're breaking records… At this point this seems like a case of "just Windows / mpv things" and pick your poison. It would be far more preferable to get mpv to pass it with a single set of double quotes through cmd in all cases, but i don't think it's possible.

stax76 commented 11 months ago

It's awesome that you could find a working solution, normally I can make my command lines work, but here I probably would have failed.

The main issue maybe is the crappy Explorer CLI.

For command line debugging, I sometimes use this tool: https://github.com/stax76/show-args

Another one that can be useful: https://github.com/stax76/run-hidden

A utility I used and updated today: https://github.com/stax76/command-line-to-exe

Sneakpeakcss commented 11 months ago

For command line debugging, I sometimes use this tool: https://github.com/stax76/show-args

I found exactly the same repo, but of course only after i finished fighting with ProcessExplorer that wasn't always showing the launched cmd.exe/powershell.exe processes to grab the passed commands. I really hope that i won't have to ever use it, because at this point i'm having "proper character escaping while passing argument through different application" flashbacks.

Another one that can be useful: https://github.com/stax76/run-hidden

Well, doesn't that look familiar... I was there in the before times, while suffering like everyone else trying to hide the window. That's probably the starting point for my mentioned flashbacks when i discovered Flow Launcher and was trying to pass arguments to .bat/.ps1 scripts in Plugin Runner without any window flashing. That one and your Favorites are my most often used plugins.


This is the moment where i say that i feel like a fool:

Everything starts working fine the moment there's a space anywhere in the path.

args = { 'cmd', '/c', 'explorer', '/select,', path .. ' ' },

"cmd" /c explorer /select, "C:\test\test2\,aaaa.mp4 "   ✔️
"cmd" /c explorer /select, "C:\test\test2\aa,aa.mp4 "   ✔️
"cmd" /c explorer /select, "C:\test\test2\..,aaaa.mp4 " ✔️

 

-- open-in-explorer.lua

mp.add_key_binding(nil, "open-in-explorer", function()
    local path = mp.get_property("path")
    if path ~= nil and not path:match("^%a[%a%d-_]+://") then
        path = string.gsub(path, "/", "\\")
        mp.command_native({
            name = "subprocess",
            playback_only = false,
            args = { 'explorer', '/select,',  path .. ' ' },
        })
    else
        mp.osd_message("Invalid path: " .. (path or "No path provided"))
    end
end)

We're spared the pains of PowerShell again. I can't wait for another edge case that will make everything explode.