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

Add clipboard support #13837

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions DOCS/man/input.rst
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,12 @@ Remember to quote string arguments in input.conf (see `Flat command syntax`_).
Like all input command parameters, the filename is subject to property
expansion as described in `Property Expansion`_.

``screenshot-to-clipboard [<flags>]``
Copy link
Member

Choose a reason for hiding this comment

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

I definitely see the usefulness but maybe copying images is better left out of the initial implementation to keep it simple first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot support is pretty core to this functionality (it's one of, like, 2 main use-cases I'm targeting), so I'm hesitant to remove it unless we have a specific reason.

Copy link
Member

Choose a reason for hiding this comment

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

My thought was that it might be better to have a fleshed-out API for the "copy text/URL(s) to the clipboard" first, merge that and then integrate image clipboards into that (in a different PR). Perhaps even in a way that can be used from the client API.

Take a screenshot and save it to the system clipboard.

The ``flags`` argument is like the first argument to ``screenshot`` and
supports ``subtitles``, ``video``, ``window``.

``playlist-next <flags>``
Go to the next entry on the playlist.

Expand Down Expand Up @@ -3573,6 +3579,35 @@ Property list
``current-ao``
Current audio output driver (name as used with ``--ao``).

``clipboard`` (RW)
Access to the system clipboard as a key/value map of types and data.

Sub-paths can be accessed directly; e.g. ``clipboard/text`` can be read, written,
or observed.

Writing a string to the root ``clipboard`` property will write to the text clipboard
as a shortcut.

Converting this property to a string will give a JSON representation of all types.
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you plan to represent image data with this? Base64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't expect raw image data to appear in the string representation; it'd only be useful to API clients interacting with binary directly (a la screenshot-raw).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have any other properties that do this? What's the intended use case for this behavor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything that uses m_property_read_sub does this (this doesn't use that to avoid reading every clipboard whenever a single one is requested, though maybe I could add a callback feature to m_sub_property to address that).

Copy link
Member

Choose a reason for hiding this comment

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

The manual doesn't mention this for any other properties so that might be what tripped me up.

Unset types will be null.

Writing to one type may cause other types to also be updated.

Currently, the following types are available:

``text``
Arbitrary plain text

``url``
A URL; if ``path`` is populated, this will be a corresponding file:// URL

``paths``
A list of absolute paths to files on disk.

``path``
A single absolute path to a file on disk.
Exposed for convenience and not included in the top-level map.
Copy link
Member

Choose a reason for hiding this comment

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

So get_property_native("clipboard") will include text, url and paths yet get_property_string("clipboard/path") will work?
It doesn't seem wise to introduce such behaviour into properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could include it; it's just redundant with paths, which would make it a little more annoying to use for development/debugging.


``user-data`` (RW)
This is a recursive key/value map of arbitrary nodes shared between clients for
general use (i.e. scripts, IPC clients, host applications, etc).
Expand Down
7 changes: 6 additions & 1 deletion meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,8 @@ cocoa = dependency('appleframeworks', modules: ['Cocoa', 'IOKit', 'QuartzCore'],
features += {'cocoa': cocoa.found()}
if features['cocoa']
dependencies += cocoa
sources += files('osdep/language-mac.c',
sources += files('osdep/clipboard-mac.m',
'osdep/language-mac.c',
'osdep/path-mac.m',
'osdep/utils-mac.c',
'osdep/mac/app_bridge.m')
Expand Down Expand Up @@ -540,6 +541,10 @@ else
features += {'win32-smtc': false}
endif

if not features['cocoa']
sources += files('osdep/clipboard-dummy.c')
Copy link
Member

Choose a reason for hiding this comment

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

The "one global handler" approach will not work on Linux. Both the X11 and Wayland code needs to be compiled in, but only one of them will be active depending on the VO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd expect to have a single linux implementation of the m_clipboard_* functions that dispatches between X and Wayland as needed.

Copy link
Member

Choose a reason for hiding this comment

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

Requiring only one implementation per platform sounds unnecessarily inflexible to me and as you proposed also leads to weird patterns where you have code that only forwards calls. Plus not being able to handle edge cases like X11 on macOS (just an example, not suggesting we should have that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There can be multiple implementations of the underlying functionality per platform, they just need a implementation of the cross-platform APIs; if e.g. clipboard-linux.c wants to call into specific routines in clipboard-wayland.c and clipboard-x11.c, that's fine.

endif

features += {'glob-posix': cc.has_function('glob', prefix: '#include <glob.h>')}

features += {'glob-win32': win32 and not features['glob-posix']}
Expand Down
35 changes: 35 additions & 0 deletions osdep/clipboard-dummy.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Dummy implementations of clipboard access routines
*
* This file is part of mpv.
*
* mpv is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 2.1 of the License, or (at your option) any later version.
*
* mpv is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public
* License along with mpv. If not, see <http://www.gnu.org/licenses/>.
*/

#include "clipboard.h"

int m_clipboard_set(struct MPContext *ctx, const struct m_clipboard_item* item)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also could we use mp_ prefix like with all other things?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely clear on what the style guide situation is for when we use m_ vs mp_; I see a lot of usage of both within the codebase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to use mp_ like we use for everything recently.

{
return CLIPBOARD_FAILED;
}

int m_clipboard_get(struct MPContext *ctx, struct m_clipboard_item* item)
{
return CLIPBOARD_FAILED;
}

bool m_clipboard_poll(struct MPContext *ctx)
{
return false;
}
Loading
Loading