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

player/dialog/file_dialog: add file dialog for macOS, Windows and Linux #15845

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

kasper93
Copy link
Contributor

@kasper93 kasper93 commented Feb 10, 2025

  • - figure out multi file selection or disable it
  • - working macOS impl
  • - connect more commands (?)
  • - add Linux impl

@kasper93
Copy link
Contributor Author

@Akemi: Could you help me with macOS implementation? I tried to put something together to make sure the api will work nicely, but it is missing filters and probably doesn't work at all currently. Also I made objc, but I don't mind replacing it completely with swift impl or anything.

@kasper93
Copy link
Contributor Author

Currently connected only to loadfile / loaddir. Can be used for more open/save commands like screenshots and so on. Also can be exported as standalone command for script use, but not sure how useful that would be.

@kasper93 kasper93 force-pushed the file-dialog branch 2 times, most recently from 9f42128 to 38844b3 Compare February 10, 2025 09:39
@Akemi
Copy link
Member

Akemi commented Feb 10, 2025

sure, i will have a look. give me a few days to think about the implementation details for macOS.

from the top of my head, i would probably move the (open) dialog functionality (https://github.com/mpv-player/mpv/blob/master/osdep/mac/menu_bar.swift#L325-L361) out of the menu bar into its own class and make it reusable. also extending its functionality to be usable with your changes.

if you don't mind, i would prepare something you can then just (re-)use here.

@na-na-hi
Copy link
Contributor

I think it's better to add a separate command instead of making loadfile accepting empty url, because the semantics of the loadfile command means that the command cannot return until it knows which items are added, and this syncronous process blocks the core.

A separate command which opens the file dialog in a separate thread, and signals the core after the file is selected would be a better idea. This can reuse the the mp_event_drop_files function used by drag and drop.

@kasper93
Copy link
Contributor Author

I think it's better to add a separate command instead of making loadfile accepting empty url, because the semantics of the loadfile command means that the command cannot return until it knows which items are added, and this syncronous process blocks the core.

Could you explain the issue, I don't understand. It works perfectly fine.

A separate command which opens the file dialog in a separate thread, and signals the core

Why? It already spawns a thread, unlocks core and wait for user selection. Also you can use asynchronous command if you don't want to wait for loadfile selection on calling thread.

@kasper93 kasper93 changed the title osdep/file_dialog: add file dialog for macOS and Windows osdep/file_dialog: add file dialog for macOS, Windows and Linux Feb 10, 2025
@kasper93
Copy link
Contributor Author

Added kdialog and zenity support. More can be added later, I think it's enough for now.

@kasper93 kasper93 force-pushed the file-dialog branch 4 times, most recently from 84e3947 to 589654d Compare February 10, 2025 23:39
Copy link

github-actions bot commented Feb 11, 2025

Download the artifacts for this pull request:

Windows
macOS

@hooke007
Copy link
Contributor

@dyphire
Copy link
Contributor

dyphire commented Feb 11, 2025

I think the support for file dialogs should be added to the audio-add and sub-add commands also.

@dyphire
Copy link
Contributor

dyphire commented Feb 11, 2025

Also can be exported as standalone command for script use, but not sure how useful that would be.

This will be useful for third-party scripts and greatly increases scalability

@na-na-hi
Copy link
Contributor

Could you explain the issue, I don't understand. It works perfectly fine.

Sorry, I didn't notice spawn_thread. However, you avoided blocking by unlocking the core which can result in data race. The only other command that does this is subprocess, and that's fine because mp_subprocess does not access mpctx. But here the thread accesses mpctx and filter options which are also accessed by the core.

So to make this viable, mp_file_dialog_get_files shouldn't take mpctx, and all filter strings need to be duplicated, before unlocking the core.

@kasper93
Copy link
Contributor Author

kasper93 commented Feb 11, 2025

Could you explain the issue, I don't understand. It works perfectly fine.

Sorry, I didn't notice spawn_thread. However, you avoided blocking by unlocking the core which can result in data race. The only other command that does this is subprocess, and that's fine because mp_subprocess does not access mpctx. But here the thread accesses mpctx and filter options which are also accessed by the core.

So to make this viable, mp_file_dialog_get_files shouldn't take mpctx, and all filter strings need to be duplicated, before unlocking the core.

Not having synchronized access to mpctx doesn't mean that we cannot use parts of it that are thread-safe, like for example mpctx->log that would be always valid. I could even move the locking inside the impl. Regardless, I don't needed it and this is kinda leftover from initial version. So no need to complicate things.

The only other command that does this is subprocess, and that's fine because mp_subprocess does not access mpctx.

The behavior of spawn_thread is well documented, no need to base it on examples. But fyi all commands that spawn thread, are unlocking core one way or another. For example track manipulation do that in mp_add_external_file.

@kasper93 kasper93 force-pushed the file-dialog branch 3 times, most recently from 2a9bbc3 to a4c405e Compare February 11, 2025 07:23
@kasper93 kasper93 marked this pull request as ready for review February 11, 2025 07:24
@kasper93
Copy link
Contributor Author

kasper93 commented Feb 11, 2025

Also can be exported as standalone command for script use, but not sure how useful that would be.

This will be useful for third-party scripts and greatly increases scalability

Added file-dialog command. Need to add docs and filtering for it still. And whole thing needs another pass of cleaning.

@kasper93 kasper93 force-pushed the file-dialog branch 4 times, most recently from 0438edd to 64b6db0 Compare February 11, 2025 18:38
@kasper93
Copy link
Contributor Author

kasper93 commented Feb 20, 2025

@na-na-hi: Would command like that meet your quality standards for low-level command?

    { "load", cmd_load,
        {
            {"flags", OPT_FLAGS(v.i,

                {"file", 1 << 0}, // default
                {"list", 1 << 1},
                {"track", 1 << 2},
                {"dir", 1 << 3},

                {"append", 1 << 4}, // default
                {"insert", 1 << 5},

                {"end", 1 << 6}, // default
                {"at", 1 << 7},
                {"next", 1 << 8},

                {"play", 1 << 9},

                {"select", 1 << 10},
                {"auto", 1 << 11},
                {"cached", 1 << 12},
                 
                // note those are both useful for track type selection as well as dialog filtering (by accident)
                {"subtitle", 1 << 13},
                {"audio", 1 << 14},
                {"video", 1 << 15},
                {"image", 1 << 16},

                {"dlg", 1 << 17}),

                .flags = MP_CMD_OPT_ARG},

            // could be vararg, but not sure if we prefer types
            {"arg0", OPT_PATHLIST(v.str_list), .flags = MP_CMD_OPT_ARG},
            {"arg1", OPT_INT(v.i), .flags = MP_CMD_OPT_ARG},
            {"arg2", OPT_STRING(v.s), .flags = MP_CMD_OPT_ARG},
            {"arg3", OPT_STRING(v.s), .flags = MP_CMD_OPT_ARG},
            {"arg4", OPT_KEYVALUELIST(v.str_list), .flags = MP_CMD_OPT_ARG},
        },

Than you can add scripts and helpers for each high level use-cases.

As for usage

load dlg would show generic dialog
load file+dlg would show file dialog
and so on.
load file "<file>" would load file
load file+insert+at <file> 55 would insert file at 55 index.
load file+append+play <file> would append play
load track+select+dlg would load track selection dialog.
load track+video+select+dlg would load video track selection dialog.
load track+subtitle+auto <file> 0 "track title" "eng" would load subtitle track with parameters

and finally we can then have keybinds

ctrl+o load video+image+audio+playlist+dlg
ctrl+d load dir+dlg

and in select

        {"Open file(s)", "load file+dlg+replace+play", true},
        {"Open directory", "load dir+dlg", true},
        {"Load subtitles", "load subtitle+dlg", true},

dlg flag is totally optional, but I added it, because you seems to be very reluctant towards optional arguments.

@Dudemanguy
Copy link
Member

Dudemanguy commented Feb 20, 2025

Added libsystemd implementation if someone prefers that. (didn't try to abstract it)

I don't like Glib/GIO and I imagine there's others here that don't like it either.

Sorry to not comment on or seriously think about this until after the code has been written considering what I'm about to type but I don't think we need this. xdg-desktop-portal links to libgio so any system that has a portal implementation should be guaranteed to have it. I can't seriously think there is going to be a large amount of people out there that would really disable the gio backend and enable the libsystemd one. And I don't understand what the practical advantage would even be. They both just call portal stuff anyway. Maybe one library is better/worse but that's not really relevant to user. Ideally it would use libportal but as I understand kasper said the API was too limited, so we dropped that idea. Maybe it can be revisited later if that ever gets better.

In terms of availability, libgio is way wider as well. Non-systemd distros, BSDs and apparently even Windows (lol). Unless there is some actual advantage to using sdbus to communicate to portals instead of just gio, I don't think we should have it. It seems like borderline dead code.

@kasper93
Copy link
Contributor Author

I fully agree on that. It's one commit, which can be skipped.

@kasper93
Copy link
Contributor Author

Changes:

  • Removed libsystemd implementation.
  • Added interactive command prefix

"load", cmd_load,

For command load please see #15927

@@ -6061,7 +6061,7 @@ static void cmd_loadfile(void *p)

void *tmp = talloc_new(NULL);

if (!files || !files[0] || !files[0][0]) {
if ((!files || !files[0] || !files[0][0]) && mp_input_is_interactive_cmd(cmd->cmd)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the interactive prefix idea but unfortunately this doesn't quite emulate the old behavior. loadfile will instantly exit mpv since the url is empty but it's optional so it's still not a hard error. How about just dropping the empty url part of the implementation and just letting whatever is selected by the dialog (or not selected) override anything that is passed in the url part of the command?

Copy link
Contributor Author

@kasper93 kasper93 Feb 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the report. This is fixed now, there was a typo in the check below.

Also interactive prefix means that iteration may happen, but not that it will/should happen.

@Dudemanguy
Copy link
Member

Dudemanguy commented Feb 22, 2025

As for the set-property argument of the file-dialog property, I believe #15921 is most of the way there. At least for the use case of "pass filename as an argument to another command". The only question is what is the right way to handle node arrays. I was thinking that probably the pipe logic itself should handle it and if there is only 1 string in the array change it to just a string type. If there are multiple ones, use a string list.

kasper93 and others added 18 commits February 22, 2025 02:48
If URL is not provided file dialog will be open on supported platforms
with selection for files (loadfile) or directories (loaddir).
For now it is just WID and works only on Windows implementation. For X11
and Wayland we would need to produce proper window identifier which is
currently not supported by mpv.

See: https://github.com/flatpak/xdg-desktop-portal/blob/71649cd9876072ed5c12a07928784b6c463a79eb/doc/window-identifiers.rst
Allows to select file dialog providers on platforms where there is
no native implementation and we have to do all sort of weird things to
make it work.
It grew too big, and some platforms can have multiple implementations,
so it only makes sense to organize it accordingly.

Initially the idea was to have it as a osdep implementation, single for
each platform, neat and simple, but we are past that point.
@mrfragger
Copy link

mrfragger commented Feb 24, 2025

one thing nice to add if possible is in uosc it has

# Default open-file menu directory
default_directory=~/datampv

So in other words when Open file(s) or Open directory in select.lua menu it'll default to a particular default directory rather than just / (root dir) assuming there is no file loaded.

Currently if there already is a file loaded it'll open up the directory which contains the loaded file which is great.

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.