ENH: Exclude unselectable text from being copied (update) #178

rkdarst · 2022-09-13T08:21:19Z

This is an update of #138, rebased to remove the conflicts. In a local test, it seems to work.

I haven't adjusted any of the code from that PR. It seems the only conflict was with the docs rearrangement, I excluded that from the rebase and then re-added it back.

I did add .gp to the default copybutton_exclude value. The reasoning behind that is here (and previous comments): #138 (comment)
I think that if default Sphinx excludes .gp, it is best to copy that into our defaults. In the long-term, it is fair to ask "should the exclusion information be in two places", but the previous PR was adjusted to use existing info (= detect what is un-selectable) to duplicating .linenos being configured, so I guess we stick with that for now.

Note that I am just the messenger here, all actual work on this comes from @bicarlsen (and commit authorship is preserved)

Closes: #138

…electable text should be excluded from copy and do so if needed. [_static/copybutton_funcs.js] Added functionality to get unselectable elements and remove their text.

…ith funcitons using .innerText. [package.json] Added browser-env module for DOM support in testing. [docs/index] Updated documentation. [copybutton, copybutton_funcs] Fixed minor errors. Updated default exclude text. [__init__] Updated default exclude text.

for more information, see https://pre-commit.ci

- `.linenos` is the line numbers, and `.gp` is the tag from the pygments highlighter for things like the `$` prompt. Upstream sphinx CSS excludes `.gp` from being copied, see the analysis here: executablebooks#138 (comment) - If upstream sphinx excludes `.gp`, we probably should as well.

- Re-add documentation from PR executablebooks#138 - that PR got conflicted with a docs rearrangement, so I add the docs back in one chunk now. I include my own suggestions in that PR.

choldgraf · 2022-09-13T09:53:10Z

Just a note that I don't have much time to give this PR a thorough review. If somebody wishes I am happy to give merge privileges to push things forward, and I welcome a review from somebody else out there.

rkdarst · 2022-09-17T21:17:45Z

