im-tomu / fomu-workshop

Support files for participating in a Fomu workshop
https://workshop.fomu.im
Apache License 2.0
162 stars 64 forks source link

Code blocks in docs don't let you select text and make the docs more difficult to use. #144

Closed aerohoff closed 4 years ago

aerohoff commented 4 years ago

The code blocks have a "click to copy" feature (at least in Safari and Chrome on macOS) which copies the whole block of text; terminal prompts, command, and output to the clipboard. This makes it difficult to select and copy just the command.

ewenmcneill commented 4 years ago

FWIW, I too find this annoying: the code blocks just auto select everything and copy everything to the clipboard, which is a useless feature, as it includes the prompt, and often the command output too. Thus it's impossible to directly cut'n'paste the example commands, you end up having to type them in again.

Perhaps the "clippy" helpful "I copied it all for you" Javascript could be disabled in favour of allowing users to use their native copy'n'paste functionality again, to copy the relevant subsections? (And if not, perhaps the code sections could be restructured so that there are bits which have just the commands, for which "click to copy everything" is actually useful instead of must an anti-feature.)

Ewen

mithro commented 4 years ago

@ewenmcneill On most people's browsers, the click to copy only copies the commands needed to run. It would be good to know what browser + version you are using.

ewenmcneill commented 4 years ago

@mithro It happens for me on Firefox 73.0 (64-bit) for Ubuntu 18.04 (the canonica-1.0 build), Firefox 68.5.0esr (64-bit) for macOS 10.14, and Safari 13.0.5 (14608.5.12) on macOS 10.14. Ie, on all of those "click to copy" causes the entire block to be copied, including prompts, command output, etc, and prevents native cut'n'paste of the actual command line ( by auto-copying-whole-block/auto-deselecting-selection on any attempt to select a useful range of text.... like the command to run). Those are the three browsers I use, so it's happening on all browsers that I use :-(

Perhaps this is functionality that only works, on eg, certain Chrome versions, and that counts as "most people's browsers"? And/or "Chrome derived" browsers (which is a non-trivial number of them these days, but not ones that i use). I still think it'd be best to avoid the "best viewed in IE6^W the browser we use" Javascript.... :-)

Ewen

ewenmcneill commented 4 years ago

FWIW, on loading the pages (not on using copy functionality) these appears in the Firefox web console:

Empty string passed to getElementById(). sphinx_symbiflow_theme.js:699:1207
Source map error: Error: request failed with status 404
Resource URL: https://workshop.fomu.im/en/latest/_static/sphinx_symbiflow_theme.js
Source Map URL: /sphinx_symbiflow_theme.js.map

And on Safari, i'm getting two 404s -- the other is for sphinx_symbiflow_theme.css.map.

