From 86c1a33369e0458748d30d0084d3199e7f736855 Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Tue, 1 Apr 2025 15:02:30 +0800
Subject: [PATCH] Fix some UI bugs and clean up unused tests (#34088)
1. Make the material icon falls back to basic theme correctly
2. Remove `TestAttributeReader`, the problem has been resolved.
3. Fix `toggleElem` bug and add tests
---
modules/fileicon/material.go | 12 +++---
modules/git/repo_attribute_test.go | 60 ------------------------------
web_src/js/utils/dom.test.ts | 18 ++++++++-
web_src/js/utils/dom.ts | 2 +-
4 files changed, 23 insertions(+), 69 deletions(-)
diff --git a/modules/fileicon/material.go b/modules/fileicon/material.go
index aa31cd8d7c..cbdb962ee3 100644
--- a/modules/fileicon/material.go
+++ b/modules/fileicon/material.go
@@ -99,12 +99,9 @@ func (m *MaterialIconProvider) FileIcon(ctx reqctx.RequestContext, entry *git.Tr
}
name := m.findIconNameByGit(entry)
- if name == "folder" {
- // the material icon pack's "folder" icon doesn't look good, so use our built-in one
- // keep the old "octicon-xxx" class name to make some "theme plugin selector" could still work
- return svg.RenderHTML("material-folder-generic", 16, "octicon-file-directory-fill")
- }
- if iconSVG, ok := m.svgs[name]; ok && iconSVG != "" {
+ // the material icon pack's "folder" icon doesn't look good, so use our built-in one
+ // keep the old "octicon-xxx" class name to make some "theme plugin selector" could still work
+ if iconSVG, ok := m.svgs[name]; ok && name != "folder" && iconSVG != "" {
// keep the old "octicon-xxx" class name to make some "theme plugin selector" could still work
extraClass := "octicon-file"
switch {
@@ -115,7 +112,8 @@ func (m *MaterialIconProvider) FileIcon(ctx reqctx.RequestContext, entry *git.Tr
}
return m.renderFileIconSVG(ctx, name, iconSVG, extraClass)
}
- return svg.RenderHTML("octicon-file")
+ // TODO: use an interface or wrapper for git.Entry to make the code testable.
+ return BasicThemeIcon(entry)
}
func (m *MaterialIconProvider) findIconNameWithLangID(s string) string {
diff --git a/modules/git/repo_attribute_test.go b/modules/git/repo_attribute_test.go
index c4f5bac46d..d8fd9f0e8d 100644
--- a/modules/git/repo_attribute_test.go
+++ b/modules/git/repo_attribute_test.go
@@ -4,16 +4,10 @@
package git
import (
- "context"
- mathRand "math/rand/v2"
- "path/filepath"
- "slices"
- "sync"
"testing"
"time"
"github.com/stretchr/testify/assert"
- "github.com/stretchr/testify/require"
)
func Test_nulSeparatedAttributeWriter_ReadAttribute(t *testing.T) {
@@ -101,57 +95,3 @@ func Test_nulSeparatedAttributeWriter_ReadAttribute(t *testing.T) {
Value: "unspecified",
}, attr)
}
-
-func TestAttributeReader(t *testing.T) {
- t.Skip() // for debug purpose only, do not run in CI
-
- ctx := t.Context()
-
- timeout := 1 * time.Second
- repoPath := filepath.Join(testReposDir, "language_stats_repo")
- commitRef := "HEAD"
-
- oneRound := func(t *testing.T, roundIdx int) {
- ctx, cancel := context.WithTimeout(ctx, timeout)
- _ = cancel
- gitRepo, err := OpenRepository(ctx, repoPath)
- require.NoError(t, err)
- defer gitRepo.Close()
-
- commit, err := gitRepo.GetCommit(commitRef)
- require.NoError(t, err)
-
- files, err := gitRepo.LsFiles()
- require.NoError(t, err)
-
- randomFiles := slices.Clone(files)
- randomFiles = append(randomFiles, "any-file-1", "any-file-2")
-
- t.Logf("Round %v with %d files", roundIdx, len(randomFiles))
-
- attrReader, deferrable := gitRepo.CheckAttributeReader(commit.ID.String())
- defer deferrable()
-
- wg := sync.WaitGroup{}
- wg.Add(1)
-
- go func() {
- for {
- file := randomFiles[mathRand.IntN(len(randomFiles))]
- _, err := attrReader.CheckPath(file)
- if err != nil {
- for i := 0; i < 10; i++ {
- _, _ = attrReader.CheckPath(file)
- }
- break
- }
- }
- wg.Done()
- }()
- wg.Wait()
- }
-
- for i := 0; i < 100; i++ {
- oneRound(t, i)
- }
-}
diff --git a/web_src/js/utils/dom.test.ts b/web_src/js/utils/dom.test.ts
index 6e71596850..6a3af91556 100644
--- a/web_src/js/utils/dom.test.ts
+++ b/web_src/js/utils/dom.test.ts
@@ -1,4 +1,10 @@
-import {createElementFromAttrs, createElementFromHTML, queryElemChildren, querySingleVisibleElem} from './dom.ts';
+import {
+ createElementFromAttrs,
+ createElementFromHTML,
+ queryElemChildren,
+ querySingleVisibleElem,
+ toggleElem,
+} from './dom.ts';
test('createElementFromHTML', () => {
expect(createElementFromHTML('<a>foo<span>bar</span></a>').outerHTML).toEqual('<a>foo<span>bar</span></a>');
@@ -32,3 +38,13 @@ test('queryElemChildren', () => {
const children = queryElemChildren(el, '.a');
expect(children.length).toEqual(1);
});
+
+test('toggleElem', () => {
+ const el = createElementFromHTML('<p><div>a</div><div class="tw-hidden">b</div></p>');
+ toggleElem(el.children);
+ expect(el.outerHTML).toEqual('<p><div class="tw-hidden">a</div><div class="">b</div></p>');
+ toggleElem(el.children, false);
+ expect(el.outerHTML).toEqual('<p><div class="tw-hidden">a</div><div class="tw-hidden">b</div></p>');
+ toggleElem(el.children, true);
+ expect(el.outerHTML).toEqual('<p><div class="">a</div><div class="">b</div></p>');
+});
diff --git a/web_src/js/utils/dom.ts b/web_src/js/utils/dom.ts
index 6d38ffa8cd..b3debfde9e 100644
--- a/web_src/js/utils/dom.ts
+++ b/web_src/js/utils/dom.ts
@@ -44,7 +44,7 @@ export function toggleClass(el: ElementArg, className: string, force?: boolean)
* @param force force=true to show or force=false to hide, undefined to toggle
*/
export function toggleElem(el: ElementArg, force?: boolean) {
- toggleClass(el, 'tw-hidden', !force);
+ toggleClass(el, 'tw-hidden', force === undefined ? force : !force);
}
export function showElem(el: ElementArg) {