Remove final newline from rendered code #18076
Conversation
|
If you do not want to render the final line you need to add a character to mark when there is not a terminal newline. The lack of the terminal newline when copying from github is a bug on their behalf IMO. |
GiteaBot
added
the
lgtm/need 2
Such marking is only present on diffs on GH, and I agree we should probably replicate but I don't see it related here.
It feels awkward to me to copy something that isn't visible. What we could do instead is have a button in the file header to copy the file contents, which then would accurately copy with or without final newline. Personally, I prefer to copy without final newlines in any case. |
|
Oh I just noticed it depends on how the text selection is started on whether the final newline (which is still present on the last line) is copied in Chrome: On line-triple-click it is selected (notice area on the right of the text):
On multi-line-select it won't be selected:
Behaviour again matches GH, I think it's sufficient. Later on we can add a full-file copy button to work around such browser issues. |
|
Github's non rendering of the final line is inconsistent and confusing. View
Edit
Local editor
|
|
What's the purpose of the View screen? It's supposed to look like the editor. Therefore the terminal new line should be there. |
|
While editing, I think it's important to have a faithful representation but while viewing I think the final newline is not important. While copying, I also don't like having it there because I usually have to delete it after pasting. |
GiteaBot
added
lgtm/need 1
|
Assume two files with almost same content, but one end with newline, and another with no newline at the end. This PR will make the the same result in the UI. But in fact, they have difference. |
|
vote for #18076 (comment) |
|
indicate it in a different way is also fine - but this pull is not ready for v1.16.0 ... - moved |
Doesn't gitea show a error symbol when there is no new line at the end of the file? That should be the difference. |
We don't have such a feature yet. GH has it on diffs only. For example a PR that removes the newline: https://try.gitea.io/silverwind/symlink-test/pulls/2/files I guess it might be possible to add a subtle indicator to the final line if there is now newline in the file view, but I'm not sure many people really care about such meaningless whitespace to warrant even having it. |
I meant that such symbol should be the difference for diffs, as I've personally worked with certain files that required new lines and for code-review it's nice to have such feature. But for normal file viewing, the final newline is useless. So from my understanding now, the diff already don't show if the newline? In that case you can disregard my comment as that's my only concern. |
|
my vote is for not trimming the final newline from display |
|
From a correctness standpoint, it's better to not trim it. But I would still prefer it because I'm just so used to GitHub's trimming on code view. Maybe it has to be made an user option. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions. |
|
Should probably make this an config option. |
|
Added a config option, defaulting to final newline being hidden as that's how GitHub, GitLab and raw view in browser display it so I think most users would expect that behaviour. |
| finalNewLine := false | ||
| if len(code) > 0 { | ||
| if !setting.UI.HideFinalNewline && len(code) > 0 { | ||
| finalNewLine = code[len(code)-1] == '\n' | ||
| } |
There was a problem hiding this comment.
a) these four lines can be shortened to a single line because we are only using booleans here (GitHub doesn't let me make a suggestion for some reason...)
b) I think a comment would be nice, that method is already large enough and could use some structuring so that you don't become confused on what has happened already, and what has yet to happen.
There was a problem hiding this comment.
What would that single line be? It's not obvious to me
There was a problem hiding this comment.
finalNewLine := !setting.UI.HideFinalNewline && len(code) > 0 && code[len(code)-1] == '\n'| `<span class="w"> | ||
| </span>`, |
There was a problem hiding this comment.
Should we also add a test where we test for the behavior if there is a newline, and HIDE_FINAL_NEWLINE = false?
There was a problem hiding this comment.
I'm not sure whether these kind of tests should be extended because the output of go test is a major pain to work with in case of string arrays. It took me like 10 minutes to figure out what to change to get the test to pass.
Maybe if it were to test a single multiline detented string, it would be easier to work with. I can check later what can be done to make these tests easier to work with.
There was a problem hiding this comment.
Some test improvments in #19798, so I will revisit here after that one is merged.
I'm fine with that - but I would by default make it false |
Issues like #19766 show users generally expect the GH/GL-style rendering. Trailing newlines are almost never significant, why show them by default? |
|
Waiting on #19798 to merge before continuing here. |
|
Please resolve the conflicts |
|
I would also argue to default to false, for all previously mentioned reasons. Currently, it seems like this is only a server-wide setting. As you might imagine, just like the preferences of the Gitea maintainers in this thread differ, so they might differ among people using the same instance. So I would suggest adding a toggle in the personal preferences (maybe under mygitea.com/user/settings/appearance). Perhaps in the future, Gitea might add more customization options). |
|
I think the behavior is stable and clear on try.gitea.io now, and maybe no need to introduce more options. See https://try.gitea.io/wxiaoguang/test/src/branch/master/test-line-0.txt https://try.gitea.io/wxiaoguang/test/src/branch/master/test-line-1.txt https://try.gitea.io/wxiaoguang/test/src/branch/master/test-line-2-empty.txt https://try.gitea.io/wxiaoguang/test/src/branch/master/test-line-2.txt |
GiteaBot
added
the
lgtm/need 2
|
ping @silverwind , is this still needed? |
|
For me, I'm happy with current behaviour of removing final newline, but I guess this PR still has value in adding the option so people like @zeripath can configure to show it again. But I will reject changing the default to show it. |
|
Current approach is clear for files with or without the last new line. Just one line without last new line: 1 line: With last new line: 2 lines: In the future, if some people really want to see the "No New Line" difference, the files without last new line could have a non-copyable icon at the ending: 🚫 but no more configurable option again:
I would prefer to avoid unnecessary options as much as possible. So if there is no strong requirements (in 1 week), I think we can close this PR. |
wxiaoguang
added
issue/needs-feedback
|
Yeah, let's close it. 🚫 in diffs would indeed be a nice feature to have. |
#16678 introduced rendering of the final newline in files but I feel this rendering is not useful and just distracting. Omit rendering this empty line which matches GH behaviour in both rendering and copied content (no final newline copied, tested in Chrome and Firefox).
Before:
After:
