Add wrapping to long diff lines to fix #1827 #1954

mjwwit · 2017-06-12T16:23:16Z

This PR adds line wrapping to the unified diff view (for long lines) (#1827). Wrapping was already done in the split view.
An additional change has been made to align the line-numbers of wrapped diff lines to the top of the line.

EDIT: Closes #1827

bkcsoft · 2017-06-12T16:40:10Z

Maybe have a checkbox toggle? For those that don't want wrapping :)

~~Also Closes #1827~~

mjwwit · 2017-06-12T16:47:32Z

The wrapping was already default behavior for the split view, are you sure you want a setting for this?
If you do want a setting, where should it be located? (we don't really have a user preferences view)
Also, should it store the preference server-side or simply keep it client-side?

ethantkoenig · 2017-06-12T17:21:24Z

public/css/index.css

@@ -2063,6 +2063,7 @@ footer .ui.language .menu {
  color: #A7A7A7;
  background: #fafafa;
  width: 1%;
+  vertical-align: top;


Please don't edit index.css directly, instead edit the appropriate .less file

Ugh... I hate generated code in VCS... I'll fix it though, good spot!

silverwind · 2017-06-12T18:01:12Z

If you do want a setting, where should it be located?

Maybe beside 'View File' in the header?

BTW I also noticed an error in the .wrap CSS: The current break-word value for word-break is invalid. It should be break-all. I recommend these rules for it:

white-space: pre-wrap;
word-break: break-all;
overflow-wrap: break-word;

Oh, and I also think this change should apply to regular code view, not just diffs.

mjwwit · 2017-06-12T20:11:41Z

I assumed the wrap class would have a proper implementation, as it turns out I was wrong. I have to keep the non-standard word-break: break-word; as well though, as it's needed for WebKit to function properly.

I'm not sure I agree with applying this to normal code views as well. And if we decide to do this, I'd do so using a user preference.

silverwind · 2017-06-12T20:37:33Z

Looks like word-break: break-word is superceded by overflow-wrap: break-word, so these two should be equivalent. I can at least say that the former isn't recognized by Firefox any more:

Thought, from the linked description, it sounds that break-all is what we want, and overflow-wrap is unnecessary.

mjwwit · 2017-06-12T20:52:27Z

Interesting, reading the W3C spec leads me to believe overflow-wrap can be used in in addition to word-break. I don't think one is superseded by the other.

word-break: break-word; should only be used (and will only be accepted) by WebKit. Other browsers correctly implemented the standard word-break: break-all; value.

silverwind · 2017-06-12T20:59:55Z

Both are valid in WebKit:

The spec seems pretty clear to me. word-wrap does not list it as a valid property value and overflow-wrap contains this note:

For legacy reasons, UAs must treat ‘word-wrap’ as an alternate name for the ‘overflow-wrap’ property, as if it were a shorthand of ‘overflow-wrap’.

lafriks · 2017-06-12T21:34:25Z

public/less/_base.less

 		word-break: break-word;
+
+		/* These are technically the same, but use both */
+  	overflow-wrap: break-word;


Something seems to be wrong with tabs

mjwwit · 2017-06-13T17:26:38Z

The spec seems pretty clear to me. word-wrap does not list it as a valid property value and overflow-wrap...

But we're talking about word-break and overflow-wrap, not word-wrap.

lunny · 2017-06-14T00:45:04Z

need @lafriks approval.

silverwind · 2017-06-14T09:57:53Z

But we're talking about word-break and overflow-wrap, not word-wrap.

Yeah, my error, I meant word-break. Anyways, I think you should try my suggestion in #1954 (comment), it should be all that's needed to achieve consistent cross-browser wrapping.

lafriks

When tabs are fixed than LG-TM

lafriks · 2017-06-14T22:14:09Z

public/less/_repository.less

-            td.message .isSigned {
-                cursor: default;
-            }
+						td.message .isSigned {


Too much tabs

lafriks · 2017-06-14T22:14:28Z

public/less/_repository.less

-            .jumpable-path {
-                color: #888;
-            }
+						.jumpable-path {


Too much tabs

lafriks · 2017-06-14T22:14:51Z

public/less/_repository.less

@@ -678,7 +678,7 @@
 			}
 			textarea {
 				height: 200px;
-                font-family: "Consolas", monospace;
+								font-family: "Consolas", monospace;


Too much tabs

lafriks · 2017-06-14T22:15:04Z

public/less/_repository.less

-        td.sha .sha.label {
-            margin: 0;
-        }
+				td.sha .sha.label {


Too much tabs

lafriks · 2017-06-14T22:16:51Z

public/less/_repository.less

-            }
-        }
-    }
+		#commits-table td.sha .sha.label, #repo-files-table .sha.label{


Too much tabs in all levels

lafriks · 2017-06-14T22:17:25Z

public/less/_repository.less

-        &:not(.positive):last-child {
-            border-bottom: 1px solid #A3C293;
-        }
+				&:not(.positive){


Too much tabs

lafriks · 2017-06-14T22:17:41Z

public/less/_repository.less

@@ -1304,7 +1305,7 @@
 }

 .issue-actions {
-    display: none;
+		display: none;


One tab too much :)

mjwwit · 2017-07-04T08:19:29Z

This whole formatting problem is caused by the .editorconfig file stating that indentation should be done using 4 spaces, but the less files are using tabs. Which should it be?

mjwwit · 2017-07-05T10:42:32Z

Lack of response leads me to believe nobody really cares if indentation is done using spaces or tabs, so I'll go with 4 spaces.

silverwind · 2017-07-05T16:07:41Z

Getting back to the topic of having it also apply on regular code views: Why should it only wrap in diffs? If you have a long line in a diff, it'll be a long line in the regular code view. I'd rather have both wrap for consistence.

Of course, a checkbox toggle in the header would be ideal, but that can come later, imho.

mjwwit · 2017-07-05T18:47:01Z

This may just be me, but I generally hate wrapping in code. The only time I do find it useful is when I need to see the difference between two pieces of code in a single glance. The wrapping is especially useful in a side-by-side diff, where the horizontal space is extra limited. I'm not against implementing wrapping in normal code-views as well, but I'd rather build the toggle setting with it so I can turn it off. Would you be willing to wait for the normal code view wrapping until that time?

lafriks · 2017-07-05T19:25:31Z

@mjwwit in code view and diff view code wrapping is needed as otherwise if code block is larger than screen you have to scroll to the bottom of code block to be able to scroll to right

silverwind · 2017-07-06T15:36:17Z

I'm fine if you want to do it later with the toggle. Having it in diffs is already an improvement. Side-by-side diffs already wrap without this patch for me, by the way.

Also agree with @lafriks, horizontal scroll on big files is generally a bad UX, but there are options like using the arrow keys to scroll.

lafriks · 2017-07-06T17:07:51Z

I vote for 4 spaces for tabbing just wanted more opinion from other Gitea developers. If other are also OK with that than it is LG-TM

bkcsoft · 2017-07-06T17:53:08Z

@lafriks LG-TM on 4 space. Haven't reviewed this one though so no real LG-TM from me ;)

lunny · 2017-09-15T15:10:19Z

hi, please rebase.

lafriks · 2017-10-25T20:38:02Z

Rebased and tested, LGTM

codecov-io · 2017-10-25T20:43:36Z

Codecov Report

Merging #1954 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1954   +/-   ##
=======================================
  Coverage   27.08%   27.08%           
=======================================
  Files          88       88           
  Lines       17264    17264           
=======================================
  Hits         4676     4676           
  Misses      11909    11909           
  Partials      679      679

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e86a0bf...8c8c163. Read the comment docs.

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 12, 2017

ethantkoenig suggested changes Jun 12, 2017

View reviewed changes

lafriks requested changes Jun 12, 2017

View reviewed changes

lunny added this to the 1.2.0 milestone Jun 13, 2017

ethantkoenig approved these changes Jun 13, 2017

View reviewed changes

lafriks approved these changes Jun 14, 2017

View reviewed changes

lafriks requested changes Jun 14, 2017

View reviewed changes

lunny modified the milestones: 1.3.0, 1.2.0 Jun 28, 2017

silverwind approved these changes Jul 6, 2017

View reviewed changes

lafriks approved these changes Jul 6, 2017

View reviewed changes

lafriks added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels Oct 25, 2017

tboerger 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 Oct 25, 2017

lafriks closed this Oct 26, 2017

lafriks mentioned this pull request Oct 26, 2017

Add wrapping to long diff lines #2789

Merged

lunny removed this from the 1.3.0 milestone Oct 27, 2017

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Uh oh!

Add wrapping to long diff lines to fix #1827 #1954

Add wrapping to long diff lines to fix #1827 #1954

Uh oh!

Conversation

mjwwit commented Jun 12, 2017 • edited by bkcsoft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkcsoft commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjwwit commented Jun 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silverwind commented Jun 12, 2017

Uh oh!

mjwwit commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjwwit commented Jun 12, 2017

Uh oh!

silverwind commented Jun 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjwwit commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lunny commented Jun 14, 2017

Uh oh!

silverwind commented Jun 14, 2017

Uh oh!

lafriks left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjwwit commented Jul 4, 2017

Uh oh!

mjwwit commented Jul 5, 2017

Uh oh!

silverwind commented Jul 5, 2017

Uh oh!

mjwwit commented Jul 5, 2017

Uh oh!

lafriks commented Jul 5, 2017

Uh oh!

silverwind commented Jul 6, 2017

Uh oh!

lafriks commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bkcsoft commented Jul 6, 2017

Uh oh!

lunny commented Sep 15, 2017

Uh oh!

lafriks commented Oct 25, 2017

Uh oh!

codecov-io commented Oct 25, 2017

Codecov Report

Uh oh!

mjwwit commented Jun 12, 2017 •

edited by bkcsoft

Loading

bkcsoft commented Jun 12, 2017 •

edited

Loading

mjwwit commented Jun 12, 2017 •

edited

Loading

silverwind commented Jun 12, 2017 •

edited

Loading

mjwwit commented Jun 13, 2017 •

edited

Loading

lafriks left a comment •

edited

Loading

lafriks commented Jul 6, 2017 •

edited

Loading