kamiyaa / joshuto

ranger-like terminal file manager written in Rust
https://crates.io/crates/joshuto
GNU Lesser General Public License v3.0
3.4k stars 150 forks source link

Absolute file path isn't correctly passed to mpv when using `fork = true`, breaks an mpv plug-in #381

Closed xfzv closed 1 year ago

xfzv commented 1 year ago

TL;DR: https://github.com/po5/memo/issues/11


memo is a mpv plug-in which allows to save watch history to a file and display said watch history in a menu.

When opening a video from another directory than the one joshuto was started in, and with fork = true, the file path written to memo history file is wrong. Indeed, it is the directory joshuto was started in, not the actual video directory. As a result, the video isn't displayed in memo history menu at all.

Steps to reproduce the issue:

  1. With
# ~/.config/joshuto/mimetype.toml

[class]
video_default = [
  { command = "mpv", fork = true, silent = true },
]

and obviously memo installed, open joshuto.

  1. Navigate to another directory containing a video file
  2. Open the video file
  3. Close mpv
  4. Open mpv again and open memo history menu with defined keybinding
  5. The video isn't displayed in the history menu
  6. Open memo file history: the file does appear at the very bottom but the path is wrong (path where joshuto was initially started in instead of actual video path)
  7. Replace fork = true with fork = false in ~/.config/joshuto/mimetype.toml and repeat steps 2-7: the correct path is printed to memo file history (and the video appears in memo history menu, of course).

I cannot reproduce the issue with other terminal file managers such as:

using their respective default settings. I can keep using the file manager after opening the video with mpv (same behavior than with joshuto fork = true) and memo works as expected.

memo maintainer suspects that https://github.com/kamiyaa/joshuto/commit/ea79ae00629ffd3ee8991727bb7122bb7e6c8381 might be the offending commit.

xfzv commented 1 year ago

Also breaks oltodosel/mpv-scripts/total_playtime.lua.

The total playtime of current playlist cannot be calculated (returns 00:00:00 (inf%)) unless joshuto was started from the video directory.

kamiyaa commented 1 year ago

Interesting. I will need to look into this

kamiyaa commented 1 year ago

I added the following to memo.lua script:

function get_full_path()
    local path = mp.get_property("path")
    if path == nil then return end

    -- additions
    print("file_path")
    print(path)
    print("working-directory")
    print(mp.get_property("working-directory", ""))
    print("$PWD")
    print(os.getenv("PWD"))
    print("pwd command output:")
    local t = os.execute("pwd")

When I tested it with a video file via mpv, I got the following output:

[memo] file_path
[memo] abc_video.mp4
[memo] working-directory
[memo] /home/kamiyaa/builds/joshuto
[memo] $PWD
[memo] /home/kamiyaa/builds/joshuto
[memo] pwd command output:
/home/kamiyaa/videos
AO: [alsa] 48000Hz stereo 2ch float
VO: [gpu] 1920x1080 yuv420p

So joshuto is correctly setting the current directory within the process (as pwd command output shows) However, mpv's mp.get_property("working-directory", "") is still returning the incorrect path. Looking into mpv 's code on how it determines its current directory, its becomes clear that they read a PWD environment variable which joshuto does not set. [3] I'm not sure if its unix tradition to set the PWD variable, but I imagine setting this variable before running mpv should resolve it.

Weird that this is the way mpv determines its current directory.

Another solution is to unset the PWD variable and force mpv to use the getcwd() function. [3]

EDIT: it looks like ranger and potentially other file managers set the PWD env var properly

[memo] file_path
[memo] /home/kamiyaa/videos/abc_video.mp4
[memo] working-directory
[memo] /home/kamiyaa/videos
[memo] $PWD
[memo] /home/kamiyaa/videos
[memo] pwd command output:
/home/kamiyaa/videos

References [1] https://github.com/mpv-player/mpv/blob/1608059d64532e0461e3bec09b20a71802dcb2aa/player/command.c#L3939 [2] https://github.com/mpv-player/mpv/blob/3bdf702b1de8b0d0e0fb700b234b0fc69e04a630/player/command.c#L3192 [3] https://github.com/mpv-player/mpv/blob/master/options/path.c#L327

kamiyaa commented 1 year ago

@xfzv Let me know if fd188e962f1e0862a5f83b996fafd7353de58155 fixes it

xfzv commented 1 year ago

Yes, it does.

kamiyaa commented 1 year ago

Closing!