This is in production in some of our sites. To do a test, try to select text in the code blocks in the link, and see if you can select the $. Then, use the copy button and see if the $ gets copied. (while these don't test it, it should be similar with things like >>> in Python prompts).

Example with sphinx_rtd_theme:
https://coderefinery.github.io/git-intro/basics/#recording-a-snapshot-with-git

Example with sphinx_book_theme:
https://coderefinery.github.io/git-intro/branch/sphinx-book-theme/basics/#recording-a-snapshot-with-git

Example with sphinx_book_theme that does not use these changes. The copy button copies the $ which will make copy and paste not be pasteable into a shell:
https://coderefinery.github.io/git-intro/branch/sphinx-book-theme-old-copybutton/basics/#recording-a-snapshot-with-git

On investigation, I found that sphinx-book-theme does exclude .gp from user-select by default. This comes from basics.css, which I presume comes from upstream Sphinx (that is where I see it, at least). So I think that excluding .gp makes sense even if sphinx-copybutton is mainly used in sphinx-book-theme.

(the branch links above might disappear if those branches are deleted at some time in the future)

rkdarst · 2022-09-17T21:24:03Z

As for review, don't trust me but I think the most difficult review has been done in #138, and the main question here would be "excluding .gp" and "is the general code still like #138", "is documentation good enough", and "is the test included good enough". I haven't looked at the javascript code much.

choldgraf

This looks good to me - one question: do we really need all of the package-lock.json changes? Other than that I think it looks good to go. I generally try not to update the package requirements in a PR that also does other stuff unless really necessary (but maybe it is necessary?)

rkdarst · 2022-09-18T22:00:23Z

I really don't know, it was all in the other pull request. I can test with it removed and see what happens - and I will also give a closer check to see if there are any changes that seem significant. I don't know JavaScript/node so don't count on me that much! Thanks for the review!

bicarlsen · 2022-09-19T06:45:20Z

docs/use.md

+copybutton_exclude = '.exclude_me'
+```
+By default `.linenos, .gp` are excluded. If you specify the
+`copybutton_exclude` option it will replace the default.  `.linenos`


There are two spaces before .linenos.

sphinx_copybutton/_static/copybutton.js_t

sphinx_copybutton/_static/test.js

bicarlsen

Thanks both for your work on this. I included a few comments to clean up the final code before merging.

bicarlsen · 2022-09-19T06:54:27Z

This looks good to me - one question: do we really need all of the package-lock.json changes? Other than that I think it looks good to go. I generally try not to update the package requirements in a PR that also does other stuff unless really necessary (but maybe it is necessary?)

~~In general the package-lock.json file should be ignored by including it in the .gitignore file.~~

sphinx_copybutton/_static/copybutton.js_t

sphinx_copybutton/_static/test.js

rkdarst · 2022-09-19T07:29:45Z

It seems that package-lock.json has real changes in it - there are new dependencies, and these get added to package-lock.json. There don't seem to be changes to the versions that were there before. So, my thought is that it should stay updated as we see here.

If package-lock.json should be ignored, I'd propose to do that in a new pull request.

Again, not an expert here, but I like the general idea of the general dependencies, and the exact working dependencies - I would want to do that more in my projects.

rkdarst · 2022-09-19T08:58:17Z

Updates seem to work on a test deployment.

bicarlsen · 2022-09-19T09:09:41Z

It seems that package-lock.json has real changes in it - there are new dependencies, and these get added to package-lock.json. There don't seem to be changes to the versions that were there before. So, my thought is that it should stay updated as we see here.

If package-lock.json should be ignored, I'd propose to do that in a new pull request.

Again, not an expert here, but I like the general idea of the general dependencies, and the exact working dependencies - I would want to do that more in my projects.

My mistake. package-lock.json should be committed.

rkdarst · 2022-09-29T17:41:39Z

This PR's branch was used across eight different (but similar, all sphinx-rtd-theme) projects in a workshop over the last six weeks and no reports of problems. (it was this same workshop last time that prompted me to work on this).

choldgraf · 2022-09-29T17:48:24Z

Can I add somebody in this PR to the maintainers so that you can press the merge button? We should not be bottlenecking progress on myself!

rkdarst · 2022-09-29T18:18:14Z

I wouldn't mind pushing merge, but don't know enough javascript to give a more critical evaluation or fix hard things if they break (but at least I can help revert it if needed...). But if it's easier to add someone than change the technical limits and you ask me to push merge, I'm happy to!

choldgraf · 2022-09-29T18:53:50Z

Done! https://github.com/executablebooks/sphinx-copybutton/invitations

I don't think we have the luxury of bottlenecking decision-making only on people that know JavaScript well, I certainly do not consider myself one of those people :-)

rkdarst · 2022-09-29T18:57:31Z

Thanks for the invite! I assume this means I should merge when I can.
I accepted the invitation, but it still requests one approving review (which I can't do since I opened the pull request).

choldgraf · 2022-09-29T21:30:03Z

I'll go ahead and merge this one, but I welcome reviews and merges in the future from you! If others are interested in maintainer status I am happy to share this if you are willing to help out with the repository.

hypergravity · 2022-10-09T11:12:35Z

Hi! I am using this sphinx_copybutton in my sphinx documentation and trying to exclude line numbers. Here are what I have observed:

The exclusion function works well on my local machine but has no effect on my documentation built on readthedocs.io. More specifically, it fails when opened with Microsoft Edge/Safari but is good in Google Chrome.
Then I download the HTML format documentation built by readthedocs.io and open it in MS Edge/Safari, and it works again.

I am confused about whether the implementation of the line number exclusion depends on web browser or it is not well supported by readthedocs.io.
Here is the code block that I try to copy (the source code section): https://csst-proto.readthedocs.io/en/latest/csst_common/csst_common.html#source-code

choldgraf · 2022-10-09T11:26:07Z

could this be because it is not yet released? or has somebody made a release of this already?

hypergravity · 2022-10-09T11:43:29Z

The version of sphinx_copybutton on PyPI has not included this function. So I put git+https://github.com/executablebooks/sphinx-copybutton.git in my requirements.txt. I guess it's the correct version. It seems that the exclusion of line numbers depends on the web browser... It fails only on MS Edge and Safari (both on MacOS), but works well on Firefox(Linux) and Chrome (MacOS). All these web browsers are the latest version. What makes me more confused is that MS Edge is based on Chrome but they behave differently...

choldgraf · 2022-10-09T12:38:54Z

That is indeed very confusing - can you open an issue to explain the bug, expected behavior, when it breaks, examples, etc? That will be easier to track.

hypergravity · 2022-10-09T14:37:05Z

I have found the function is OK when visiting the web page with Safari on several other MacBooks.
I guess this problem is probably related to my web browsers rather than your package.
Once I can replicate this problem I will come and open an issue. It may take some time.
Anyway, thanks very much for your quick response! @choldgraf

rkdarst · 2022-10-11T07:07:23Z

Could it be some sort of javascript blocker or adblocker (or other extension)? That often messes up sites for me (but not this sphinx-copybutton case with my particular settings...)

hassec · 2022-11-17T11:25:27Z

@rkdarst @choldgraf is it possible that this PR clashes with settings like

copybutton_prompt_text = r">>> |\.\.\."
copybutton_prompt_is_regexp = True
copybutton_only_copy_prompt_lines = True

This doesn't do what the documentation says anymore. I think the reason is that by default .gp is now filtered out which means that >>> and ... are already filtered out here before formatCopyText is called and thus the regex logic of finding any prompt inside that function won't work...

sphinx-copybutton/sphinx_copybutton/_static/copybutton.js_t

Lines 154 to 157 in 4345c48

    
           let exclude = '{{ copybutton_exclude }}'; 
        
           let text = filterText(target, exclude); 
        
           return formatCopyText(text, {{ "{!r}".format(copybutton_prompt_text) }}, {{ copybutton_prompt_is_regexp | lower }}, {{ copybutton_only_copy_prompt_lines | lower }}, {{ copybutton_remove_prompts | lower }}, {{ copybutton_copy_empty_lines | lower }}, {{ "{!r}".format(copybutton_line_continuation_character) }}, {{ "{!r}".format(copybutton_here_doc_delimiter) }})

setting

copybutton_exclude = ".lineos"

stops filtering .gp out and then I observe the expected behavior and with the above settings.

rkdarst · 2022-11-17T11:38:37Z

Oh, interesting. You know, I didn't even know about those settings.

But also, in theory that shouldn't be needed. In your case, do you highlight with the pygments language pyconsole (or wahtever it's called)? The lexer marks that as a prompt (.gp), and then it gets excluded. (in my opinion) prompt exclusion should happen at the lexer level, not the copying level.

But copybutton_only_copy_prompt_lines is definitely a real issue, not the above, right? So relying on the lexer doesn't help unless it's re-thought.

Hm... what to do... at least docs need to be updated right away.

.go is the lexer class for "output from the consoles", that could be excluded? What if you add that to copybutton_exclude - are there problems with linebreaks and stuff?

hassec · 2022-11-17T12:39:12Z

@rkdarst .go exclusion works partially. It does filter out the console output, but it does leave one empty line per filtered line in the output which is a bit cumbersome.

I don't have a strong preference where the filtering should happen, but it does seem like copybutton_exclude doesn't work nicely with the regex based exclusion and the new default .lineos, .gp breaks the regex based functionality.

choldgraf · 2022-11-17T13:06:02Z

Could somebody please open an issue to track this bug and how to reproduce it + links to examples? I think that will be easiest at figuring out how to fix it. I am happy to review PRs if somebody thinks they know how to resolve this

rkdarst · 2022-11-17T13:07:28Z

yes, I will open a new issue once I have time, and describe the two conflicting strategies and the trade-offs. + update docs to say the current state.

flywire · 2023-01-13T22:22:05Z

yes, I will open a new issue

Please make sure example works for empty lines in code ie prompts with and without a trailing space: >>> and >>>

See #194

bicarlsen and others added 11 commits September 13, 2022 10:36

[__init__] Added config value. [_static/copybutton.js_t] Check if uns…

058bf6a

…electable text should be excluded from copy and do so if needed. [_static/copybutton_funcs.js] Added functionality to get unselectable elements and remove their text.

[test.js] Removed unused function from import.

3bdbe6f

Added documentation.

1d1ffdb

[__init__] Removed config value. Cleaned up rest of code.

04144f1

Always remove Nodes. Give user option to exclude other Nodes.

f2dec52

Fixed bugs.

77e70de

Changed formatting.

a2a275a

[pre-commit.ci] auto fixes from pre-commit.com hooks

417be14

for more information, see https://pre-commit.ci

copybutton_exclude: Add documentation (copied from executablebooks#138)

0674a7d

- Re-add documentation from PR executablebooks#138 - that PR got conflicted with a docs rearrangement, so I add the docs back in one chunk now. I include my own suggestions in that PR.

This was referenced Sep 13, 2022

Prompt (e.g. $ is copied with sphinx-copybutton) coderefinery/sphinx-lesson#93

Open

Exclude unselectable text from being copied #138

Closed

choldgraf reviewed Sep 18, 2022

View reviewed changes

bicarlsen reviewed Sep 19, 2022

View reviewed changes

sphinx_copybutton/_static/copybutton.js_t Outdated Show resolved Hide resolved

bicarlsen reviewed Sep 19, 2022

View reviewed changes

sphinx_copybutton/_static/copybutton.js_t Outdated Show resolved Hide resolved

bicarlsen reviewed Sep 19, 2022

View reviewed changes

sphinx_copybutton/_static/test.js Outdated Show resolved Hide resolved

bicarlsen reviewed Sep 19, 2022

View reviewed changes

rkdarst commented Sep 19, 2022

View reviewed changes

sphinx_copybutton/_static/copybutton.js_t Outdated Show resolved Hide resolved

sphinx_copybutton/_static/test.js Outdated Show resolved Hide resolved

rkdarst added 2 commits September 19, 2022 10:11

Update sphinx_copybutton/_static/copybutton.js_t

c2e0bb3

Update sphinx_copybutton/_static/test.js

c392d09

rkdarst changed the title ~~Exclude unselectable text from being copied (update)~~ ENH: Exclude unselectable text from being copied (update) Sep 29, 2022

choldgraf approved these changes Sep 29, 2022

View reviewed changes

choldgraf merged commit cd2440f into executablebooks:master Sep 29, 2022

rkdarst deleted the exclude-unselectable-3 branch September 30, 2022 06:32

rkdarst restored the exclude-unselectable-3 branch September 30, 2022 09:29

dhirajgagrai mentioned this pull request Oct 25, 2022

Copy button on docs page copies line numbers along with code python-websockets/websockets#1251

Closed

rkdarst mentioned this pull request Nov 17, 2022

Conflict between regex exclusion/prompt selection and copybutton_exclude #185

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Exclude unselectable text from being copied (update) #178

ENH: Exclude unselectable text from being copied (update) #178

rkdarst commented Sep 13, 2022

choldgraf commented Sep 13, 2022

rkdarst commented Sep 17, 2022 •

edited

rkdarst commented Sep 17, 2022

choldgraf left a comment

rkdarst commented Sep 18, 2022 via email

bicarlsen Sep 19, 2022

bicarlsen left a comment •

edited

bicarlsen commented Sep 19, 2022 •

edited

rkdarst commented Sep 19, 2022

rkdarst commented Sep 19, 2022

bicarlsen commented Sep 19, 2022

rkdarst commented Sep 29, 2022

choldgraf commented Sep 29, 2022

rkdarst commented Sep 29, 2022

choldgraf commented Sep 29, 2022

rkdarst commented Sep 29, 2022

choldgraf commented Sep 29, 2022

hypergravity commented Oct 9, 2022 •

edited

choldgraf commented Oct 9, 2022

hypergravity commented Oct 9, 2022

choldgraf commented Oct 9, 2022

hypergravity commented Oct 9, 2022

rkdarst commented Oct 11, 2022 via email

hassec commented Nov 17, 2022 •

edited

rkdarst commented Nov 17, 2022

hassec commented Nov 17, 2022 •

edited

choldgraf commented Nov 17, 2022

rkdarst commented Nov 17, 2022 •

edited

flywire commented Jan 13, 2023

ENH: Exclude unselectable text from being copied (update) #178

ENH: Exclude unselectable text from being copied (update) #178

Conversation

rkdarst commented Sep 13, 2022

choldgraf commented Sep 13, 2022

rkdarst commented Sep 17, 2022 • edited

rkdarst commented Sep 17, 2022

choldgraf left a comment

Choose a reason for hiding this comment

rkdarst commented Sep 18, 2022 via email

bicarlsen Sep 19, 2022

Choose a reason for hiding this comment

bicarlsen left a comment • edited

Choose a reason for hiding this comment

bicarlsen commented Sep 19, 2022 • edited

rkdarst commented Sep 19, 2022

rkdarst commented Sep 19, 2022

bicarlsen commented Sep 19, 2022

rkdarst commented Sep 29, 2022

choldgraf commented Sep 29, 2022

rkdarst commented Sep 29, 2022

choldgraf commented Sep 29, 2022

rkdarst commented Sep 29, 2022

choldgraf commented Sep 29, 2022

hypergravity commented Oct 9, 2022 • edited

choldgraf commented Oct 9, 2022

hypergravity commented Oct 9, 2022

choldgraf commented Oct 9, 2022

hypergravity commented Oct 9, 2022

rkdarst commented Oct 11, 2022 via email

hassec commented Nov 17, 2022 • edited

rkdarst commented Nov 17, 2022

hassec commented Nov 17, 2022 • edited

choldgraf commented Nov 17, 2022

rkdarst commented Nov 17, 2022 • edited

flywire commented Jan 13, 2023

rkdarst commented Sep 17, 2022 •

edited

bicarlsen left a comment •

edited

bicarlsen commented Sep 19, 2022 •

edited

hypergravity commented Oct 9, 2022 •

edited

hassec commented Nov 17, 2022 •

edited

hassec commented Nov 17, 2022 •

edited

rkdarst commented Nov 17, 2022 •

edited