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

Scoped label improvements #22974

Open
4 of 9 tasks
brechtvl opened this issue Feb 18, 2023 · 18 comments
Open
4 of 9 tasks

Scoped label improvements #22974

brechtvl opened this issue Feb 18, 2023 · 18 comments
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first. type/summary This issue aggregates a bunch of other issues

Comments

@brechtvl
Copy link
Contributor

brechtvl commented Feb 18, 2023

Feature Description

Now that there is an initial implementation of scoped labels (#22585), there are some things to improve. Aimed at v1.19:

Important (for v1.20?):

  • Render non-exclusive scoped labels in the same style (Scoped label display improvements #23164)
  • Automatically remove conflicting scoped labels on all existing issues, when marking labels as exclusive.
    • Ideally it would display the number of labels that will be removed beforehand. This might be fairly expensive to compute for all labels when opening the labels page, so might need an AJAX request. Perhaps not critical for an initial solution.

Nice to haves:

  • Convenient scoped label switching by clicking on the label itself, and showing a menu with other labels in the same scope.
  • Nested scoped labels could have some custom rendering, currently they still show the / in the text.
  • Avoid overflow when label contains long words (see Scoped label display improvements #23164 for details)
@brechtvl brechtvl added type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Feb 18, 2023
@lafriks
Copy link
Member

lafriks commented Feb 18, 2023

Created PR #22976 to allow creating labels from yaml template files that allows exclusive scope option set

@lafriks
Copy link
Member

lafriks commented Feb 18, 2023

One thing I noticed (could be bug?) - non-exclusive scoped labels should also be rendered same as exclusive

@brechtvl
Copy link
Contributor Author

Communicating which labels are exclusive seems important. So even if we render non-exclusive ones in a similar style, there should probably be some visual difference still? I don't immediately have a good idea for what that would look like.

@lafriks
Copy link
Member

lafriks commented Feb 18, 2023

I don't think it's important to see it's exclusive or not when displaying issue or in issue list. It's only important when displaying label selection menu where it's already understandable which are exclusive and which are not

@lunny lunny added type/summary This issue aggregates a bunch of other issues and removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels Feb 19, 2023
brechtvl added a commit to brechtvl/gitea that referenced this issue Mar 4, 2023
Alt doesn't work on all browsers, the simplest solution for v1.19 is to just
not require it and toggle the label by just clicking.

Part of go-gitea#22974
brechtvl added a commit to brechtvl/gitea that referenced this issue Mar 4, 2023
brechtvl added a commit to brechtvl/gitea that referenced this issue Mar 4, 2023
It is convenient to be able to toggle off this option after removing / from
the name. This ensures the muted state is communicated to blind users even
when the input is not fully disabled.

Part of go-gitea#22974
lunny pushed a commit that referenced this issue Mar 5, 2023
Part of #22974

---------

Co-authored-by: delvh <dev.lh@web.de>
GiteaBot pushed a commit to GiteaBot/gitea that referenced this issue Mar 5, 2023
lunny pushed a commit that referenced this issue Mar 5, 2023
…23309)

Backport #23304

Part of #22974

Co-authored-by: Brecht Van Lommel <brecht@blender.org>
Co-authored-by: delvh <dev.lh@web.de>
lunny added a commit that referenced this issue Mar 5, 2023
…23306)

It is convenient to be able to toggle off this option after removing /
from the name. This ensures the muted state is communicated to blind
users even when the input is not fully disabled.

Part of #22974

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
GiteaBot pushed a commit to GiteaBot/gitea that referenced this issue Mar 5, 2023
…o-gitea#23306)

It is convenient to be able to toggle off this option after removing /
from the name. This ensures the muted state is communicated to blind
users even when the input is not fully disabled.

Part of go-gitea#22974

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
lunny added a commit that referenced this issue Mar 5, 2023
…23306) (#23311)

Backport #23306

It is convenient to be able to toggle off this option after removing /
from the name. This ensures the muted state is communicated to blind
users even when the input is not fully disabled.

Part of #22974

Co-authored-by: Brecht Van Lommel <brecht@blender.org>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
lunny added a commit that referenced this issue Mar 6, 2023
Alt doesn't work on all browsers, the simplest solution for v1.19 is to
just not require it and toggle the label by just clicking.

Part of #22974

Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
GiteaBot pushed a commit to GiteaBot/gitea that referenced this issue Mar 6, 2023
Alt doesn't work on all browsers, the simplest solution for v1.19 is to
just not require it and toggle the label by just clicking.

Part of go-gitea#22974

Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
jolheiser pushed a commit that referenced this issue Mar 6, 2023
Backport #23303

Alt doesn't work on all browsers, the simplest solution for v1.19 is to
just not require it and toggle the label by just clicking.

Part of #22974

Co-authored-by: Brecht Van Lommel <brecht@blender.org>
@BLumia
Copy link
Member

BLumia commented Mar 8, 2023

Quoted from the release note:

This should probably mark as breaking since existing labels may already contain /, e.g. triage | confirmed / verified (we used to use | as delimiter).

Also, could we re-consider making the scope delimiter configurable? A per-instance option could be enough IMO. Some reason:

  1. For existing labels, / can means "or". Using / as scope delimiter makes people not able to use / to express "or" for short.
  2. Being able to set delimiter to something else (e.g. | or ::) could help avoid public services like Codeberg break labels in existing projects Scoped labels #22585 (comment)
  3. To me, :: make perfect sense since it stands for namespace in some programming language and also could avoid such conflict. Also, GitLab already uses that so people don't need to learn it again if they are familiar with GitLab. It might seem ugly if we see that as pain text but we don't need to render it that way...

@lafriks
Copy link
Member

lafriks commented Mar 8, 2023

As I already mentioned in PR configurable delimiters will make it problematic especially in following cases:

  • If instance configuration is changed - all existing labels will break
  • Migrating repository from one instance to other with different configurations will break scoped labels
  • Same as above applies for federation - will have problems if instances have different delimiter configurations

@lafriks
Copy link
Member

lafriks commented Mar 8, 2023

NB, if you do not explicitly set that this scoped label is explicit only display will be affected not functionality so nothing will break for existing instances

@BLumia
Copy link
Member

BLumia commented Mar 9, 2023

If instance configuration is changed - all existing labels will break

IMHO it's the instance maintainer's responsibility to keep the label not breaking.

Migrating repository from one instance to other with different configurations will break scoped labels

For the current configuration, migrating issues from GitLab to Gitea might break the scoped labels since the delimiter is different and we cannot change it to the same one if intended to keep labels not breaking.

Same as above applies for federation - will have problems if instances have different delimiter configurations

Honestly, if we insist to keep this unconfigurable, I'll +1 for using :: instead of / which will cause much less issues, but since / already been merged, so I think making it configurable could still help for some small team to avoid breaking scoped labels.

this scoped label is explicit only display will be affected not functionality so nothing will break for existing instances

Existing labels that contain / will display in a confusing way if the user doesn't realize there is a new feature related to label. That's also considered breaking.

@lunny
Copy link
Member

lunny commented Mar 9, 2023

Quoted from the release note:

This should probably mark as breaking since existing labels may already contain /, e.g. triage | confirmed / verified (we used to use | as delimiter).

Also, could we re-consider making the scope delimiter configurable? A per-instance option could be enough IMO. Some reason:

1. For existing labels, `/` can means "or". Using `/` as scope delimiter makes people not able to use `/` to express "or" for short.

2. Being able to set delimiter to something else (e.g. `|` or `::`) could help avoid public services like Codeberg break labels in existing projects [Scoped labels #22585 (comment)](https://github.com/go-gitea/gitea/pull/22585#issuecomment-1408550024)

3. To me, `::` make perfect sense since it stands for namespace in some programming language and also could avoid such conflict. Also, GitLab already uses that so people don't need to learn it again if they are familiar with GitLab. It might seem ugly if we see that as pain text but we don't need to render it that way...

I don't think it's a break current implementation? There is a new column exclusive. The labels contains / will be considered scoped only exclusive is true.

@BLumia
Copy link
Member

BLumia commented Mar 9, 2023

Sorry for not actually trying the feature, this feature looks like this:

image

If user mark it as exclusive then / will be used as scope delimiter. For existing labels, exclusive will not be marked so it won't break anything. So I guess we don't need to mark it as breaking :)

btw, a use-case, do we support escaping the delimiter? For example, the label I mentioned above: triage | confirmed / verified. How can I write this in the current implementation? Is triage/confirmed \/ verified correct?

(p.s. the reason I think :: is good for use as delimiter is, we can safely say escaping is not needed at all since it semantically means namespace and user won't use it for anything other than scoping)

@lafriks
Copy link
Member

lafriks commented Mar 9, 2023

For the current configuration, migrating issues from GitLab to Gitea might break the scoped labels since the delimiter is different and we cannot change it to the same one if intended to keep labels not breaking.

As we know that GitLab will always have :: as delimiter we should convert them to Gitea delimiter when migrating labels.

@BLumia
Copy link
Member

BLumia commented Mar 10, 2023

As we know that GitLab will always have :: as delimiter we should convert them to Gitea delimiter when migrating labels.

For example, how it will be if the original label in GitLab is triage::confirmed / verified?

@BLumia
Copy link
Member

BLumia commented Mar 10, 2023

Another idea:

  1. Still make the delimiter configurable, and use / by default since people may think :: is ugly.
  2. Whatever user inputs in the label form, when they submit the form, replace all the delimiter to :: in database if Exclusive is checked. Which means we always use :: as delimiter in database.
  3. When retrieving label name from database, replace the :: back to the delimiter that user configured.

If we do this, then:

  1. If instance configuration is changed, all existing labels will NOT break since the in-database delimiter won't get changed.
  2. It's then possible to use / as non-delimiter without escaping, so triage::confirmed / verified is possible without doing escaping.
  3. Migrating repository from one instance to other with different configurations will NOT break scoped labels, too.
  4. Users will still able to use / (or whatever they like) as delimiter in the web interface so user won't think it's ugly.

@brechtvl
Copy link
Contributor Author

Personally my preference was to use :: but I also think the / we ended up with is fine. For either I think it's best for this to not be configurable. I made arguments for this in the original code review and my opinion on this is still the same.

I don't think changing :: just in the database works well, there also the API and any automation like bots built on that to consider. We can't cleanly hide the details of that, to me it seems like that risks further confusion. Having a single simple convention is easier in that sense.

Personally I think it's reasonable that in the (rare) case a label contains / with a different purpose it can be changed to e.g. or, | or something else.

@BLumia
Copy link
Member

BLumia commented Mar 12, 2023

I don't think changing :: just in the database works well, there also the API and any automation like bots built on that to consider.

Just ensure all API will always get :: and document this behavior. Make the configurable delimiter only works on the frontend instead of backend (consider those APIs are for backend, so always use the raw string that uses :: as delimiter in the API. I guess that will work.

Personally I think it's reasonable that in the (rare) case a label contains / with a different purpose

I honestly don't think the usage of / for 'or' will be rare.

@dboreham
Copy link

dboreham commented Jun 1, 2023

Hopefully this is a good place to mention: has any work been done on sorting by scoped label value in the issues list? The specific use case I have is to sort on Priority/xxx. This would need some way to assign an order key to each label in the set. I'm interested in implementing this if nobody is working on it.

@brechtvl
Copy link
Contributor Author

brechtvl commented Jun 1, 2023

To my knowledge, no work has been done on such sorting.

@lafriks
Copy link
Member

lafriks commented Jun 1, 2023

Hopefully this is a good place to mention: has any work been done on sorting by scoped label value in the issues list? The specific use case I have is to sort on Priority/xxx. This would need some way to assign an order key to each label in the set. I'm interested in implementing this if nobody is working on it.

Yes, I'm slowly working on this for 3y now 😅 #11669

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first. type/summary This issue aggregates a bunch of other issues
Projects
None yet
Development

No branches or pull requests

5 participants