Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move UI configurations that do not require restarting gitea to take effect to the admin dashboard #33909

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

hiifong
Copy link
Member

@hiifong hiifong commented Mar 16, 2025

replace: #33740

image

Verified

This commit was signed with the committer’s verified signature.
hiifong hiifong
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 16, 2025
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 16, 2025
@github-actions github-actions bot added modifies/translation modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/migrations modifies/frontend labels Mar 16, 2025
hiifong added 4 commits March 17, 2025 00:45

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
hiifong hiifong

Verified

This commit was signed with the committer’s verified signature.
hiifong hiifong

Verified

This commit was signed with the committer’s verified signature.
hiifong hiifong
@lunny lunny added this to the 1.24.0 milestone Mar 16, 2025
@lunny
Copy link
Member

lunny commented Mar 16, 2025

The old configuration in setting module should be removed to reduce confusion.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Sorry but I do not think this change is right.

Most setting options here are "user settings", it should belong to user's preference.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 17, 2025
@hiifong
Copy link
Member Author

hiifong commented Mar 17, 2025

Sorry but I do not think this change is right.

Most setting options here are "user settings", it should belong to user's preference.

Now the migration is the configuration in the ini file, these configurations currently belong to the global configuration, not the user preferences, you mentioned the user preferences configuration we will implement in the next pr, what do you think?

@wxiaoguang
Copy link
Contributor

Now the migration is the configuration in the ini file, these configurations currently belong to the global configuration, not the user preferences, you mentioned the user preferences configuration we will implement in the next pr, what do you think?

If there is a widely-known roadmap, then we can "implement it in the next pr".

For this feature, at least it is not clear to me what's your plan. What the final solution look like?

  • Should we keep or remove these ini options?
    • If keep, how to make the ini-options and db-options co-exist.
    • If remove, how to make users could set initial values for the config options.
  • What the user-setting system would look like.
  • If an option should be a user option, how to make it work with system option?
  • What kinds of options should be supported?
    • ini-only option
    • system-wide db option (inherit legacy ini option?)
    • user-setting db option (inherit system-wide option and inherit legacy ini option?)

hiifong added 2 commits March 18, 2025 00:59

Verified

This commit was signed with the committer’s verified signature.
hiifong hiifong

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@a1012112796
Copy link
Member

Most setting options here are "user settings", it should belong to user's preference.

no, I think they are performance related config options for web service, They will directly affect the performance overhead of the server. so it should not be configable for all users.

@wxiaoguang
Copy link
Contributor

Most setting options here are "user settings", it should belong to user's preference.

no, I think they are performance related config options for web service, They will directly affect the performance overhead of the server. so it should not be configable for all users.

Are you sure they should not be configurable for users? For example, not only one user mentions that they want to customize the default sort option like EXPLORE_PAGING_DEFAULT_SORT.

See the comment above: #33909 (comment) : clarify the config options first.

@lunny
Copy link
Member

lunny commented Mar 20, 2025

Most setting options here are "user settings", it should belong to user's preference.

no, I think they are performance related config options for web service, They will directly affect the performance overhead of the server. so it should not be configable for all users.

The database query is fast enough and I don't think we need to worry about it will impact performance.

The user level configuration could be another PR since this one's task is moving them to database so that a global change will not require restart Gitea instance.

@hiifong
Copy link
Member Author

hiifong commented Mar 20, 2025

If the paging size is freely configurable by the user, it will greatly affect the performance of the server.

Verified

This commit was signed with the committer’s verified signature.
hiifong hiifong
hiifong added 2 commits March 21, 2025 00:03

Verified

This commit was signed with the committer’s verified signature.
hiifong hiifong

Verified

This commit was signed with the committer’s verified signature.
hiifong hiifong
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged modifies/api This PR adds API routes or modifies them modifies/frontend modifies/go Pull requests that update Go code modifies/migrations modifies/templates This PR modifies the template files modifies/translation size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants