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

Display project's name in pages which are related to issues by using project.Name instead of project.Title #23038

Closed

Conversation

yp05327
Copy link
Member

@yp05327 yp05327 commented Feb 21, 2023

This PR is a part of #22865

Although we have different icons between repo project and user project,
(in some places they have same icons, I will fix them later)
but it is still hard to know which one is a repo's project if they have the same title.

Before:
image
After:
image

Maybe we can remove -?

@lunny
Copy link
Member

lunny commented Feb 21, 2023

How about moving the repository or owner information in the title?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 21, 2023
@yp05327 yp05327 changed the title Display projects Name instead of Title Display project's name when it is related to issues by project.Name instead of project.Title Feb 21, 2023
@yp05327 yp05327 changed the title Display project's name when it is related to issues by project.Name instead of project.Title Display project's name in pages which are related to issues by using project.Name instead of project.Title Feb 21, 2023
@yp05327
Copy link
Member Author

yp05327 commented Feb 21, 2023

How about moving the repository or owner information in the title?

project.Title will be used in Projects List/View Pages. It is no necessary to display the repository or owner information in these pages.
project.Name will be used in pages which are related to issues. (where I fixed in this PR)

Maybe we can change Name to DisplayName or something else.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 21, 2023

I have a draft design in my mind about this design, introduce a ProjectsTmplData struct:

  • ProjectsTmplData.Projects the project list
  • ProjectsTmplData.ProjectRepos the related repo map for the projects
  • ProjectsTmplData.ProjectOwners the related owner map for the projects
  • ProjectsTmplData.RenderFullTitle render the owner/repo/project title, or more functions to render to other formats

The benefit:

  • Backend code can prepare this structure within its context, then no need to use db.DefaultContext in Name()
  • It decouples the dependencies between project/repo/owner
  • It's database-friendly, these maps could be prepared by SELECT IN query, which is faster

@yp05327
Copy link
Member Author

yp05327 commented Feb 22, 2023

Since we have context cache now(#22294), I think we can add ProjectsTmplData or only RenderFullTitle to the cache?

@wxiaoguang
Copy link
Contributor

I don't think this problem is related to cache directly. If cache could help, then use it. If cache does no help, then no need to consider it.

ps: I think zeripath's PR #23051 is also the similar idea, while 23051 could use a JOIN query to load models.

@yp05327
Copy link
Member Author

yp05327 commented Feb 23, 2023

I don't think this problem is related to cache directly. If cache could help, then use it. If cache does no help, then no need to consider it.

ps: I think zeripath's PR #23051 is also the similar idea, while 23051 could use a JOIN query to load models.

@wxiaoguang
Your suggestion is really good. I have done what I understood about this idea.
Can you review them again? Thanks.

@yp05327
Copy link
Member Author

yp05327 commented Feb 23, 2023

I don't understand why IssueList is vaild here, but CI is angry with 'ProjectList' :(

type IssueList []*Issue

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 23, 2023

I don't understand why IssueList is vaild here, but CI is angry with 'ProjectList' :(

That's Go's quirk. It doesn't like PkgSymbol name in a package called pkg (aka pkg.PkgSymbol), it likes pkg.Symbol.

However, I think ProjectList is fine(better) here, you can add a nolint to suppress the CI error.

ps: lunny knows more about XORM, maybe he could help to review.

@yp05327
Copy link
Member Author

yp05327 commented Feb 23, 2023

So how about change the pkg name from Project to Projects.
I can't understand why the pkg name is Project which is without 's', as the other pkg names have. e.g. Issues, Packages

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 23, 2023

I would suggest not mixing the refactoring changes and feature changes together when a PR doesn't depend on the refactoring directly.

  • If some feature(or bug fix) changes must depend on refactoring changes, then it's fine to make them in one PR
  • If some feature(or bug fix) changes don't need refactoring changes, it's better to leave the refactoring to a separate PR to keep PRs clear.

@yp05327
Copy link
Member Author

yp05327 commented Feb 23, 2023

I would suggest not mixing the refactoring changes and feature changes together when a PR doesn't depend on the refactoring directly.

  • If some feature(or bug fix) changes must depend on refactoring changes, then it's fine to make them in one PR
  • If some feature(or bug fix) changes don't need refactoring changes, it's better to leave the refactoring to a separate PR to keep PRs clear.

I know this. I mean how about do it in a separate PR.

@wxiaoguang
Copy link
Contributor

I know this. I mean how about do it in a separate PR.

I have no idea. https://docs.gitea.io/en-us/guidelines-backend/#package-name

@brechtvl
Copy link
Contributor

brechtvl commented Mar 3, 2023

Personally I think a different icon for repo and organization projects would be enough. Giving the projects distinguishable titles is also just a responsibility of the user in my opinion.

To me this feels like it's adding clutter to the issue list. The screenshot shows a simple case, but if you have labels, PR status and conflicts, PR branches, org/repo on the dashboard pages, ... it all adds up and too much information makes it harder to scan for the relevant bits.

@yp05327 yp05327 closed this Mar 6, 2023
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants