Improve diff file headers (#36215)

- reduce file name font size from 15px to 14px
- fix labels and buttons being cut off when their size is constrained
- change labels from monospace to sans-serif font
- move diff stats to right and change them from sum of changes to +/-
- change filemode to label and change text to match other labels

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

modules/git/parse.go

@@ -46,8 +46,8 @@ func parseLsTreeLine(line []byte) (*LsTreeEntry, error) {
 		entry.Size = optional.Some(size)
 	}
 
-	entry.EntryMode, err = ParseEntryMode(string(entryMode))
-	if err != nil || entry.EntryMode == EntryModeNoEntry {
+	entry.EntryMode = ParseEntryMode(string(entryMode))
+	if entry.EntryMode == EntryModeNoEntry {
 		return nil, fmt.Errorf("invalid ls-tree output (invalid mode): %q, err: %w", line, err)
 	}
 

modules/git/tree_entry_mode.go

@@ -4,7 +4,6 @@
 package git
 
 import (
-	"fmt"
 	"strconv"
 )
 
@@ -55,21 +54,38 @@ func (e EntryMode) IsExecutable() bool {
 	return e == EntryModeExec
 }
 
-func ParseEntryMode(mode string) (EntryMode, error) {
+func ParseEntryMode(mode string) EntryMode {
 	switch mode {
 	case "000000":
-		return EntryModeNoEntry, nil
+		return EntryModeNoEntry
 	case "100644":
-		return EntryModeBlob, nil
+		return EntryModeBlob
 	case "100755":
-		return EntryModeExec, nil
+		return EntryModeExec
 	case "120000":
-		return EntryModeSymlink, nil
+		return EntryModeSymlink
 	case "160000":
-		return EntryModeCommit, nil
-	case "040000", "040755": // git uses 040000 for tree object, but some users may get 040755 for unknown reasons
-		return EntryModeTree, nil
+		return EntryModeCommit
+	case "040000":
+		return EntryModeTree
 	default:
-		return 0, fmt.Errorf("unparsable entry mode: %s", mode)
+		// git uses 040000 for tree object, but some users may get 040755 from non-standard git implementations
+		m, _ := strconv.ParseInt(mode, 8, 32)
+		modeInt := EntryMode(m)
+		switch modeInt & 0o770000 {
+		case 0o040000:
+			return EntryModeTree
+		case 0o160000:
+			return EntryModeCommit
+		case 0o120000:
+			return EntryModeSymlink
+		case 0o100000:
+			if modeInt&0o777 == 0o755 {
+				return EntryModeExec
+			}
+			return EntryModeBlob
+		default:
+			return EntryModeNoEntry
+		}
 	}
 }

modules/git/tree_entry_test.go

@@ -27,3 +27,30 @@ func TestEntriesCustomSort(t *testing.T) {
 	entries.CustomSort(strings.Compare)
 	assert.Equal(t, expected, entries)
 }
+
+func TestParseEntryMode(t *testing.T) {
+	tests := []struct {
+		modeStr   string
+		expectMod EntryMode
+	}{
+		{"000000", EntryModeNoEntry},
+		{"000755", EntryModeNoEntry},
+
+		{"100644", EntryModeBlob},
+		{"100755", EntryModeExec},
+
+		{"120000", EntryModeSymlink},
+		{"120755", EntryModeSymlink},
+		{"160000", EntryModeCommit},
+		{"160755", EntryModeCommit},
+
+		{"040000", EntryModeTree},
+		{"040755", EntryModeTree},
+
+		{"777777", EntryModeNoEntry}, // invalid mode
+	}
+	for _, test := range tests {
+		mod := ParseEntryMode(test.modeStr)
+		assert.Equal(t, test.expectMod, mod, "modeStr: %s", test.modeStr)
+	}
+}

options/locale/locale_en-US.json

@@ -2542,8 +2542,8 @@
   "repo.diff.too_many_files": "Some files were not shown because too many files have changed in this diff",
   "repo.diff.show_more": "Show More",
   "repo.diff.load": "Load Diff",
-  "repo.diff.generated": "generated",
-  "repo.diff.vendored": "vendored",
+  "repo.diff.generated": "Generated",
+  "repo.diff.vendored": "Vendored",
   "repo.diff.comment.add_line_comment": "Add line comment",
   "repo.diff.comment.placeholder": "Leave a comment",
   "repo.diff.comment.add_single_comment": "Add single comment",
@@ -3724,8 +3724,8 @@
   "projects.exit_fullscreen": "Exit Fullscreen",
   "git.filemode.changed_filemode": "%[1]s → %[2]s",
   "git.filemode.directory": "Directory",
-  "git.filemode.normal_file": "Normal file",
-  "git.filemode.executable_file": "Executable file",
-  "git.filemode.symbolic_link": "Symbolic link",
+  "git.filemode.normal_file": "Regular",
+  "git.filemode.executable_file": "Executable",
+  "git.filemode.symbolic_link": "Symlink",
   "git.filemode.submodule": "Submodule"
 }

services/gitdiff/git_diff_tree.go

@@ -166,16 +166,6 @@ func parseGitDiffTreeLine(line string) (*DiffTreeRecord, error) {
 		return nil, fmt.Errorf("unparsable output for diff-tree --raw: `%s`, expected 5 space delimited values got %d)", line, len(fields))
 	}
 
-	baseMode, err := git.ParseEntryMode(fields[0])
-	if err != nil {
-		return nil, err
-	}
-
-	headMode, err := git.ParseEntryMode(fields[1])
-	if err != nil {
-		return nil, err
-	}
-
 	baseBlobID := fields[2]
 	headBlobID := fields[3]
 
@@ -201,8 +191,8 @@ func parseGitDiffTreeLine(line string) (*DiffTreeRecord, error) {
 	return &DiffTreeRecord{
 		Status:     status,
 		Score:      score,
-		BaseMode:   baseMode,
-		HeadMode:   headMode,
+		BaseMode:   git.ParseEntryMode(fields[0]),
+		HeadMode:   git.ParseEntryMode(fields[1]),
 		BaseBlobID: baseBlobID,
 		HeadBlobID: headBlobID,
 		BasePath:   basePath,

services/gitdiff/gitdiff.go

@@ -405,8 +405,8 @@ type DiffFile struct {
 	Addition     int
 	Deletion     int
 	Type         DiffFileType
-	Mode        string
-	OldMode     string
+	EntryMode    string
+	OldEntryMode string
 	IsCreated    bool
 	IsDeleted    bool
 	IsBin        bool
@@ -501,23 +501,38 @@ func (diffFile *DiffFile) ShouldBeHidden() bool {
 	return diffFile.IsGenerated || diffFile.IsViewed
 }
 
-func (diffFile *DiffFile) ModeTranslationKey(mode string) string {
-	switch mode {
-	case "040000":
-		return "git.filemode.directory"
-	case "100644":
-		return "git.filemode.normal_file"
-	case "100755":
-		return "git.filemode.executable_file"
-	case "120000":
-		return "git.filemode.symbolic_link"
-	case "160000":
-		return "git.filemode.submodule"
+func (diffFile *DiffFile) TranslateDiffEntryMode(locale translation.Locale) string {
+	entryModeTr := func(mode string) string {
+		entryMode := git.ParseEntryMode(mode)
+		switch {
+		case entryMode.IsDir():
+			return locale.TrString("git.filemode.directory")
+		case entryMode.IsRegular():
+			return locale.TrString("git.filemode.normal_file")
+		case entryMode.IsExecutable():
+			return locale.TrString("git.filemode.executable_file")
+		case entryMode.IsLink():
+			return locale.TrString("git.filemode.symbolic_link")
+		case entryMode.IsSubModule():
+			return locale.TrString("git.filemode.submodule")
 		default:
 			return mode
 		}
 	}
 
+	if diffFile.EntryMode != "" && diffFile.OldEntryMode != "" {
+		oldMode := entryModeTr(diffFile.OldEntryMode)
+		newMode := entryModeTr(diffFile.EntryMode)
+		return locale.TrString("git.filemode.changed_filemode", oldMode, newMode)
+	}
+	if diffFile.EntryMode != "" {
+		if entryMode := git.ParseEntryMode(diffFile.EntryMode); !entryMode.IsRegular() {
+			return entryModeTr(diffFile.EntryMode)
+		}
+	}
+	return ""
+}
+
 type limitByteWriter struct {
 	buf   bytes.Buffer
 	limit int
@@ -695,10 +710,10 @@ parsingLoop:
 				strings.HasPrefix(line, "new mode "):
 
 				if strings.HasPrefix(line, "old mode ") {
-					curFile.OldMode = prepareValue(line, "old mode ")
+					curFile.OldEntryMode = prepareValue(line, "old mode ")
 				}
 				if strings.HasPrefix(line, "new mode ") {
-					curFile.Mode = prepareValue(line, "new mode ")
+					curFile.EntryMode = prepareValue(line, "new mode ")
 				}
 				if strings.HasSuffix(line, " 160000\n") {
 					curFile.IsSubmodule, curFile.SubmoduleDiffInfo = true, &SubmoduleDiffInfo{}
@@ -733,7 +748,7 @@ parsingLoop:
 				curFile.Type = DiffFileAdd
 				curFile.IsCreated = true
 				if strings.HasPrefix(line, "new file mode ") {
-					curFile.Mode = prepareValue(line, "new file mode ")
+					curFile.EntryMode = prepareValue(line, "new file mode ")
 				}
 				if strings.HasSuffix(line, " 160000\n") {
 					curFile.IsSubmodule, curFile.SubmoduleDiffInfo = true, &SubmoduleDiffInfo{}

templates/repo/diff/box.tmpl

@@ -82,43 +82,42 @@
 					{{$isExpandable := or (gt $file.Addition 0) (gt $file.Deletion 0) $file.IsBin}}
 					{{$isReviewFile := and $.IsSigned $.PageIsPullFiles (not $.Repository.IsArchived) $.IsShowingAllCommits}}
 					<div class="diff-file-box file-content {{TabSizeClass $.Editorconfig $file.Name}} tw-mt-0" id="diff-{{$file.NameHash}}" data-old-filename="{{$file.OldName}}" data-new-filename="{{$file.Name}}" {{if or ($file.ShouldBeHidden) (not $isExpandable)}}data-folded="true"{{end}}>
-						<h4 class="diff-file-header sticky-2nd-row ui top attached header">
+						<div class="diff-file-header sticky-2nd-row ui top attached header">
 							<div class="diff-file-name tw-flex tw-flex-1 tw-items-center tw-gap-1 tw-flex-wrap">
-								<button class="fold-file btn interact-bg tw-p-1{{if not $isExpandable}} tw-invisible{{end}}">
+								<div class="flex-text-block">
+									<button class="fold-file btn interact-bg tw-flex-shrink-0 tw-p-1{{if not $isExpandable}} tw-invisible{{end}}">
 										{{if $file.ShouldBeHidden}}
 											{{svg "octicon-chevron-right" 18}}
 										{{else}}
 											{{svg "octicon-chevron-down" 18}}
 										{{end}}
 									</button>
-								<div class="tw-font-semibold tw-flex tw-items-center tw-font-mono">
-									{{if $file.IsBin}}
-										<span class="tw-ml-0.5 tw-mr-2">
-											{{ctx.Locale.Tr "repo.diff.bin"}}
-										</span>
-									{{else}}
-										{{template "repo/diff/stats" dict "file" . "root" $}}
-									{{end}}
+									{{$entryModeText := $file.TranslateDiffEntryMode ctx.Locale}}
+									<a class="muted file-link tw-font-mono" title="{{if $file.IsRenamed}}{{$file.OldName}} → {{end}}{{$file.Name}}" href="#diff-{{$file.NameHash}}">
+										{{if $file.IsRenamed}}{{$file.OldName}} → {{end}}{{$file.Name}}
+									</a>
 								</div>
-								<span class="file tw-flex tw-items-center tw-font-mono tw-flex-1"><a class="muted file-link" title="{{if $file.IsRenamed}}{{$file.OldName}} → {{end}}{{$file.Name}}" href="#diff-{{$file.NameHash}}">{{if $file.IsRenamed}}{{$file.OldName}} → {{end}}{{$file.Name}}</a>
-									<button class="btn interact-fg tw-p-2" data-clipboard-text="{{$file.Name}}" data-tooltip-content="{{ctx.Locale.Tr "copy_path"}}">{{svg "octicon-copy" 14}}</button>
-									{{if .IsLFSFile}}<span class="ui label">LFS</span>{{end}}
+								<button class="btn interact-fg tw-p-2 tw-shrink-0" data-clipboard-text="{{$file.Name}}" data-tooltip-content="{{ctx.Locale.Tr "copy_path"}}">{{svg "octicon-copy" 14}}</button>
+								{{if $file.IsLFSFile}}
+									<span class="ui label">LFS</span>
+								{{end}}
 								{{if $file.IsGenerated}}
 									<span class="ui label">{{ctx.Locale.Tr "repo.diff.generated"}}</span>
 								{{end}}
 								{{if $file.IsVendored}}
 									<span class="ui label">{{ctx.Locale.Tr "repo.diff.vendored"}}</span>
 								{{end}}
-									{{if and $file.Mode $file.OldMode}}
-										{{$old := ctx.Locale.Tr ($file.ModeTranslationKey $file.OldMode)}}
-										{{$new := ctx.Locale.Tr ($file.ModeTranslationKey $file.Mode)}}
-										<span class="tw-mx-2 tw-font-mono tw-whitespace-nowrap">{{ctx.Locale.Tr "git.filemode.changed_filemode" $old $new}}</span>
-									{{else if $file.Mode}}
-										<span class="tw-mx-2 tw-font-mono tw-whitespace-nowrap">{{ctx.Locale.Tr ($file.ModeTranslationKey $file.Mode)}}</span>
+								{{if $entryModeText}}
+									<span class="ui label">{{$entryModeText}}</span>
 								{{end}}
-								</span>
 							</div>
-							<div class="diff-file-header-actions tw-flex tw-items-center tw-gap-1 tw-flex-wrap">
+							<div class="diff-file-header-actions flex-text-block tw-justify-end tw-flex-wrap">
+								{{if $file.IsBin}}
+									{{ctx.Locale.Tr "repo.diff.bin"}}
+								{{else}}
+									{{template "repo/diff/stats" dict "Addition" .Addition "Deletion" .Deletion}}
+								{{end}}
+
 								{{if $showFileViewToggle}}
 									<div class="ui compact icon buttons">
 										<button class="ui tiny basic button file-view-toggle" data-toggle-selector="#diff-source-{{$file.NameHash}}" data-tooltip-content="{{ctx.Locale.Tr "repo.file_view_source"}}">{{svg "octicon-code"}}</button>
@@ -157,7 +156,7 @@
 									</div>
 								{{end}}
 							</div>
-						</h4>
+						</div>
 						<div class="diff-file-body ui attached unstackable table segment" {{if and $file.IsViewed $.IsShowingAllCommits}}data-folded="true"{{end}}>
 							<div id="diff-source-{{$file.NameHash}}" class="file-body file-code unicode-escaped code-diff{{if $.IsSplitStyle}} code-diff-split{{else}} code-diff-unified{{end}}{{if $showFileViewToggle}} tw-hidden{{end}}">
 								{{if or $file.IsIncomplete $file.IsBin}}

templates/repo/diff/stats.tmpl

@@ -1,5 +1,17 @@
-{{Eval .file.Addition "+" .file.Deletion}}
-<span class="diff-stats-bar tw-mx-2" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.stats_desc_file" (Eval .file.Addition "+" .file.Deletion) .file.Addition .file.Deletion}}">
-	{{/* if the denominator is zero, then the float result is "width: NaNpx", as before, it just works */}}
-	<div class="diff-stats-add-bar" style="width: {{Eval 100 "*" .file.Addition "/" "(" .file.Addition "+" .file.Deletion "+" 0.0 ")"}}%"></div>
+{{/* Template Attributes:
+* Addition: Number of additions
+* Deletion: Number of deletions
+* Classes: Additional classes for the root element
+*/}}
+{{if or .Addition .Deletion}}
+<div class="flex-text-block tw-flex-shrink-0 tw-text-[13px] {{if .Classes}}{{.Classes}}{{end}}">
+	<span>
+		{{if .Addition}}<span class="tw-text-diff-prompt-add-fg">+{{.Addition}}</span>{{end}}
+		{{if .Deletion}}<span class="tw-text-diff-prompt-del-fg">-{{.Deletion}}</span>{{end}}
 	</span>
+	<span class="diff-stats-bar" data-tooltip-content="{{ctx.Locale.Tr "repo.diff.stats_desc_file" (Eval .Addition "+" .Deletion) .Addition .Deletion}}">
+		{{/* if the denominator is zero, then the float result is "width: NaNpx", as before, it just works */}}
+		<div class="diff-stats-add-bar" style="width: {{Eval 100 "*" .Addition "/" "(" .Addition "+" .Deletion "+" 0.0 ")"}}%"></div>
+	</span>
+</div>
+{{end}}

templates/repo/pulls/tab_menu.tmpl

@@ -16,12 +16,7 @@
 			<span class="ui small label">{{if .NumFiles}}{{.NumFiles}}{{else}}-{{end}}</span>
 		</a>
 		{{if or .DiffShortStat.TotalAddition .DiffShortStat.TotalDeletion}}
-		<span class="tw-ml-auto tw-pl-3 tw-whitespace-nowrap tw-pr-0 tw-font-bold tw-flex tw-items-center tw-gap-2">
-			<span><span class="text green">{{if .DiffShortStat.TotalAddition}}+{{.DiffShortStat.TotalAddition}}{{end}}</span> <span class="text red">{{if .DiffShortStat.TotalDeletion}}-{{.DiffShortStat.TotalDeletion}}{{end}}</span></span>
-			<span class="diff-stats-bar">
-				<div class="diff-stats-add-bar" style="width: {{Eval 100 "*" .DiffShortStat.TotalAddition "/" "(" .DiffShortStat.TotalAddition "+" .DiffShortStat.TotalDeletion "+" 0.0 ")"}}%"></div>
-			</span>
-		</span>
+			{{template "repo/diff/stats" dict "Addition" .DiffShortStat.TotalAddition "Deletion" .DiffShortStat.TotalDeletion "Classes" "tw-ml-auto tw-pl-3 tw-font-semibold"}}
 		{{end}}
 	</div>
 	<div class="ui tabs divider"></div>

web_src/css/repo.css

@@ -1293,8 +1293,7 @@ td .commit-summary {
   filter: drop-shadow(-4px 0 0 var(--color-primary-alpha-30)) !important;
 }
 
-.code-comment:target,
-.diff-file-box:target {
+.code-comment:target {
   border-color: var(--color-primary) !important;
   border-radius: var(--border-radius) !important;
   box-shadow: 0 0 0 3px var(--color-primary-alpha-30) !important;
@@ -1681,18 +1680,27 @@ tbody.commit-list {
   margin-top: 1em;
 }
 
+.diff-file-box:target {
+  border-color: var(--color-primary) !important;
+  border-radius: var(--border-radius) !important;
+  box-shadow: 0 0 0 3px var(--color-primary-alpha-30) !important;
+}
+
 .diff-file-header {
   padding: 5px 8px !important;
-  box-shadow: 0 -1px 0 1px var(--color-body); /* prevent borders being visible behind top corners when sticky and scrolled */
-  font-weight: var(--font-weight-normal);
   display: flex;
   justify-content: space-between;
   align-items: center;
   flex-wrap: wrap;
+  gap: 0.5em;
+
+  /* prevent borders from being visible behind top corners when sticky and scrolled,
+  this "shadow" is used to use body's color to cover the scrolled-up left and right borders at corners */
+  box-shadow: 0 -1px 0 1px var(--color-body);
 }
 
-.diff-file-header .file {
-  min-width: 0;
+.diff-file-box:target .diff-file-header {
+  box-shadow: unset; /* when targeted, still use the parent's box-shadow, remove the patched above */
 }
 
 .diff-file-header .file-link {
@@ -1715,6 +1723,7 @@ tbody.commit-list {
   .diff-file-header {
     flex-direction: column;
     align-items: stretch;
+    gap: 0;
   }
 }
 
@@ -1743,13 +1752,13 @@ tbody.commit-list {
 
 .diff-stats-bar {
   display: inline-block;
-  background-color: var(--color-red);
+  background-color: var(--color-diff-prompt-del-fg); /* the background is used as "text foreground color" */
   height: 12px;
   width: 44px;
 }
 
 .diff-stats-bar .diff-stats-add-bar {
-  background-color: var(--color-green);
+  background-color: var(--color-diff-prompt-add-fg);
   height: 100%;
 }
 

web_src/css/themes/theme-gitea-dark.css

@@ -158,6 +158,8 @@ gitea-theme-meta-info {
   --color-diff-removed-row-bg: #301e1e;
   --color-diff-removed-row-border: #634343;
   --color-diff-removed-word-bg: #6f3333;
+  --color-diff-prompt-add-fg: #87ab63;
+  --color-diff-prompt-del-fg: #cc4848;
   --color-diff-inactive: #22282d;
   --color-error-border: #a04141;
   --color-error-bg: #522;

web_src/css/themes/theme-gitea-light.css

@@ -158,6 +158,8 @@ gitea-theme-meta-info {
   --color-diff-removed-row-bg: #ffeef0;
   --color-diff-removed-row-border: #f1c0c0;
   --color-diff-removed-word-bg: #fdb8c0;
+  --color-diff-prompt-add-fg: #21ba45;
+  --color-diff-prompt-del-fg: #db2828;
   --color-diff-inactive: #f0f2f4;
   --color-error-border: #e0b4b4;
   --color-error-bg: #fff6f6;