Add shorthand opts, plus many bug fixes and debugging features

hackerb9 commented 3 years ago

@astrand: please double-check that this works as expected and introduces no new problems. I have been testing this out (on the branch "shorthandopts") for a while and am confident in it. It seems quite reliable and doesn't fail in certain cases (see borked.c) that cause the current release of xclip to fall over.

Some features:

* Short letter opts, like '-c' for '-selection clipboard'
* Several INCR transfer bugs fixed (e.g., hanging processes, xfr failure)
* Better (though still not finished) ICCCM compliance
  + XErrorHandlers xchandler() and xcnull()
    > xcnull sets the global variables xcerrflag and xcerrevt
    > xchandler also sends a client message and shows debugging info
  + catch X alloc (and other) errors using new xcchangeprop() function
  + use correct maximum request size (multiply, not divide, by four)
  + catch selection owner signalling an error via NULL property
    (New state XCLIB_XCOUT_SELECTION_REFUSED)
  + requestors are no longer identified by their window IDs alone
    (Now uses property name as well. Note that this still isn't perfect.)
  (Next ICCCM bug to fix: XSetSelectionOwner shouldn't use CurrentTime)
* Workaround for a bug in xsel's maximum request size (4,000,000 bytes)
* Bash tab completion (e.g., "xclip -t TAB" will show available targets)
  ('make install.completion' to install)
* PDF rendering of man page
* Improved xctest script: test large files, easier to understand output
* "Borked", a purposefully broken X selection program for testing
* Autogroff.sh for 'make watch'.
  (so man page can be edited easily while watching the PDF file update.)
* Garrulous -debug output, for when -verbose isn't enough
* Improved man page and command line help

astrand commented 3 years ago

Wrt reviewing the code: Good that you are using "fixup" messages, but it would be even better if you could do git rebase -i --autosquash. Then force push. Can you do that?

hackerb9 commented 3 years ago

Yes, I can do that. I just need to learn a little more. I actually attempted to rebase -i (a few different ways) before the pull request, but failed.

Git gave me some guff, saying there was a "conflict" during the interactive rebase. I'll keep trying and figure it out.

hackerb9 commented 3 years ago

@astrand: Have you had a chance to try out this branch? I've been using it as my everyday xclip and have had no problems.

I think I've squashed the history as much as makes sense logically (and that I can do without causing strange git breakage). Did I do enough? If it's helpful to you, I am willing to try learning more about how to reorder commits for squashing. Alternately, I've found it useful to look at the overall diff instead of the patch history: https://github.com/astrand/xclip/pull/88.diff

astrand commented 3 years ago

I have now done a checkout, build and quick test run. Basic test seems to work, but strange behaviour when running without arguments:

[astrand@sara xclip]$ /tmp/xclip/bin/xclip /tmp/xclip/bin/xclip-[astrand@sara xclip]$

The output string seems to vary.

hackerb9 commented 3 years ago

Hi @astrand,

That's actually what it is supposed to do:

“Sensible stdin”: If stdin is a keyboard (tty), xclip will default to -output.

The old behavior was to silently block, waiting for the user to type Control-D at the beginning of a line. As we saw in a bug report last week, the old behavior can lead to user confusion where they think xclip is broken. But confusion isn't the primary reason for the change. I believe it's a major improvement in usability as users no long need to think about whether -i or -o is required. (E.g., this now works: xclip | figlet | xclip).

This is actually the issue that got me motivated to start working on the xclip source code over a year ago. I hope you're not surprised that it's in this pull request. I first mentioned it in pull requests #63 and #65. After I split the pull request into parts, you had seemed generally positive but had no time to review the changes for stability before merging, but mentioned the possibility of feedback from the community. None ever came in, but I was left with the impression that you thought the idea was a good one, if only it could be reviewed.

If you want, I can share more thoughts about why it's a good and important change.

As for stability, I have personally tested my changes quite extensively, even adding new tests to xctests and creating a new tool (borked) for simulating broken clients. I am positive this merge is vastly more stable than the current release of xclip. But, of course, I'd feel better if there was at least some testing by someone who's not me when I make major improvements, especially when it changes the behavior xclip users may be accustomed to.

astrand commented 3 years ago

Hi again, thanks for explaining this. Haven't noticed this in the other pull requests. Wrt the "sensible stdin", I'm not convinced. I see several problems with it: 1) It breaks backword compatibility; this is not how xclip worked before 2) It violates https://www.gnu.org/prep/standards/standards.html#User-Interfaces 3) It's a bit strange that "xclip | grep something" works but that if you use it as part of another pipeline, or run it through SSH ("ssh localhost xclip | grep something"), xclip will seem to hang. 4) It can actually be very useful to just run "xclip" in order to type something into the clipboard.

There's some discussion about this principle here: https://news.ycombinator.com/item?id=8484718

For me, changing the behaviour slightly by checking if stdin is a tty might be OK, but in this case the main operating mode is changed. I think this can lead to a lot of confusion; the SSH example above is just one such case. I am sure there are many others.

Perhaps it is possible to find some "smaller" solution or middle ground? For example, require -o for output as it is today, but require explicit -i if stdin is a tty? Ie exit with an error/hint if tty without -i.

hackerb9 commented 3 years ago

I've thought hard about this and I've written up responses only to find that they are too verbose. I don't want to waste your time, but I do want to communicate. Let's see if I can manage it this time. ☺

The "middle ground" solution of requiring users to specify either -i or -o to choose a mode is actually further away from my goal of requiring less cognitive effort from users. If an argument is always required to pick a mode, it'd be better to just have two utilities — xcin and xcout — instead of one.

I could, however, go for a similar situation, where the documentation would say:

“Xclip has two modes: in and out. One or the other is required to be specified in careful usage, but the casual user may leave them off and let xclip "guess" which is meant (based on stdin being a tty). Xclip nearly always gets it right, but don't rely on that behaviour: always explicitly specify either -i or -o in scripts.”

astrand commented 3 years ago

I've thought hard about this and I've written up responses only to find that they are too verbose. I don't want to waste your time, but I do want to communicate. Let's see if I can manage it this time.

Thanks :-)

The "middle ground" solution of requiring users to specify either -i or -o to choose a mode is actually further away from my goal of requiring less cognitive effort from users. If an argument is always required to pick a mode, it'd be better to just have two utilities — xcin and xcout — instead of one.

Don't agree on this one, though. For example, "tar" can both extract and and create archives.

I could, however, go for a similar situation, where the documentation would say:

“Xclip has two modes: in and out. One or the other is required to be specified in careful usage, but the casual user may leave them off and let xclip "guess" which is meant (based on stdin being a tty). Xclip nearly always gets it right, but don't rely on that behaviour: always explicitly specify either -i or -o in scripts.”

I think we take a step back and consider which problem we actually are trying to solve; why the current implementation is problematic. For example, the problem with users getting confused by "xclip" without arguments (waiting for input from TTY) can be solved with a warning.

When in doubt, I think it is best to look at how existing Linux utils works, and I haven't seen many commands which automatically detects the mode based on "position in the pipeline" so to say.

Janfel commented 3 years ago

I believe these are the problems we are trying to solve:

The current behavior is surprising and unintuitive.
Having to manually specify the mode is “stating the obvious” and a poor user experience.

The user getting confused by xclip without arguments is just a symptom of the first problem. This is how a new user would intuitively expect xclip to behave:

Command	Expected Behavior	Corresponding Arguments
`xclip`	Print the contents of the clipboard.	`xclip -out`
`foo \\| xclip`	Put something into the clipboard.	`xclip -in`
`xclip \\| bar`	Get something out of the clipboard.	`xclip -out`
`foo \\| xclip \\| bar`	`foo \\| tee /dev/clipboard \\| bar`	`xclip -in -filter`
`xclip < foo.txt`	Copy the contents of `foo.txt` to the clipboard.	`xclip -in`
`xclip > bar.txt`	Write the contents of the clipboard to `bar.txt`.	`xclip -out`
`foo $(xclip)`	Pass the contents of the clipboard to `foo`.	`xclip -out`

According to the Principle of Least Surprise, this is also how xclip should behave by default. This brings the distinct advantage, that a new user can simply guess how to use xclip, just by knowing that it “provides an interface to X selections”. I believe that a tool should do what the user wants, even if that might not be what they expect. If you want a list of file names, then dir > list.txt does what the user expects, but ls > list.txt does what the user actually wants. The fact that pretty much everybody uses ls instead of dir proves my point.

Regarding the points you made in https://github.com/astrand/xclip/pull/88#issuecomment-727265527:

It breaks backword compatibility; this is not how xclip worked before.

I do not believe there are many scripts that depend on this particular behavior. And even if there are, fixing them would be very easy, on top of making them more robust in general. Furthermore, we will not be able make the behavior of xclip more intuitive without actually changing it.

It violates https://www.gnu.org/prep/standards/standards.html#User-Interfaces. [...] When in doubt, I think it is best to look at how existing Linux utils works, and I haven't seen many commands which automatically detects the mode based on "position in the pipeline" so to say.

These are some of the tools that detect their “position in the pipeline” to provide a better user experience. The standard even explicitly exempts some of them:

ls, exa, lsd, etc. provide a column view when stdout is a TTY.
less, man, journalctl, git log, etc. only act as a pager when stdout is a TTY.
bash, zsh, tcsh, python, lua, etc. only provide a REPL when stdin is a TTY.
tar, gzip, bzip2, xz, lz4, etc. do not print processed data to a TTY.
ripgrep, ack, ag, etc. search the current directory instead of stdin when it is a TTY.
bat, exa, ripgrep, ack, ag, etc. use the equivalent of --color=auto by default.

It's a bit strange that "xclip | grep something" works but that if you use it as part of another pipeline, or run it through SSH ("ssh localhost xclip | grep something"), xclip will seem to hang.

By saying “use it as part of another pipeline”, are you talking about foo | xclip | bar? This can be solved by activating -filter in such cases. See #115. Using xclip over SSH fails for me with xclip: Error: Can't open display: (null).

It can actually be very useful to just run xclip in order to type something into the clipboard.

There are many other ways of doing this, like heredocs (xclip <<EOF) and cat (cat | xclip). These have the benefit of working the same for all programs that read from stdin.

In conclusion: I agree with @hackerb9. While using isatty can harm usability, it can also improve it greatly. And using it to make xclip more intuitive and easier to use is very much a case of the latter.

astrand commented 3 years ago

Thanks a lot for this extensive feedback! A few comments below:

Command Expected Behavior Corresponding Arguments xclip Print the contents of the clipboard. xclip -out foo | xclip Put something into the clipboard. xclip -in xclip | bar Get something out of the clipboard. xclip -out foo | xclip | bar foo | tee /dev/clipboard | bar xclip -in -filter xclip < foo.txt Copy the contents of foo.txt to the clipboard. xclip -in xclip > bar.txt Write the contents of the clipboard to bar.txt. xclip -out foo $(xclip) Pass the contents of the clipboard to foo. xclip -out According to the Principle of Least Surprise, this is also how xclip should behave by default.

For me, such a behaviour is actually surprising. As you can see in my comment on 8 Nov 2020, I was actually thinking it was a bug. Wrt git, for example, I actually type "git branch | cat" several times per day, to work around that "helpful" feature of starting a pager. Ok, I admit that probably the "git branch | cat" is used more seldomly than "git log | less". So it saves some keystrokes per day, but still not very obvious or inituitive.

It breaks backword compatibility; this is not how xclip worked before.

I do not believe there are many scripts that depend on this particular behavior. And even if there are, fixing them would be very easy, on top of making them more robust in general.

I cannot really see how they would be more robust than with the existing behaviour, where the mode needs to be specified?

Furthermore, we will not be able make the behavior of xclip more intuitive without actually changing it.

I agree on this one!

It's a bit strange that "xclip | grep something" works but that if you use it as part of another pipeline, or run it through SSH ("ssh localhost xclip | grep something"), xclip will seem to hang.

By saying “use it as part of another pipeline”, are you talking about foo | xclip | bar? This can be solved by activating -filter in such cases. See #115. Using xclip over SSH fails for me with xclip: Error: Can't open display: (null).

You need to make sure X11 forwarding is setup (ie use -X etc).

It can actually be very useful to just run xclip in order to type something into the clipboard.

There are many other ways of doing this, like heredocs (xclip <<EOF) and cat (cat | xclip). These have the benefit of working the same for all programs that read from stdin.

In conclusion: I agree with @hackerb9. While using isatty can harm usability, it can also improve it greatly. And using it to make xclip more intuitive and easier to use is very much a case of the latter.

Personally, I'm not convinced. However, it seems there are at least 3 persons positive for this change now, and I'm not that active in the project any longer. Therefore, I think it would be wrong for me to "block" this change. Therefore, it's OK with me to merge this feature.

Thanks again for your dedicated work :-)

brodyck commented 2 years ago

Hi,

I am experiencing the issue in #43 using master despite it apparently being fixed. Using the hackerb9 branch fixes this issue for me.

Is there any plans to push this forward? I am, unfortunately, not versed in this code, C, or git well enough to help out a whole lot.

If you know of a way I can use git to simply test which set of commits makes this work, I can try that; and if I can find a working combo, a separate pull request could be made where only those changes that fix my variation of #43 are merged. That way the blocking issues with the flags aren't in the way.

I have been trying to do this on my own with cherry-pick and testing individual commits with no success. I'll continue as I have time.

AtomToast commented 1 month ago

I have the same issue as @brodyck. Would it be possible to at least merge just the part that fixes #43? Otherwise, since this seems like it would be quite a significant release and @astrand does not appear to be working on this project actively anymore, while having mentioned being fine with this being merged, is there even anything else opposing this getting merged? Or is there even anybody left still able to get this merged, since most of recent development seems to be done by @hackerb9. They also seem to have stopped working on this project for 2 years now.

astrand / xclip

Add shorthand opts, plus many bug fixes and debugging features #88