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

[PATCH] Allow running clipboard commands if DISPLAY is not set #4133

Closed
mc-butler opened this issue Oct 15, 2020 · 13 comments
Closed

[PATCH] Allow running clipboard commands if DISPLAY is not set #4133

mc-butler opened this issue Oct 15, 2020 · 13 comments
Assignees
Labels
area: core Issues not related to a specific subsystem prio: low Minor problem or easily worked around
Milestone

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/4133
Reporter devzero (vadim.ush@….com)

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.

Note

Original attachments:

@mc-butler
Copy link
Author

Changed by devzero (vadim.ush@….com) on Oct 15, 2020 at 17:54 UTC

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Oct 17, 2020 at 11:00 UTC (comment 1)

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

@mc-butler
Copy link
Author

Changed by ossi (@ossilator) on Oct 17, 2020 at 12:47 UTC (comment 2)

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

@mc-butler
Copy link
Author

Changed by devzero (vadim.ush@….com) on Oct 17, 2020 at 16:05 UTC (comment 1.3)

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.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Oct 18, 2020 at 8:51 UTC (comment 3.4)

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.


@mc-butler
Copy link
Author

Changed by ossi (@ossilator) on Oct 18, 2020 at 10:21 UTC (comment 5)

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.

@mc-butler
Copy link
Author

Changed by angel_il (@ilia-maslakov) on Oct 18, 2020 at 10:52 UTC (comment 6)

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.

@mc-butler
Copy link
Author

Changed by devzero (vadim.ush@….com) on Oct 18, 2020 at 16:09 UTC (comment 7)

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

@mc-butler
Copy link
Author

Changed by devzero (vadim.ush@….com) on Oct 18, 2020 at 16:48 UTC (comment 4.8)

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.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Oct 29, 2020 at 14:12 UTC (comment 9)

  • Branch state changed from no branch to on review
  • Owner set to andrew_b
  • Milestone changed from Future Releases to 4.8.26
  • Status changed from new to accepted

Branch: 4133_clipboard_no_display
[cf29517d41ee11a0dd4b5142b8a1b8c508f5b5c4]

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Nov 21, 2020 at 12:00 UTC (comment 10)

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Nov 21, 2020 at 12:01 UTC (comment 11)

  • Status changed from accepted to testing
  • Votes changed from andrew_b to committed-master
  • Branch state changed from approved to merged
  • Resolution set to fixed

Merged to master: [2225af8].

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Nov 21, 2020 at 12:01 UTC (comment 12)

  • Status changed from testing to closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues not related to a specific subsystem prio: low Minor problem or easily worked around
Development

No branches or pull requests

2 participants