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.

Sign up for GitHub

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

Jump to bottom

Support CRLF when splitting code lines for display #1862

Merged
merged 5 commits into from
Jun 10, 2017
Merged

Support CRLF when splitting code lines for display #1862

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b677b69
Support CRLF when splitting code lines for display
silverwind Jun 7, 2017
8e8385a
refactor, fix mixed match
silverwind Jun 7, 2017
00763ed
fmt
silverwind Jun 8, 2017
3c3c8eb
split on both LF and CRLF, use raw literals in regexes
silverwind Jun 8, 2017
623e1a3
simplify
silverwind Jun 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion public/css/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1435,7 +1435,7 @@ footer .ui.language .menu {
.repository.file.list #file-content .code-view .lines-code ol li,
.repository.file.list #file-content .code-view .lines-num .hljs li,
.repository.file.list #file-content .code-view .lines-code .hljs li {
display: inline-block;
display: block;
width: 100%;
}
.repository.file.list #file-content .code-view .lines-num pre li.active,
Expand Down
2 changes: 1 addition & 1 deletion public/less/_repository.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@
margin: 0;
padding: 0 !important;
li {
display: inline-block;
display: block;
width: 100%;
&.active {
background: #ffffdd;
Expand Down
22 changes: 20 additions & 2 deletions routers/repo/view.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
gotemplate "html/template"
"io/ioutil"
"path"
"regexp"
"strconv"
"strings"

Expand Down Expand Up @@ -210,9 +211,26 @@ func renderFile(ctx *context.Context, entry *git.TreeEntry, treeLink, rawLink st
}

var output bytes.Buffer
lines := strings.Split(fileContent, "\n")
var terminator string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By using a single terminator, we aren't able to correctly render files that sometime use \r\n and in other places use \n? Is there a reason we can't just do

lines := strings.Split(fileContent, "\n")
for index, line := range lines {
	if index < len(lines) - 1 {
		line += "\n"
	}
	output.WriteString(fmt.Sprintf(`<li class="L%d" rel="L%d">%s</li>`, index+1, index+1, line))
}

Since both \r\n and \n end in \n, it seems to me this would work, and it would handle the case where a file mixes \r\n and \n

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, yes, this looks to work. My first approach was similar to this one, but for some reason I stopped pursuing it. Anyways, this copies fine, preserves newlines in mixed files and also highlights fine, looking good!


if strings.Index(fileContent, "\r\n") != -1 {
lf, _ := regexp.MatchString("[^\r]\n", fileContent)
if lf {
terminator = "\n"
} else {
terminator = "\r\n"
}
} else {
terminator = "\n"
}
lines := strings.Split(fileContent, terminator)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To deal with mixed newlines it would be better to split using:
lines := regexp.MustCompile("\\r?\\n").Split(fileContent, -1)

Copy link
Member Author

@silverwind silverwind Jun 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but how do I know whether to join the lines with LF or CRLF afterwards in the loop? I'd like to preserve CRLF if it's there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave detection code, just replace strings split with regexp split

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, actually I'll try your suggestion. I can keep the detection code above to know.


for index, line := range lines {
output.WriteString(fmt.Sprintf(`<li class="L%d" rel="L%d">%s</li>`, index+1, index+1, gotemplate.HTMLEscapeString(line)) + "\n")
line = gotemplate.HTMLEscapeString(line)
if index != len(lines) - 1 {
line += terminator
}
output.WriteString(fmt.Sprintf(`<li class="L%d" rel="L%d">%s</li>`, index+1, index+1, line))
}
ctx.Data["FileContent"] = gotemplate.HTML(output.String())

Expand Down