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

Make copy/paste work for source code #12191

Merged
merged 5 commits into from
Jul 8, 2020
Merged

Conversation

mrsdizzie
Copy link
Member

@mrsdizzie mrsdizzie commented Jul 8, 2020

Fix regression casued by #12047 so copy/paste works properly in all browsers.

Fixes #12184

Also while looking at this I saw a small display issue for blame view. I think #12023 was merged into original PR through an update branch before #12047 was merged and made one of the css rules not apply anymore.

Also fix existing and unrelated bug where copy/paste from PR diff would select the + symbol from add comment features

Fix regression casued by go-gitea#12047 so copy/paste works properly in all browsers.

Fixes go-gitea#12184

Also while looking at this I saw a small display issue for blame view. I think go-gitea#12023 was merged into original PR through an update branch before go-gitea#12047 was merged and made one of the css ruules not apply anymore.
@silverwind
Copy link
Member

Looking pretty good now. One thing I've noticed it that copying two or more lines out of a diff appends a trailing newline in both Firefox and Chrome. It does not do that on the file view, and I think that newline should ideally not be copied.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 8, 2020
@mrsdizzie
Copy link
Member Author

@silverwind will take a look

@mrsdizzie
Copy link
Member Author

Actually can't reproduce the diff issue -- can you link to an example to copy?

@silverwind
Copy link
Member

Any diff like https://try.gitea.io/silverwind/updates/commit/7c5b7c608d95fa85644c05d9a33355511a3ae492

Select at least two lines, copy them and paste them into an editor and you'll notice the cursor being on the line after the two lines.

image

@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

Can't reproduce on this PR @silverwind

@mrsdizzie Copying from PR diff view where you have [+] buttons for code comment still copies the '+' character from it on every line. This causes Firefox to place it in separate line while for Chromium it's placed on same line, before actual content.

Chromium:

## New features in version 2
+
+- 500+ new supported file types
+- Batch mode
+- Scan only mode to determine file types without extracting
+- Scan only mode to detect the type of any given file
+- Built-in updater
+- 100+ new supported file types

Firefox:

## New features in version 2
+

+
- 500+ new supported file types
+
- Batch mode
+
- Scan only mode to determine file types without extracting
+
- Scan only mode to detect the type of any given file
+
- Built-in updater
+
- 100+ new supported file types

Desired output:

## New features in version 2

- 500+ new supported file types
- Batch mode
- Scan only mode to determine file types without extracting
- Scan only mode to detect the type of any given file
- Built-in updater
- 100+ new supported file types

@mrsdizzie
Copy link
Member Author

@CirnoT ah OK -- appears that it has always been like this for PR diff rather than a regression from recent changes. I'll try and fix it for this PR too since it should just be similar to regular diff

@silverwind
Copy link
Member

Generally, adding user-select: none should make something like that [+] uncopyable.

@mrsdizzie
Copy link
Member Author

@CirnoT should be fixed for PR diff -- seems ok in Firefox/Chrome from testing.

@silverwind
Copy link
Member

silverwind commented Jul 8, 2020

Regarding the newline copying issue in diff, it seems to be because each line in the diff has a trailing newline which is not present on regular view lines. In regular file, there's no newline after the last chroma token (last <span>):

<code>  <span class="nt">"license"</span><span class="p">:</span> <span class="s2">"BSD-2-Clause"</span><span class="p">,</span></code>

but in diff view, there's a newline after it

<span class="mono wrap">  <span class="s2">"license"</span><span class="err">:</span> <span class="s2">"BSD-2-Clause"</span><span class="err">,</span>
</span>

Can we chop it off maybe?

@lafriks lafriks added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Jul 8, 2020
@lafriks lafriks added this to the 1.13.0 milestone Jul 8, 2020
@mrsdizzie
Copy link
Member Author

@silverwind I pushed a change to fix that if you can test -- I see the new lines in the source but don't experience the copy/paste issue so can't verify that it would fix it.

@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

@mrsdizzie It works fine now, thanks.

@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

Actually the newest commit broke newlines again, they're no longer present on copy.

@silverwind
Copy link
Member

silverwind commented Jul 8, 2020

Trailing newline is now fixed for me. @CirnoT which newlines exactly?

@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

Between lines obviously

@mrsdizzie
Copy link
Member Author

ah ok I see sheesh sorry, will fix next commit

@silverwind
Copy link
Member

I mean diff or code view, or both? It does seem to work fine here in Firefox 79 and Chrome 83.

@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

## New features in version 2
--
  |   |   |  
  |   |   | - 500+ new supported file types
  |   |   | - Batch mode
  |   |   | - Scan only mode to determine file types without extracting
  |   |   | - Scan only mode to detect the type of any given file
  |   |   | - Built-in updater
  |   |   | - 100+ new supported file types
  |   |   | - Audio and video extraction for multimedia files
  |   |   | - Cascading context menu
  |   |   | - Support for password list for common archives
  |   |   | - Improved optional status box with progress indicator
  |   |   | - Improved context menu integration and status box
  |   |   | - Better and faster file analysis
  |   |   | - Support for some extractors not shipping with UniExtract as plugins
  |   |   | - Silent mode, not showing any prompts
  |   |   | - Many interface improvements and redesigned dialogs
  |   |   | - Resource usage/speed improvements, lots of fixes
  |   |   | - Auto-using 64 bit versions of extractors if supported by OS
  |   |   | - Resource usage/speed improvements, lots of bug fixes

Is what I get when I copy this here on GH:
chrome_2020-07-08_21-56-13

@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

Copying it outside browser however produces this:

## New features in version 2
- 500+ new supported file types
- Batch mode
- Scan only mode to determine file types without extracting
- Scan only mode to detect the type of any given file
- Built-in updater
- 100+ new supported file types
- Audio and video extraction for multimedia files
- Cascading context menu
- Support for password list for common archives
- Improved optional status box with progress indicator
- Improved context menu integration and status box
- Better and faster file analysis
- Support for some extractors not shipping with UniExtract as plugins
- Silent mode, not showing any prompts
- Many interface improvements and redesigned dialogs
- Resource usage/speed improvements, lots of fixes
- Auto-using 64 bit versions of extractors if supported by OS
- Resource usage/speed improvements, lots of bug fixes

@silverwind
Copy link
Member

I don't understand. Where do you paste to get these vertical lines?

@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

Just here in GH textarea, I guess it adds it for some reason? Pasting it in raw <textarea> on simple HTML page results in

## New features in version 2
- 500+ new supported file types
- Batch mode
- Scan only mode to determine file types without extracting
- Scan only mode to detect the type of any given file
- Built-in updater
- 100+ new supported file types
- Audio and video extraction for multimedia files
- Cascading context menu
- Support for password list for common archives
- Improved optional status box with progress indicator
- Improved context menu integration and status box
- Better and faster file analysis
- Support for some extractors not shipping with UniExtract as plugins
- Silent mode, not showing any prompts
- Many interface improvements and redesigned dialogs
- Resource usage/speed improvements, lots of fixes
- Auto-using 64 bit versions of extractors if supported by OS
- Resource usage/speed improvements, lots of bug fixes

Note the missing empty newline

@silverwind
Copy link
Member

Indeed, I see very strange paste results when copy/pasting inside Firefox but it does work normally when I paste into an external editor, wtf.

@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

What OS if I can ask @silverwind. Neither me nor @mrsdizzie can't reproduce your original report about stray newline at the end?

@silverwind
Copy link
Member

Final newline issue was both in Windows and macOS, Firefox and Chrome. Thought in Chrome, not always.

@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

Must be some GH feature; try it on any other site that has simple unmodified <textarea> element.

@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

Windows here, could not reproduce on Chrome and Firefox alike.

@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

I can reproduce it If i try, by which I mean specifically try to move cursor so that it catches empty element on next line by ending drag selection on line below. I wouldn't consider that a bug however?

@silverwind
Copy link
Member

silverwind commented Jul 8, 2020

Yeah apparantly GH textarea does magic to break pasted text, nice. Try pasting into a pristine textarea like https://jsfiddle.net/silverwind/znpcLo4w/.

Not sure what else to tell you, I start selection on bottom right of the line, move cursor to top left, hit copy and then paste.

Current version is LGTM.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 8, 2020
@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

No, the current version does not preserve empty newlines on diff view

@mrsdizzie
Copy link
Member Author

@CirnoT I believe that last push should fix copying newline in diff again

@silverwind
Copy link
Member

Ah right, I can see the empty newline issue too. Testing latest fix.

@silverwind
Copy link
Member

Confirming everything working now for me.

@CirnoT
Copy link
Contributor

CirnoT commented Jul 8, 2020

Works fine now here too

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 8, 2020
@zeripath
Copy link
Contributor

zeripath commented Jul 8, 2020

Make lg-tm work

@zeripath zeripath merged commit a6168fa into go-gitea:master Jul 8, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copying text from source code view/diff view adds or removes newlines
6 participants