Also FWIW my Firefox browsers are configured to block third party cookies (IIRC that's now the default, but I've had it set for ages; I'm reminded because one of my Firefox instances logged a bunch of reports of blocking that to readthedocs); from memory that's not an option in Safari (and I tend to use Safari only as a work around for websites that don't work with my standard Firefox config...

Ewen

mithro commented 4 years ago

@ewenmcneill - Couple of things;

ewenmcneill commented 4 years ago

@mithro I'm assuming you're meaning the first Example (at https://sphinxcontrib-session.readthedocs.io/en/latest/#example for me; the link you give seems to open in the middle of the table of contents, starting at that Example).

If so, that looks much more promising. Thanks for investigating.

For that Python example:

My main note for that Python example is that the whitespace indent in Python is significant, and it's not included (the copy I get is all hard left, no leading white space). So it'd be "nice to have" (but not essential) that the significant leading white space in Python examples was also included. (Eg, detect REPL prompt plus separating white space, then include all other white space.) That could easily be a different issue for later though.

As for "click to copy", if the "click to copy" is configured in such a way that it always "does what I mean" then I can live with it taking over the whole selection behaviour. (It's just taking over the whole selection behaviour and doing the wrong thing which is especially aggrevating :-) ) So "tie action to 'click to copy' button" could be a "nice to have" issue for later if the main behaviour is working for our examples now (ie, only selecting the command to run, which IIRC are only single line commands). Off the top of my head it'd need a float with an onClick() that could then find the related code block and trigger the copy of that... and I'm not sure how that'd interact with the DOM JS permissions model.

Thanks for digging into this,

Ewen

Firefox 68.5.0 ESR (macOS) selection:

Firefox 68.5.0 ESR (macOS) selection

Safari 13.0.5 (macOS) selection:

Safari 13.0.5 (macOS) selection

mithro commented 4 years ago

@ewenmcneill - This should be fixed now, can you confirm.... Screenshot from 2020-02-17 11-53-30

ewenmcneill commented 4 years ago

@mithro Capturing significant whitespace seems to work in both Firefox 68 ESR and Safari 13 (both on macOS) for me. Safari still has a visual artefact showing one of the REPL prompts included in the selection, but the actual cut'n'paste text is perfect, so I think we just ignore that visual artefact. Your current version is clearly much better than what I was seeing previously.

Thanks for fixing it,

Ewen

Firefox 68 ESR (macOS) Safari 13 (macOS)

mithro commented 4 years ago

@ewenmcneill It is weird that the prompt is also colored wrong too...

ewenmcneill commented 4 years ago

It is weird that the prompt is also colored wrong too...

@mithro The "coloured wrong" (in Safari) is what I'm talking about for the visual oddness, ie the visual representation of the selection shows text that won't be copied. The result of copying "does the right thing", so I think it's fine. FWIW it looks like a normal prompt, in Safari, when I'm not doing a selection over it, so I think Safari's UI rendering is just a bit confused.

Ewen

Screenshot-2020-02-18-12-43-04
ewenmcneill commented 4 years ago

@mithro Now that you've got working JavaScript for copying just the right bits (ie, the commands to paste in), what needs to happen before it can end up built into, eg, the Fomu workshop at https://workshop.fomu.im/en/latest/?

Do you need to get your code upstream into Sphinx or similar? And then rebuild the Fomu workshop? Is there an upstream issue/pull request that I can encourage merging?

Ewen

mithro commented 4 years ago

@ewenmcneill It's not already there?

ewenmcneill commented 4 years ago

@ewenmcneill It's not already there?

@mithro Testing further, it seems like it's unevenly applied. And it happened that the page I was up to https://workshop.fomu.im/en/latest/verilog.html is one of the ones that it doesn't work on :-)

Eg, https://workshop.fomu.im/en/latest/python.html seems to work okay (eg, Firefox 73.0.1 on Ubuntu Linux; Safari 13.0.5 on macOS). Although with "one line to copy" examples it does seem to copy leading spaces in that one command line which is presumably a side effect of the way the Python indenting works. But other than that every example on that page works correctly, just copying the "text you need to input". As do many of the other pages I tried with Python or Shell command line examples.

But the verilog example https://workshop.fomu.im/en/latest/verilog.html still copies the entire pre block on Firefox 73.0.1 on Ubuntu Linux / Firefox 68.5.0esr on macOS / Safari 13.0.5 on macOS.

Another example that fails(copies all the text) is the Rust examples on https://workshop.fomu.im/en/latest/riscv.html. And the "running the app in Renode" example on https://workshop.fomu.im/en/latest/renode-zephyr.html, and the "peripherals" example on https://workshop.fomu.im/en/latest/renode-bridge.html.

So my current assumption is that it's now mostly working, but there are some examples that, eg, need additional markup to indicate "these are the bits to copy". (Plus it'd be nice to trim out the common to all lines leading whitespace, while retaining the "not all lines have this" white space; I know roughly how I'd do that -- baby duck off the first line -- but I don't know where the code you were editing lives.)

Ewen

ewenmcneill commented 4 years ago

So my current assumption is that it's now mostly working, but there are some examples that, eg, need additional markup to indicate "these are the bits to copy".

Aha, reading https://sphinxcontrib-session.readthedocs.io/en/latest/ and comparing https://raw.githubusercontent.com/im-tomu/fomu-workshop/master/docs/verilog.rst and https://raw.githubusercontent.com/im-tomu/fomu-workshop/master/docs/python.rst hints at what is missing: .. session:: takes an optional language suffix. And presumably that language suffix is driving the JavaScript which decides what to copy. That language suffix is missing from the verilog.rst for instance; the Python one has:

.. session:: shell-session

but the Verilog one has only:

.. session::

I'll make a pull request for the ones that I can find with grep :-)

Ewen

ewenmcneill commented 4 years ago

I'll make a pull request for the ones that I can find with grep :-)

https://github.com/im-tomu/fomu-workshop/pull/185 has the ones that I could find an easy fix for hopefully fixed. Including the verilog.rst and riscv.rst ones. The renode-* ones are harder, as several of the "copy everything" examples are renode "monitor" examples, which has its own prompt syntax that I'm not sure any of the existing parsers will support. So I've left those alone, other than the one code:: bash example which seemed like it should just work as session:: shell-session so I changed it over.

At a guess once that is merged, pushed up to the workshop website, and tested, we've done as much as possible and should close this issue.

Ewen

ewenmcneill commented 4 years ago

@mithro @aerohoff I feel like we solved this issue as much as is possible? Ie, the auto-select magic now (mostly, apart from some "unknown language" examples) selects just the text you'll want if you want the whole example.

So possibly we could close this issue now?

Ewen

PS: Brought to you by "why is this still on my open issues list in GitHub" :-)

aerohoff commented 4 years ago

Hey, sorry. I confirmed that I can select text as expected. I'm allowed to highlight and copy the commands without the >>> python prompt. Thanks you!

aerohoff commented 4 years ago

Ooh, and the button doesn't copy the prompts! Well done!