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

feat: Add sorting by exclusive label. #33206

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

Conversation

telackey
Copy link

@telackey telackey commented Jan 10, 2025

This PR adds a new sort option for exclusive labels.

While this has been looked at before (see the first comment below for a list), the goal of this particular approach was to do it in the most lightweight and flexible way possible, by making use of the existing exclusive label system. No changes are made to the Issue model.

For exclusive labels, a new property is exposed called "order", while in the UI options are populated automatically in the Sort column (see screenshot below) for each exclusive label scope.

This doesn't impose any particular scheme for prioritization, labeling, etc.; that's all up to the user.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 10, 2025
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 10, 2025
@github-actions github-actions bot added modifies/translation modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/frontend labels Jan 10, 2025
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 10, 2025
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 10, 2025
@telackey telackey changed the title DRAFT: Add sorting of exclusive labels. DRAFT: Add sorting by exclusive labels. Jan 10, 2025
@telackey telackey changed the title DRAFT: Add sorting by exclusive labels. DRAFT: Add sorting by exclusive label. Jan 10, 2025
@wxiaoguang
Copy link
Contributor

There are different PRs for this problem and they conflict with each other (and the problem is pending for long time)

@dboreham
Copy link

Screen shot showing the feature:

image

@techknowlogick
Copy link
Member

pinging @lafriks as this is likely relevant to your interests

@dboreham
Copy link

Proposed fix for #2616

@telackey telackey marked this pull request as ready for review February 17, 2025 20:11
@telackey telackey changed the title DRAFT: Add sorting by exclusive label. Add sorting by exclusive label (fix for #2616) Feb 17, 2025
@telackey
Copy link
Author

telackey commented Mar 6, 2025

Can you also display the order in the label list?

Yes, I can. But, how would you feel about it on the rendered label?

image

@telackey
Copy link
Author

telackey commented Mar 6, 2025

Can you also display the order in the label list?

Yes, I can. But, how would you feel about it on the rendered label?

image

Added

@telackey
Copy link
Author

telackey commented Mar 24, 2025

Non-DB indexers (search engines) are still problematic. No idea how to handle.

@wxiaoguang

Agreed, my proposal above (and in the current code) is to use the DB indexer for all sort-by-exclusive-label queries. While that isn't ideal for all cases, it does cover what I expect to be by far the most common use case, which is to click into a repo and then sort the open, active tickets by some sort of priority or status label. It also avoids sending any queries to external indexers which they cannot fully handle.

@wxiaoguang
Copy link
Contributor

Hmm, I am neutral on this. What do maintainers think? also cc @lafriks

@lunny
Copy link
Member

lunny commented Mar 25, 2025

I have no better idea how to resolve it and I will not block this.

@lafriks
Copy link
Member

lafriks commented Mar 25, 2025

Compared to my implementation this does have some bonuses (less code, no need for calculating/re-calculating issue priority) but also I don't know how it will perform on large db as join by like 'something/%' will most probably result in table full-scan

@telackey
Copy link
Author

telackey commented Mar 25, 2025

I don't know how it will perform on large db as join by like 'something/%' will most probably result in table full-scan

@lafriks

I don't think this will be an issue, as the LIKE portion of the query will be a filtering step after joining by label.id. The number of issue labels per-repo is likely to be small, no matter how large the repo or how great the number of issues, and so even if you did end up doing a scan across the small set of labels already matched by id, it is likely as fast or faster than an index (since linear scans are usually more efficient than an index is when dealing with small sets).

If an index did prove useful, it can be done fairly simply. Since this is a prefix-based LIKE, it can make use of a basic alphabetical index to turn it into a range scan. Something like:

create index labels_name_idx on labels ( name asc );

@telackey
Copy link
Author

The latest commits have no functional changes, just merging in the last code from main.

@wxiaoguang
Copy link
Contributor

Will take a look later when I get some time.

(ps: no need to update for every main branch commit 😄 )

@wxiaoguang wxiaoguang self-assigned this Mar 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/frontend modifies/go Pull requests that update Go code modifies/migrations modifies/templates This PR modifies the template files modifies/translation size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants