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

Included tag search capabilities #32045

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Conversation

bsofiato
Copy link
Contributor

@bsofiato bsofiato commented Sep 15, 2024

Resolves #31998

The first screenshot shows the tag page without any filter being applied:

image

The second one, shows the page when the given filter returns no tag:

image

The last one shows a single tag being filtered:

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 15, 2024
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 15, 2024
@github-actions github-actions bot added modifies/translation modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Sep 15, 2024
@techknowlogick
Copy link
Member

Thanks for the PR! Would you be able to include a screenshot of the change?

@bsofiato
Copy link
Contributor Author

For sure, @techknowlogick . Here they go :)

The first screenshot shows the tag page without any filter being applied:

image

The second one, shows the page when the given filter returns no tag:

image

The last one shows a single tag being filtered:

image

@@ -214,6 +214,8 @@ func TagsList(ctx *context.Context) {
ctx.Data["HideBranchesInDropdown"] = true
ctx.Data["CanCreateRelease"] = ctx.Repo.CanWrite(unit.TypeReleases) && !ctx.Repo.Repository.IsArchived

namePattern := ctx.FormTrim("q")
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really a fan of allowing end users to set SQL LIKE statements (i.e. they might not expect that entering v1_% shows all tags in Version 1X).
However, what would be the alternative?
Parsing a glob instead and mapping it to the LIKE syntax?

Copy link
Member

Choose a reason for hiding this comment

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

If we choose to merge this as it is, please add a docs page in https://gitea.com/gitea/docs describing that this search accepts the SQL LIKE syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ideia behind is to macht all tags whose related commit message contains the criterion (i.e., if the user passes "2" as the criterion. It should matches v2, v1.2, v.1.32, and so on). I double-checked the generated SQL and xorm puts the wildcard on both sides of the literal (see evidence attached). This behavior is inline with how the commit search works.

image

About the documentation changes, gonna submitt a PR latter today :)

Copy link
Member

@lunny lunny Sep 15, 2024

Choose a reason for hiding this comment

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

It uses builder.Like{"lower_tag_name", strings.ToLower(opts.NamePattern.Value())}, which will generate a SQL like lower_tag_name = ?, And the % will be added automatically if it hasn't been added. If you search with '2%', then the '%' will not be added automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@delvh do you think we still need to add a docs page for this particular change after the explanation ? I can't seem to find any existing page that is closely related to this change (the closest seem to be Usage > Protected tags but I feel is not quite right). Any ideas ?

Copy link
Member

@delvh delvh Sep 16, 2024

Choose a reason for hiding this comment

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

My problem is simply that I don't like hiding functionality from the user in intransparent ways.
What we could do as a simple workaround is add a tooltip (data-tooltip-content=… attribute) to the search bar stating
Search for matching tags. Use '%' as a placeholder for any sequence of chars and '_' for a single unknown char or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@delvh Cool, I'm gonna add the tooltip latter today :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@delvh What do you think ?

image

If you guys think it is ok, I'll push the changes :)

Copy link
Member

Choose a reason for hiding this comment

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

Last thing: I'm not quite happy with using the query param q as tag name.
I mean… it is sort of a query.
However, it might be confusing in the future if there are multiple search boxes why one is q while the other one isn't.

Copy link
Contributor Author

@bsofiato bsofiato Sep 16, 2024

Choose a reason for hiding this comment

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

Last thing: I'm not quite happy with using the query param q as tag name.
I mean… it is sort of a query.
However, it might be confusing in the future if there are multiple search boxes why one is q while the other one isn't.

