Search code, repositories, users, issues, pull requests...

Copy link

Member

@lunny lunny commented Oct 1, 2025

screenshots:

image

lunny added 2 commits

September 30, 2025 11:14


 Allow configure themes via admin UI

160b7ec


 improvements

48451f6

@lunny lunny added this to the 1.26.0 milestone

@lunny lunny added the type/enhancement label

@GiteaBot GiteaBot added the lgtm/need 2 label

@github-actions github-actions bot added modifies/translation modifies/api modifies/go modifies/templates modifies/frontend labels

lunny added 2 commits

September 30, 2025 20:28


 Fix test

17bf354


 Update swagger documentation

629e4ba

silverwind

silverwind reviewed

web_src/css/admin.css

.admin .ui.table.segment.dropdown-container {

overflow: visible; /* allow dropdown menus to extend beyond container boundaries */

}

Copy link

Member

@silverwind silverwind Oct 1, 2025

Choose a reason for hiding this comment

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

Which problematic overflow: hidden in the ancestors is causing this? Can we possibly remove it?

@wxiaoguang

Copy link

Contributor

wxiaoguang commented Oct 1, 2025

It seems that Golang developers are really good at copying & pasting code and flood the code base with garbage .......

wxiaoguang reviewed

models/user/user.go

func (u *User) AfterLoad() {

if u.Theme == "" {

u.Theme = setting.UI.DefaultTheme

u.Theme = setting.Config().Theme.DefaultTheme.Value(context.Background())

Copy link

Contributor

@wxiaoguang wxiaoguang Oct 1, 2025

Choose a reason for hiding this comment

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

You should not use context.Background(), as always.

And the AfterLoad could be completely removed, if you have read the code.

The UserThemeName used by templates is already able to handle empty theme name correctly

wxiaoguang reviewed

modules/fileicon/render.go

func RenderEntryIconHTML(renderedIconPool *RenderedIconPool, entry *EntryInfo) template.HTML {

if setting.UI.FileIconTheme == "material" {

if setting.Config().Theme.DefaultFileIconTheme.Value(context.Background()) == "material" {

Copy link

Contributor

@wxiaoguang wxiaoguang Oct 1, 2025

Choose a reason for hiding this comment

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

No context.Background() please

I have spent enough time on cleaning up the rubbishes like

You have reviewed and approved those.

If you don't agree that "context.Background()" should be avoided, just tell me and BLOCK my PRs.

wxiaoguang reviewed

routers/api/v1/settings/settings.go

AllowedReactions: setting.UI.Reactions,

CustomEmojis: setting.UI.CustomEmojis,

DefaultTheme: setting.Config().Theme.DefaultTheme.Value(ctx),

DefaultFileIconTheme: setting.Config().Theme.DefaultFileIconTheme.Value(ctx),

Copy link

Contributor

@wxiaoguang wxiaoguang Oct 1, 2025

Choose a reason for hiding this comment

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

I don't understand why it needs to be exposed.

What's the real use case? Who need it? If it is important, why no test to make sure it is a stable API?

wxiaoguang reviewed

services/webtheme/webtheme.go

func initThemes() {

availableThemes = nil

defaultTheme := setting.Config().Theme.DefaultTheme.Value(context.Background())

Copy link

Contributor

@wxiaoguang wxiaoguang Oct 1, 2025

Choose a reason for hiding this comment

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

initThemes is called in "once", but defaultTheme can be dynamically changed now, so how could the logic be right for a changed "default theme"?

wxiaoguang reviewed

tests/integration/repo_test.go

Comment on lines +228 to +234

defer func() {

err := system_model.SetSettings(t.Context(), map[string]string{

setting.Config().Theme.DefaultFileIconTheme.DynKey(): "basic",

})

assert.NoError(t, err)

config.GetDynGetter().InvalidateCache()

}()

Copy link

Contributor

@wxiaoguang wxiaoguang Oct 1, 2025

Choose a reason for hiding this comment

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

Are you sure what you are doing?

defer test.MockVariableValue(&setting.UI.FileIconTheme, "basic")() means "use basic theme" for this test, and revert it after this test.

But your "defer" is only executed after the test?

wxiaoguang reviewed

web_src/js/features/admin/config.ts

for (const el of elAdminConfig.querySelectorAll('.js-theme-config-dropdown')) {

fomanticQuery(el).dropdown({

async onChange(value: string, _text: string, _$item: any) {

if (!value) return;

Copy link

Contributor

@wxiaoguang wxiaoguang Oct 1, 2025

Choose a reason for hiding this comment

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

Why !value would happen?

wxiaoguang reviewed

web_src/js/features/admin/config.ts

Comment on lines +32 to +33

const configKey = this.getAttribute('data-config-key');

if (!configKey) return;

Copy link

Contributor

@wxiaoguang wxiaoguang Oct 1, 2025

Choose a reason for hiding this comment

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

Why !configKey would happen?

wxiaoguang reviewed