- 
                Notifications
    You must be signed in to change notification settings 
- Fork 3.2k
clipboard-x11: add x11 clipboard backend #16965
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
Conversation
| codespell complains about "requestor", could you add it to allowed words? | 
2a41cee    to
    658f862      
    Compare
  
    658f862    to
    808732e      
    Compare
  
    This adds the x11 clipboard backend and native clipboard support to x11. Clipboard monitoring is supported.
808732e    to
    94ff415      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
On Wayland envitonments with Xwayland available but without ext-data-control-v1, mpv will fall back to the X11 backend instead of VO backend. This may result in clipboard not working properly between mpv and native Wayland clients. Fix this by disabling X11 backend on Wayland envitonments by default and adding the --clipboard-xwayland option for manual override.
| Is xfixes really such an uncommon dependency that it even warrants its own build switch? Looking at its consumers on my system, it seems unlikely anyone would have a modern X11 setup without something already having pulled in xfixes as a dep i.e. GTK, KDE Plasma, SDL, PipeWire, Firefox or Mesa when their respective X11 support is enabled. And I'm not sure, how viable it would even be to deploy "pure" Xorg without one of its own many constituting libraries and extensions needing it anyway. I bring this up because, if x11 is explicitly disabled but x11-clipboard isn't being set, the default automagic behavior kicks in, which might then trigger a link time failure, if the common xfixes dep was found. This just happened to me on the account of distro packaging not yet being aware of having to tie the new Meson flag with the existing X11 disablement. | 
| The library needs to be linked I presume but the detection can just also require  | 
| 
 The reason is the the current "x11" feature is X11 support for VOs. X11 clipboard does not depend on that, so it has its own feature flag. 
 Having libraries installed does not mean that development headers are installed, which is the case on debian and derivatives. Adding xfixes to the "x11" feature dependency can cause builds having X11 support disabled if xfixes is not found. | 
| Except it actually failed to build on my system, where -Dx11=disabled is passed by the packaging tool doing the build. If I'm reading the build log right, clipboard-x11.c requires a bunch of X11 symbols to link the resulting object, which presumably require x11 to be enabled? Or is the issue actually subtle underlinking on the assumption that other parts of MPV will have those symbols anyway? Here's the symbols that were reported as missing when trying to link the object file: XConvertSelection, XCloseDisplay, XFree, XInternAtom, XChangeProperty, XCreateWindow, XNextEvent, XDestroyWindow, XGetWindowProperty, XFlush, XOpenDisplay, XSetSelectionOwner, XPending, XSendEvent, XQueryExtension As a side note, while user facing failures are not nice, I'd suggest they're fundamentally due to the use of automagic dependencies, which result in unexpected build configurations that aren't actually valid, so either the build system gets the deps right and aborts early telling the one building to get their duck in a row or it will blow up during the build phase anyway but now with errors generated by the toolchain. Meaning it will fail either way just at different points and with different degrees of clarity. Or, as you worry, it might actually build but be missing important bits, however that's the tradeoff with using automagic deps - sometimes that magic explodes in user's face. Which is why distros tend to explicitly set deps to either enabled or disabled to at least have predictable build explosions over surprising binaries. Therefore, if you're concerned about that, then add also a header check in addition to library presence check. This is fairly routine issue that plenty of projects deal with and Meson does have  | 
This adds the x11 clipboard backend and native clipboard support to x11.
Clipboard monitoring is supported.