Remove the last newline from the copied text

[tzach]

Adding a newline at the end of the copied content is a bad UX when pasting to a Bash prompt (for example) as it runs the command before the user can edit it.

This PR removes the new line from the end of the content.

Fix #56

[choldgraf]

Ah shoot - I had made this comment a week ago but apparently I didn't hit the button to finalize the review. I had one quick thought on the right function to use here, WDYT?

[choldgraf]

Suggested change

	return textContent.slice(0, -1).join('\n') + textContent.slice(-1)
	textContent = textContent.join('\n')
	// Remove a trailing newline to avoid auto-running when pasting
	if (textContent.endsWith("\n")) {
	textContent = textContent.slice(0, -1)
	}
	return textContent

I looked into this a bit, and it seems like there's actually a javascript function to do just this (strip whitespace and line breaks). Can you try this suggestion and see if it works as well? https://stackoverflow.com/questions/14572413/remove-line-breaks-from-start-and-end-of-string/14572494

(using "trim" would remove line breaks from both beginning and end, but I think our issue is only with the end string)

[mgeier]

I don't think trimRight() should be used!

It trims all whitespace characters, while it should only trim one newline character.

One example where this would do the wrong thing would be the first few cells in https://nbsphinx.readthedocs.io/en/0.5.1/code-cells.html.

A probably better solution would be to remove a single character from the original string textContent.

It looks like the method textContent() always adds a newline character (and the current code in this PR assumes this), but I don't know if that's in any way guaranteed by the "language" (i.e. JavaScript).

I guess the most reasonable implementation would be to check whether the last character of textContent is \n, and if yes, remove it (by slicing?).

[choldgraf]

hmmm, good point. So then something like:

text = textContent.join('\n')
if (text.endsWith('\n')) {
    text = test.slice(0, -1);
}
return text

?

(note that I believe in Javascript the \n becomes a single character, thus the -1)

another option would be before the join statement, to run:

if (text[text.length - 1].length == 0) {
   text.pop(text.length - 1)
}
return textContent.join('\n')

or maybe even more robust in case there are multiple empty lines at the end:

while (text[text.length - 1].length == 0) {
    text.pop(text.length - 1)
}
return textContent.join('\n')

[mgeier]

I think I would even do it before the .split('\n'), but I guess it doesn't make a lot of difference.

Doing it on the array of strings should also be fine, however in JavaScript this just looks insane (even after removing the unnecessary argument to pop()).

This would be a different (not less ugly) way to do it:

if (text.slice(-1)[0] === '') {
    text.pop();
}

And please don't remove multiple empty lines! That was the whole point of my comment here!

[choldgraf]

ah ok - I updated my suggested change with your suggestion, I think it's cleaner. WDYT?

[mgeier]

What I said in https://github.com/choldgraf/sphinx-copybutton/pull/57#discussion_r372896823:

I think I would even do it before the .split('\n'), but I guess it doesn't make a lot of difference.

If you prefer it after the .join(), the current version looks good!

[choldgraf]

@tzach could you accept this change so we can get it into master?

[choldgraf]

since your branch doesn't let me commit to it, I applied your commit in #58 and then also added the suggested patch here to that PR...so if that one gets merged it'll supercede this PR. Just FYI

[tzach]

Hi @choldgraf
Feel free to close this PR.
Thanks for pushing the project and this fix forward!

[choldgraf]

sounds good, will do so!