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

Fix tags view #23243

Merged
merged 5 commits into from
Mar 27, 2023
Merged

Fix tags view #23243

merged 5 commits into from
Mar 27, 2023

Conversation

wiktor-k
Copy link
Contributor

@wiktor-k wiktor-k commented Mar 2, 2023

Hi,

This PR fixes several issues reported in #23221.

It does three things:

  1. Fixes the DefaultBranch variable that has not been set.
  2. Sets Title and Message for newly created tags from the Tag message. This makes it easier to create releases from tags that have messages and for those that don't it doesn't have any effect.
  3. Makes UI changes so that tags look more like proper releases.

Before:

2023-03-02-12-31-19

After:

2023-03-02-12-31-31

I purposefully didn't reformat the template so that the diff is cleaner but can do so if that's welcome.

Thanks for your time!

@silverwind
Copy link
Member

silverwind commented Mar 2, 2023

I'm a bit confused because in the issue, the GitHub screenshot shows a tag, but in the gitea screenshot, it shows a release.

  • Release body text is from the manually created release UI.
  • Tag body text is from the git tag message.

Is this how this works? It almost seems like you are using the tag message as the release body text, but I don't think this is matching GitHub's behaviour.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2023
@wiktor-k
Copy link
Contributor Author

wiktor-k commented Mar 2, 2023

I'm a bit confused because in the issue, the GitHub screenshot shows a tag, but in the gitea screenshot, it shows a release.

It shows a tag. Why do you think it shows a release? The PR makes the display of tags similar to releases. (Or maybe I misunderstood something...)

  • Release body text is from the manually created release UI.
  • Tag body text is from the git tag message.

Is this how this works? It almost seems like you are using the tag message as the release body text, but I don't think this is matching GitHub's behaviour.

In my experience it matches GitHub's behavior, see for yourself here: https://github.com/wiktor-k/pysequoia/releases/tag/v0.1.14 and compare with https://try.gitea.io/wiktor/pysequoia/releases/tag/v0.1.14

Then if you compare with my second screenshot it looks more similar to what GitHub renders.

Maybe I misunderstood your question...

@silverwind
Copy link
Member

silverwind commented Mar 2, 2023

Ah, I see Github has a separate page for display of a single tag, which gitea lacks (clicking the tag takes you to the code view).

Do you think we can match GitHub in this regard? E.g. Releases tab should only be for manually created releases, and single Tags should be viewable on a separate page just like https://github.com/wiktor-k/pysequoia/releases/tag/v0.1.14.

@wiktor-k
Copy link
Contributor Author

wiktor-k commented Mar 2, 2023

Ah, I see Github has a separate page for display of a single tag, which gitea lacks (clicking the tag takes you to the code view).

Actually the route is there and the single tag view is also there it's just not used when you click the tag. This is one more thing that I've fixed in this PR.

