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

Remove leading $, > and # chars from clipboard of console snippets #1346

Closed
wants to merge 1 commit into from

Conversation

diegocn
Copy link

@diegocn diegocn commented Sep 30, 2020

This PR aims to add a function to remove leading $, >, and # characters from the clipboard of console snippets.

Related PR: #1110
Related issues: #864, rust-lang/book#1756, rust-lang/book#2420, rust-lang/book#2179,

@diegocn diegocn changed the title Remove leading $, > and # chars from console snippets Remove leading $, > and # chars from clipboard of console snippets Sep 30, 2020
@diegocn
Copy link
Author

diegocn commented Oct 8, 2020

Hello folks, just a friendly reminder about this PR :D
@Michael-F-Bryan @budziq, @steveklabnik, @frewsxcv

@frewsxcv
Copy link
Member

frewsxcv commented Oct 8, 2020

I'm a little concerned this would be surprising behavior more than it'd be helpful behavior, so I don't feel comfortable merging it. Especially since it's trivial for a user to remove special leading characters from console output. Not blocking though in case someone else approves.

@diegocn
Copy link
Author

diegocn commented Oct 12, 2020

Hi @frewsxcv. I did not get your point. Would you mind to elaborate a little bit more on the reason to not accept this PR?

From my point of view, as a user/consumer of the Rust book that uses mdBook, I expect that the action of copying/pasting console snippets just works without the need to remove a unnecessary leading char. The related PR/Issues shows that other users think the same.

@tim-seoss
Copy link
Contributor

concerned this would be surprising behavior more than it'd be helpful behavior

I've encountered this behavior (i.e. copy button excludes shell prompts) on a few other sites in the past, and personally have always found it pleasantly surprising.

I can't think of many circumstances where this would be the "wrong thing" to do (and on the few occasions when it would be undesirable, it can be worked around trivially, by manually selecting the entire block, including the prompts - whereas removing the prompt characters is a more drawn out manual process).

Here's an online demo for another implementation of the same feature, which can be used to try out the behaviour (and workaround): https://sphinx-copybutton.readthedocs.io/en/latest/

Also here's another example of irritation with the current behaviour. The example cited is a (broken link) to the Embedded Rust "Discovery" mdBook, which has since moved here.

@diegocn
Copy link
Author

diegocn commented Nov 9, 2020

Hi @tim-seoss, thanks for your comment.

I really like the Sphinx-copybutton example that you bring here.

@diegocn
Copy link
Author

diegocn commented Nov 19, 2020

Here is another example of a copy button that removes the leading $
https://www.envoyproxy.io/docs/envoy/latest/start/install#install-envoy-on-debian-gnu-linux

@frewsxcv
Copy link
Member

frewsxcv commented Nov 19, 2020

Here is another example of a copy button that removes the leading $
https://www.envoyproxy.io/docs/envoy/latest/start/install#install-envoy-on-debian-gnu-linux

Here's the GitHub Issue and PR for this feature: readthedocs/readthedocs.org#4698 readthedocs/readthedocs.org#4990

And here's the library they're using: https://sbrunner.github.io/sphinx-prompt/

This seems to be a different approach than the one proposed in this PR. Their approach requires the user to opt-in to making the prompt prefix symbol unselectable, whereas the approach in this PR is to remove all prompt prefix symbols whether the user wants it or not

@tim-seoss
Copy link
Contributor

Looking at: https://sphinx-copybutton.readthedocs.io/en/latest/

By default, if sphinx-copybutton detects lines that begin with code prompts, it will only copy the text in those lines (after stripping the prompts). This assumes that the rest of the code block contains outputs that shouldn’t be copied.

When this default hasn't been opted out of, the user is still able to easily select the text, including the prompts i.e.

  1. Mouse or finger to drag-select (click-select, etc.) the text in question, then
  2. Copy via context menu etc.

The prompt stripping only occurs when the copy icon at the edge of the text block element.

If you consider the case where the user is presented with a list of 5 shell commands, and wants to execute them all, having visually inspected them...

i.e. With this functionality in place the user has:
"I want the commands without the promtps" → Very easy
"I want the commands with the prompts" → Easy

... you can verify this by experimenting with the example in the above link.