Hey @delvh, this one could be a little tricky :(

The thing is that the page is using the combo.tmpl template. Which, in turn uses the input,tmpl. The latter (source-code attached) assumes the name of the field to be q. I could change it to receive a custom name, but it feels that it is out of the scope of this particular PR.

How do you guys want to proceed ? Do you want me to create another PR that allows a custom name for the combo component and them rebase the this one when the new one is merged ? Or do you want me to put it all together on a single PR ?

image

@lunny lunny added this to the 1.23.0 milestone Sep 15, 2024
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Sep 15, 2024
@lunny
Copy link
Member

lunny commented Sep 15, 2024

It's better to have a search results count following the text of the Table head. The number is different from the one on the Tab which means the total tags amount.

@bsofiato
Copy link
Contributor Author

bsofiato commented Sep 15, 2024

It's better to have a search results count following the text of the Table head. The number is different from the one on the Tab which means the total tags amount.

@lunny , I've submitted some changes to show the number of tags on the header of the tags table. I've attached some evidences to show how it looks now.

image

@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 Sep 15, 2024
@bsofiato bsofiato force-pushed the feature/tags-search branch 2 times, most recently from 5bc7e2c to c540c7d Compare September 16, 2024 20:52
@@ -214,6 +214,8 @@ func TagsList(ctx *context.Context) {
ctx.Data["HideBranchesInDropdown"] = true
ctx.Data["CanCreateRelease"] = ctx.Repo.CanWrite(unit.TypeReleases) && !ctx.Repo.Repository.IsArchived

namePattern := ctx.FormTrim("q")
Copy link
Member

Choose a reason for hiding this comment

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

Last thing: I'm not quite happy with using the query param q as tag name.
I mean… it is sort of a query.
However, it might be confusing in the future if there are multiple search boxes why one is q while the other one isn't.

@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 Sep 17, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 17, 2024
@lunny lunny merged commit 7dde3e6 into go-gitea:main Sep 17, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 17, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 18, 2024
* giteaofficial/main:
  Add missing comment reply handling (go-gitea#32050)
  Fix CI (go-gitea#32062)
  Lazy load avatar images (go-gitea#32051)
  Included tag search capabilities (go-gitea#32045)
  Do not escape relative path in RPM primary index (go-gitea#32038)
  feat(go-gitea#31666): Set the columns height to hug all its contents (go-gitea#31726)
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
  Use a common message template instead of a special one (go-gitea#31878)
  Check if the `due_date` is nil when editing issues (go-gitea#32035)
matera-bs pushed a commit to matera-ar/gitea that referenced this pull request Sep 19, 2024
…ea#32045)

Resolves go-gitea#31998

The first screenshot shows the tag page without any filter being
applied:

![image](https://github.com/user-attachments/assets/eac0e51c-9e48-42b2-bb1c-a25896ca40cb)

The second one, shows the page when the given filter returns no tag:

![image](https://github.com/user-attachments/assets/98df191e-1a7b-4947-b0ef-4987a0293c3e)

The last one shows a single tag being filtered:

![image](https://github.com/user-attachments/assets/79c7e05e-8c86-4f06-b17e-15818b7b9291)

Signed-off-by: Bruno Sofiato <[email protected]>
matera-bs added a commit to matera-ar/gitea that referenced this pull request Sep 19, 2024
…ea#32045) (#3)

Resolves go-gitea#31998

The first screenshot shows the tag page without any filter being
applied:

![image](https://github.com/user-attachments/assets/eac0e51c-9e48-42b2-bb1c-a25896ca40cb)

The second one, shows the page when the given filter returns no tag:

![image](https://github.com/user-attachments/assets/98df191e-1a7b-4947-b0ef-4987a0293c3e)

The last one shows a single tag being filtered:

![image](https://github.com/user-attachments/assets/79c7e05e-8c86-4f06-b17e-15818b7b9291)

Signed-off-by: Bruno Sofiato <[email protected]>
Co-authored-by: Bruno Sofiato <[email protected]>
matera-bs added a commit to matera-ar/gitea that referenced this pull request Oct 29, 2024
…ties (go-gitea#32045) (#3)

Resolves go-gitea#31998

The first screenshot shows the tag page without any filter being
applied:

![image](https://github.com/user-attachments/assets/eac0e51c-9e48-42b2-bb1c-a25896ca40cb)

The second one, shows the page when the given filter returns no tag:

![image](https://github.com/user-attachments/assets/98df191e-1a7b-4947-b0ef-4987a0293c3e)

The last one shows a single tag being filtered:

![image](https://github.com/user-attachments/assets/79c7e05e-8c86-4f06-b17e-15818b7b9291)

Signed-off-by: Bruno Sofiato <[email protected]>
Co-authored-by: Bruno Sofiato <[email protected]>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 16, 2024
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. modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add searching capabilities to the tags page
5 participants