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

Fix copy/paste of empty lines #19798

Merged
merged 10 commits into from
Jun 10, 2022
Merged

Fix copy/paste of empty lines #19798

merged 10 commits into from
Jun 10, 2022

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented May 24, 2022

Fixes: #19331
Regressed by: #18270

Needed to do another string replacement in the Chroma output HTML to get copy/paste work again. The previous replacement conditions are probably obsolete, but as I'm not 100% sure, I opted to keep them.

Specifically, the Chroma HTML change mentioned in #18270 (comment) broke our previous newline replacement for such empty lines.

Also included are a few changes to make the test more pleasant to work with.

Fixes: go-gitea#19331
Regressed by: go-gitea#18270

Needed to do another newline addition to the Chroma output HTML to get
copy/paste work again. The previous replacement conditions are probably
obsolete, but as I'm not 100% sure, I opted to keep them.

Specifically, the Chroma HTML change mentioned in
go-gitea#18270 (comment)
broke our previous newline replacement for such empty lines.

Also included are a few changes to make the test more pleasant to work
with.
@silverwind silverwind changed the title Fix copy/paste of empty newlines again Fix copy/paste of empty lines May 24, 2022
go.mod Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 24, 2022
@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 May 25, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 25, 2022

The dedent.Dedent could be replaced by a regex like ^\s*(.*?)\s*$ => $1 , then the dependency can be removed.

func main() {
	m := regexp.MustCompile(`(?m:^\s*(.*?)\s*$)`)   // use multi-line mode to trim spaces
	s := "  a  \n\tb c  \n"
	t := m.ReplaceAllString(s, "$1")
	fmt.Printf("origin:\n%s\n--------\ntrimed:\n%s\n(len=%d)\n", s, t, len(t))
}
origin:
  a  
        b c  

--------
trimed:
a
b c
(len=5)

@silverwind
Copy link
Member Author

The dedent.Dedent could be replaced by a regex like ^\s*(.?)\s$ => $1 , then the dependency can be removed.

Might be an option, yeah. One thing I don't like about the dependency is that it does not handle leading/trailing newlines, I mentioned it in lithammer/dedent#14.

@silverwind
Copy link
Member Author

The dedent.Dedent could be replaced by a regex like ^\s*(.*?)\s*$ => $1 , then the dependency can be removed.

func main() {
	m := regexp.MustCompile(`(?m:^\s*(.*?)\s*$)`)   // use multi-line mode to trim spaces
	s := "  a  \n\tb c  \n"
	t := m.ReplaceAllString(s, "$1")
	fmt.Printf("origin:\n%s\n--------\ntrimed:\n%s\n(len=%d)\n", s, t, len(t))
}
origin:
  a  
        b c  

--------
trimed:
a
b c
(len=5)

Actually, this is not a true dedent because b should not indent to 0 but only be dedented by 2 places. I think it's not possible with a regex alone to achieve a proper detent so I think the module is fine to keep, but I'll add a wrapper function that also performs strings.TrimSpace on the result because that's what I would expect from a proper dedent.

@wxiaoguang
Copy link
Contributor

Oh yup, I was thinking that the spaces should have been wrapped inside tags, so all spaces around the line could be trimmed. If it's not the case, then regex won't work.

@silverwind
Copy link
Member Author

Refactored to util.Dedent that also does the trimming. I think it will be useful in the future, especially for test code.

modules/util/util.go Outdated Show resolved Hide resolved
@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 May 26, 2022
@silverwind
Copy link
Member Author

ready to merge from my side.

@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Jun 10, 2022
@wxiaoguang
Copy link
Contributor

I think this could be in 1.17 (keep it open for one or two days for more suggestions)

@silverwind
Copy link
Member Author

I'd even backport it to 1.16. I copy a lot of code from UI and it's a big issue for me.

@lunny lunny merged commit 527e5bd into go-gitea:main Jun 10, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 11, 2022
* giteaoffical/main:
  Fix data-race problems in git module (quick patch) (go-gitea#19934)
  [skip ci] Updated translations via Crowdin
  Fix copy/paste of empty lines (go-gitea#19798)
  Normalize line endings in fomantic build files (go-gitea#19932)
  Make user profile image show full image on mobile (go-gitea#19840)
  Custom regexp external issues (go-gitea#17624)
  Use Golang 1.18 for Gitea 1.17 release (go-gitea#19918)
  Refactor git module, make Gitea use internal git config (go-gitea#19732)
  [skip ci] Updated translations via Crowdin
@silverwind silverwind deleted the copy branch June 14, 2022 13:10
silverwind added a commit to silverwind/gitea that referenced this pull request Jun 14, 2022
Followup to go-gitea#19798. Trim off both the .line and .cl classes from
Chroma's HTML which made the old conditional work again. This fixed Copy
of YAML files for me.
silverwind added a commit to silverwind/gitea that referenced this pull request Jun 14, 2022
Fixes: go-gitea#19331

Followup to go-gitea#19798. Trim off both the .line and .cl classes from Chroma's HTML which made the old conditional work again. This fixed Copy of YAML files for me while also reducing the amount of rendered HTML nodes.
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
* Fix copy/paste of empty newlines again

Fixes: go-gitea#19331
Regressed by: go-gitea#18270

Needed to do another newline addition to the Chroma output HTML to get
copy/paste work again. The previous replacement conditions are probably
obsolete, but as I'm not 100% sure, I opted to keep them.

Specifically, the Chroma HTML change mentioned in
go-gitea#18270 (comment)
broke our previous newline replacement for such empty lines.

Also included are a few changes to make the test more pleasant to work
with.

* run go mod tidy

* add util.Dedent

* copy in the code

Co-authored-by: techknowlogick <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
Co-authored-by: Lauris BH <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
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. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy/Paste ommitting newlines (again)
7 participants