Without the functionality (or with it disabled), the user has:
"I want the commands without the promtps" → Hard
"I want the commands with the prompts" → Very Easy

I can't think of an occasion when I've wanted to copy shell commands with the prompts, but if I did, it would still be reasonably easy to do-so (and by an IMO intuitive method).

@alsyia
Copy link

alsyia commented Mar 3, 2021

I began reading the Book today, and I came here to find if there already was an issue or a PR about this. It would be a nice feature to have in my (very humble) opinion!

@ISSOtm
Copy link
Contributor

ISSOtm commented Aug 31, 2021

I wanted this feature, so I implemented it in my project's custom mdBook back-end. The result is available online.

NB: As a comment above notes, it's very fragile (I won't even try disputing that, I'm fine with relying on impl details & having to update those bodges periodically for my own stuff), but it still produces something that works for the end user and can be judged, and the implementation will be cleaned up if this is deemed good enough to upstream.

The logic is as follows:

  • Only apply this filtering to the console language (which is just an alias for shell), since that's where you'll want this behavior.
  • Only apply it to the lines starting with $ ; this is not exactly what highlight.js does, though.
  • Remove the console language, since we don't want the lines themselves to be highlighted, and instead create inline spans tagged with the bash language, again imitating what hljs does. Lines that don't start with a prompt are not highlighted, anyway.

It's not the most general logic, it's what works for my case; the custom renderer is a bunch of bodges, so I didn't bother making it generic & clean, but I would bother if this was to be upstreamed.

Rationale:

  • This works JS-less, allowing plain copy-paste without the hassle, including if you only want some of those lines still without the prompt.
  • This also does not require tampering with the clipboard.
  • The creation of a separate console-line element allows giving it the hljs-meta class as well so that styling is not lost but not duplicated either.
  • This would be cleaner as a preprocessor, but I think I encountered a couple of snags? I'd have to check again. Shouldn't be a big deal in theory, since markup gets processed neither in code blocks nor in HTML tags.
  • This does duplicate some logic, but the alternative to that (while keeping the behavior) would be to scan all elements after hljs generated the lines, and performing the transformation then, which I like less because of the heavier CPU tax (DOM manipulation!) & still complex logic.

@c-git
Copy link

c-git commented Aug 14, 2022

Given how often people create issue on the rust book I think most users find this surprising that the $ is copied.
The most recent issue. Counting off the top of my head this is at least the 2nd or 3rd this month.

@tim-seoss
Copy link
Contributor

Independent of this PR as it currently stands, is there any consensus on whether this would a useful enhancement? I suppose the options are:

  • Never useful (bad UX)
  • Useful but should be an opt-in feature, perhaps just using a special "language" for this, e.g. with something like:
    ```shell-commands
    $ curl https://example.org
  • Useful if unconditionally applied

Personally, I think as an opt-in feature it would allow each individual book author to decide on whether this was appropriate, so wouldn't risk changing behaviour in a surprising way for any existing books. Any thoughts?

@ISSOtm
Copy link
Contributor

ISSOtm commented Aug 16, 2022

I think it's useful to apply it to the console language specifically, being exactly the "shell commands preceded by prompts" language.

@diegocn
Copy link
Author

diegocn commented Aug 23, 2022

Hi folks! I had forgotten this PR (as it's been here for almost 2 years).
@ISSOtm that conditional does exactly what you have said.

I think it would be nice to have an opt-in if this is something that might cause unexpected behaviors when forced.

@ISSOtm
Copy link
Contributor

ISSOtm commented Aug 28, 2022

I'd consider the console language to be a suitable opt-in, since it's one of at least three aliases to the same language.

@hamirmahal
Copy link
Contributor

I ran into this issue again today when copying

$ curl --proto '=https' --tlsv1.2 https://sh.rustup.rs -sSf | sh

from https://doc.rust-lang.org/book/ch01-01-installation.html#installing-rustup-on-linux-or-macos.

@diegocn
Copy link
Author

diegocn commented Oct 28, 2023

Closing this. There’s absolutely no point in keeping such a simple change waiting for 3+ years for a resolution.

@chriskrycho
Copy link

@ehuss and @Dylan-DPC, the issue this meant to address just got flagged up again over on TRPL. Is there anything I can do to help get this particular PR or another like it actually landed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants