Ticket #4133 (closed enhancement: fixed)

Opened 6 weeks ago

Last modified 3 days ago

[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.

Change History

comment:1 follow-up: ↓ 3 Changed 5 weeks ago by andrew_b

What about get rid of $DISPLAY usage here at all instead of adding yet another micro-option?

comment:2 Changed 5 weeks 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 5 weeks 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 5 weeks 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 5 weeks 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 5 weeks 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:7 Changed 5 weeks ago by devzero

(Deleted. Looks like My tests were incorrect. Sorry.)

Last edited 5 weeks ago by devzero (previous) (diff)

comment:8 in reply to: ↑ 4 Changed 5 weeks 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 weeks 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 3 days ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:11 Changed 3 days 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

comment:12 Changed 3 days ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.