Ticket #4133 (closed enhancement: fixed)
[PATCH] Allow running clipboard commands if DISPLAY is not set
Reported by: | devzero | Owned by: | andrew_b |
---|---|---|---|
Priority: | minor | Milestone: | 4.8.26 |
Component: | mc-core | Version: | master |
Keywords: | Cc: | ||
Blocked By: | Blocking: | ||
Branch state: | merged | Votes for changeset: | committed-master |
Description
Hello!
I ran into a restriction related to the options clipboard_store and clipboard_paste.
I'm currently working on a script that brings a transparent clipboard support to any application that supports executing arbitrary commands on the copy and paste actions in the same way as mc does. It also works for applications running on a virtual terminal and falls back to the file-based clipboard storage if an X server is not running or cannot be detected.
Here is the script:
https://github.com/sde-gui/sde-common-clipboard/blob/master/sde-common-clipboard
I noticed that mc refuses running clipboard_store and clipboard_paste commands if the DISPLAY environment variable is not set. This limitation didn't seem to be explicitly stated in the documentation (especially in the russian manual page -- it lacks any mention of X11 regarding clipboard at all), so I had to check the source code out to make sure that was the case.
My suggestion is to bring a new configuration variable controlling this behavior. The attached patch implements a new option clipboard_force_no_display allowing to override the default behavior.
Attachments
Change History
Changed 4 years ago by devzero
- Attachment 0001-Allow-running-clipboard-commands-if-DISPLAY-is-not-s.patch added
comment:1 follow-up: ↓ 3 Changed 4 years ago by andrew_b
What about get rid of $DISPLAY usage here at all instead of adding yet another micro-option?
comment:2 Changed 4 years ago by ossi
the concerns are:
- backwards compatibility of the configurations. that's somewhat minor, as an affected user would somewhat easily spot the problem.
- making the common case more complicated. this could be alleviated by mc shipping a wrapper for xclip itself.
- given that this would be a hidden option that configures two other hidden options, it isn't all that bad to start with. arguably, the usability problem is that this needs such low-level configuration to start with. otoh, the reporter would be certainly unhappy if it wasn't that way ...
comment:3 in reply to: ↑ 1 ; follow-up: ↓ 4 Changed 4 years ago by devzero
Replying to andrew_b:
What about get rid of $DISPLAY usage here at all instead of adding yet another micro-option?
clipboard_file_from_ext_clip() should be reworked in this case.
Running with:
clipboard_store=xclip -i clipboard_paste=xclip -o
with DISPLAY unset, or just with any failing command:
clipboard_store=false clipboard_paste=false
breaks the mc clipboard.
If this bug is fixed and there are no other hidden reasons for checking DISPLAY, it seems better to throw the check away.
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 8 Changed 4 years ago by andrew_b
Replying to devzero:
Running with:
[...]
with DISPLAY unset
[...]
breaks the mc clipboard.
Correct. Expected that DISPLAY is set appropriately.
If this bug is fixed
Why this is a bug?
Look at the vim. It is also requires the DISPLAY variable:
DISPLAY environment variable on Linux systems
Make sure your DISPLAY environment variable is set appropriately - otherwise vim can not connect to your x-session to access the clipboard.
comment:5 Changed 4 years ago by ossi
ehm, i kind of expect mc's clipboard to work on a plain console.
Look at the vim. It is also requires the DISPLAY variable:
yes - when you explicitly ask for the x11 clipboard.
comment:6 Changed 4 years ago by angel_il
mc's clipboard is work with global clipboard (X-clipboard) throuth 'xclip' if DISPLAY is set, and local (editor, fields, command line) throuth mc-clip-file.
comment:8 in reply to: ↑ 4 Changed 4 years ago by devzero
Replying to andrew_b:
Why this is a bug?
Sorry for the misinformation. Looks like something was wrong in my configuration the last night, when I detected the "bug". I double checked it now.
You are right, the check for DISPLAY seems unneeded and the deletion of the check seems to have no visible effect on the application functioning.
comment:9 Changed 4 years ago by andrew_b
- Owner set to andrew_b
- Status changed from new to accepted
- Branch state changed from no branch to on review
- Milestone changed from Future Releases to 4.8.26
Branch: 4133_clipboard_no_display
changeset:cf29517d41ee11a0dd4b5142b8a1b8c508f5b5c4
comment:10 Changed 4 years ago by andrew_b
- Votes for changeset set to andrew_b
- Branch state changed from on review to approved
comment:11 Changed 4 years ago by andrew_b
- Status changed from accepted to testing
- Votes for changeset changed from andrew_b to committed-master
- Resolution set to fixed
- Branch state changed from approved to merged
Merged to master: [2225af84f5579ed2c904005e63d61ff743d82041].