(replacing src in URL takes to the single tag view: https://try.gitea.io/wiktor/pysequoia/releases/tag/v0.1.15 ).

Do you think we can match GitHub in this regard? E.g. Releases tab should only be for manually created releases, and single Tags should be viewable on a separate page just like https://github.com/wiktor-k/pysequoia/releases/tag/v0.1.14.

I think it already almost works like that. See the URL above. IMHO the tag view just lacked a little bit of polish that the release view had. I hope that this PR bridges the gap a little.

@silverwind
Copy link
Member

silverwind commented Mar 2, 2023

Sounds good, I will test this and I think we should change the link in the tag list to point to the single tag page instead. The old link to file view could then either be moved to the footer or dropped altogether.

@wiktor-k
Copy link
Contributor Author

wiktor-k commented Mar 2, 2023

Great! Thanks for your time!

Edit: forgot to mention: remember that the tag title and message is updated only on push. So if you test it locally use a fresh repo or remove all tags in gitea UI and then git push --follow-tags...

@a1012112796
Copy link
Member

a1012112796 commented Mar 3, 2023

a single change that can make the view more like github:

diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
index 1d1a0f588..9849888e5 100644
--- a/options/locale/locale_en-US.ini
+++ b/options/locale/locale_en-US.ini
@@ -1065,6 +1065,7 @@ release = Release
 releases = Releases
 tag = Tag
 released_this = released this
+tagged_this = tagged this
 file.title =  %s at %s
 file_raw = Raw
 file_history = History
@@ -2282,6 +2283,7 @@ release.compare = Compare
 release.edit = edit
 release.ahead.commits = <strong>%d</strong> commits
 release.ahead.target = to %s since this release
+tag.ahead.target = to %s since this tag
 release.source_code = Source Code
 release.new_subheader = Releases organize project versions.
 release.edit_subheader = Releases organize project versions.
diff --git a/routers/web/repo/release.go b/routers/web/repo/release.go
index bd5d5a388..3ffadd34a 100644
--- a/routers/web/repo/release.go
+++ b/routers/web/repo/release.go
@@ -226,7 +226,6 @@ func releasesOrTagsFeed(ctx *context.Context, isReleasesOnly bool, formatType st
 
 // SingleRelease renders a single release's page
 func SingleRelease(ctx *context.Context) {
-	ctx.Data["Title"] = ctx.Tr("repo.release.releases")
 	ctx.Data["PageIsReleaseList"] = true
 	ctx.Data["DefaultBranch"] = ctx.Repo.Repository.DefaultBranch
 
@@ -242,6 +241,12 @@ func SingleRelease(ctx *context.Context) {
 		ctx.ServerError("GetReleasesByRepoID", err)
 		return
 	}
+	ctx.Data["PageIsSingleTag"] = release.IsTag
+	if release.IsTag {
+		ctx.Data["Title"] = release.TagName
+	} else {
+		ctx.Data["Title"] = release.Title
+	}
 
 	err = repo_model.GetReleaseAttachments(ctx, release)
 	if err != nil {
diff --git a/templates/repo/release/list.tmpl b/templates/repo/release/list.tmpl
index eb21cd59b..acd083118 100644
--- a/templates/repo/release/list.tmpl
+++ b/templates/repo/release/list.tmpl
@@ -5,10 +5,10 @@
 		{{template "base/alert" .}}
 		<h2 class="ui compact small menu header">
 			{{if .Permission.CanRead $.UnitTypeReleases}}
-				<a class="{{if (not .PageIsTagList)}}active {{end}}item" href="{{.RepoLink}}/releases">{{.locale.Tr "repo.release.releases"}}</a>
+				<a class="{{if (and (not .PageIsSingleTag) (not .PageIsTagList))}}active {{end}}item" href="{{.RepoLink}}/releases">{{.locale.Tr "repo.release.releases"}}</a>
 			{{end}}
 			{{if .Permission.CanRead $.UnitTypeCode}}
-				<a class="{{if .PageIsTagList}}active {{end}}item" href="{{.RepoLink}}/tags">{{.locale.Tr "repo.release.tags"}}</a>
+				<a class="{{if (or .PageIsSingleTag .PageIsTagList)}}active {{end}}item" href="{{.RepoLink}}/tags">{{.locale.Tr "repo.release.tags"}}</a>
 			{{end}}
 		</h2>
 		{{if .EnableFeed}}
@@ -105,14 +105,14 @@
 									<a href="{{.Publisher.HomeLink}}">{{.Publisher.Name}}</a>
 								</span>
 								<span class="released">
-									{{$.locale.Tr "repo.released_this"}}
+									{{$.locale.Tr "repo.tagged_this"}}
 								</span>
 								{{if .CreatedUnix}}
 									<span class="time">{{TimeSinceUnix .CreatedUnix $.locale}}</span>
 								{{end}}
 								|
 								{{end}}
-								<span class="ahead"><a href="{{$.RepoLink}}/compare/{{.TagName | PathEscapeSegments}}{{if .Target}}...{{.Target | PathEscapeSegments}}{{end}}">{{$.locale.Tr "repo.release.ahead.commits" .NumCommitsBehind | Str2html}}</a> {{$.locale.Tr "repo.release.ahead.target" $.DefaultBranch}}</span>
+								<span class="ahead"><a href="{{$.RepoLink}}/compare/{{.TagName | PathEscapeSegments}}{{if .Target}}...{{.Target | PathEscapeSegments}}{{end}}">{{$.locale.Tr "repo.release.ahead.commits" .NumCommitsBehind | Str2html}}</a> {{$.locale.Tr "repo.tag.ahead.target" $.DefaultBranch}}</span>
 							</p>
 						{{else}}
 							<p class="text grey">

example view:
image

@a1012112796 a1012112796 added the topic/ui Change the appearance of the Gitea UI label Mar 3, 2023
@wiktor-k
Copy link
Contributor Author

wiktor-k commented Mar 3, 2023

@a1012112796 Indeed, this looks better. I've captured your diff into a commit and added it here. Thanks!

@codecov-commenter
Copy link

Codecov Report

Merging #23243 (0ccc6d3) into main (33e556e) will increase coverage by 0.14%.
The diff coverage is 54.99%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main   #23243      +/-   ##
==========================================
+ Coverage   47.44%   47.58%   +0.14%     
==========================================
  Files        1140     1143       +3     
  Lines      150804   151088     +284     
==========================================
+ Hits        71549    71900     +351     
+ Misses      70788    70681     -107     
- Partials     8467     8507      +40     
Impacted Files Coverage Δ
cmd/serv.go 2.59% <0.00%> (-0.06%) ⬇️
models/actions/run.go 1.72% <0.00%> (+0.01%) ⬆️
models/actions/run_list.go 0.00% <0.00%> (ø)
models/actions/status.go 22.85% <0.00%> (-1.39%) ⬇️
models/actions/task.go 1.69% <0.00%> (-0.01%) ⬇️
models/auth/oauth2.go 60.52% <ø> (ø)
models/auth/twofactor.go 19.73% <ø> (ø)
models/db/engine.go 44.80% <ø> (ø)
models/unittest/testdb.go 12.92% <0.00%> (ø)
modules/auth/password/hash/pbkdf2.go 69.69% <ø> (ø)
... and 119 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lunny lunny added this to the 1.20.0 milestone Mar 3, 2023
@silverwind
Copy link
Member

Gave it a test, there's still a few things missing from the Tag view:

Compare GitHub to this PR:

image

Screenshot 2023-03-14 at 22 40 15

  • Big header should be added
  • Tag message (as seen via git show <tag>) is missing

I'm not sure the backend infrastructure is there to obtain the tag message, so if it isn't we could defer that to another PR.

@wiktor-k
Copy link
Contributor Author

Tag message (as seen via git show ) is missing

@silverwind are you observing a tag that was pushed before this change? Since this commit I fixed the message update for pushed tags. But that will only work for new tags, as the code runs on tag push. (For testing you can remove the tag from UI and git push --follow-tags from console).

Big header should be added

Agreed. I'll fix that right away.

@wiktor-k
Copy link
Contributor Author

Big header should be added

Agreed. I'll fix that right away.

Okay, apparently that commit also sets correct title.

2023-03-15-10-45-22

Not sure what to do with existing tags, write a database migration that'd set title and message? (Not sure how it's done here). For me it's sufficient if new tags look nice but if you have a better idea I'm all ears.

Thanks for testing! 👋

@silverwind
Copy link
Member

silverwind commented Mar 15, 2023

@silverwind are you observing a tag that was pushed before this change

Yes, old tag. Do we store the tag message in the database? We should not, this is retrievable live from the git repo.

Releases are fine to store in the database, but tag name and message is part of the git repo.

@lunny
Copy link
Member

lunny commented Mar 16, 2023

@silverwind are you observing a tag that was pushed before this change

Yes, old tag. Do we store the tag message in the database? We should not, this is retrievable live from the git repo.

Releases are fine to store in the database, but tag name and message is part of the git repo.

Tags have been synced to databases when necessary because they just could be added or deleted, and cannot be modified. I think branches should also be synced to databases in future PRs. The beneficial of the operations will help in better performance, better paginations, better search, and better sort.

@silverwind
Copy link
Member

silverwind commented Mar 16, 2023

It sound completely wrong to me to sync tags to database.

You can for example not delete a tag without also deleting it from git itself. Releases are fine in the DB as they are unrelated to git, but tags should be purely based on git. I would suggest at minimum falling back to reading tag info from git if DB does not have it, but really this sounds completely broken to me.

@silverwind
Copy link
Member

It's fine to have additional attributes related to a tag in the DB, but the tag base attributes like tag name, message, signature and the tag's existance should come from git only.

@lunny
Copy link
Member

lunny commented Mar 16, 2023

It sound completely wrong to me to sync tags to database.

You can for example not delete a tag without also deleting it from git itself. Releases are fine in the DB as they are unrelated to git, but tags should be purely based on git. I would suggest at minimum falling back to reading tag info from git if DB does not have it, but really this sounds completely broken to me.

But the code was there for many years from @lafriks. And there is no better method to resolve the problem to search, sort, pagination problem. If there is any better idea, I think we can discuss them.

@silverwind
Copy link
Member

silverwind commented Mar 16, 2023

Sure, but I would like to at least see the tag message,signature etc of past tags, just like on GitHub. I think it should be possible to fallback to git in case db does not have the tag, right?

Gitea shows past tags with their name at least, so I think it at least partially already relies on git and we can probably just add additional fallbacks for message and signature.

@silverwind
Copy link
Member

I guess we can postpone the tag discussion, and take this PR purely as an UI improvment. I strongly believe that all tag data should come from git and git only. Releases on the other hand are a concept specific to Gitea, of course and have nothing to do with git, except that they can optionally point to tags.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 19, 2023
@lunny
Copy link
Member

lunny commented Mar 27, 2023

Please resolve the conflicts.

Signed-off-by: Wiktor Kwapisiewicz <wiktor@metacode.biz>
This makes it easier to create new releases from tags as the
release form is pre-filled with data that is in the tag
message anyway.

Signed-off-by: Wiktor Kwapisiewicz <wiktor@metacode.biz>
This patch makes Tag view look more similar to the one of
Release by making more template fragments reuse the same
markup.

Fixes: go-gitea#23221
@wiktor-k
Copy link
Contributor Author

wiktor-k commented Mar 27, 2023

Resolved conflict, rebased on top of main. Tested at it still looks nice:

2023-03-27-10-52-02

Edit: btw thanks for the comment, I didn't get the notification from GitHub that the PR is in conflict :/

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 27, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 27, 2023
@lunny lunny merged commit b78c955 into go-gitea:main Mar 27, 2023
@lunny lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 27, 2023
@wiktor-k wiktor-k deleted the fix-tags-view branch March 27, 2023 13:42
@silverwind
Copy link
Member

silverwind commented Mar 27, 2023

There's still some things I'm not happy about because of the way gitea handles tags vs. releases. We should follow up and fix:

  1. If a tag and a release exists with the same name, the tag list links to the release page with nothing linking to the tag page. Tag list should always link to tags only. Release list links to releases.
  2. Tag page is missing both tag title and tag message for "unsynced" tags, showing a rather broken page (as seen above). Idk, ideally fix the sync bug first.

As a first step, gitea should not try to suggest that tags are releases as the current UI does. Those are two distinct concepts that should not be mixed.

zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 28, 2023
* upstream/main:
  Fix issue due date edit toggle bug (go-gitea#23723)
  Fix profile page email display, respect settings (go-gitea#23747)
  Update Gitea version in docs (go-gitea#23755)
  Fix SVG close tag, improve commit graph page UI alignment (go-gitea#23751)
  Remove incorrect HTML self close tag (go-gitea#23748)
  Refactor repo commit list (go-gitea#23690)
  Fix tags view (go-gitea#23243)
  Add commit info in action page (go-gitea#23210)
  Use GitHub Actions compatible globbing for `branches`, `tag`, `path` filter (go-gitea#22804)
@Zettat123 Zettat123 mentioned this pull request Apr 4, 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/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants