Conversation

zokkis

Show depending and blocked PRs/Issues in the PR/Issue list (Resolves #29018) and added a divider between texts in the item-body

before:
image

after:
image

@GiteaBotGiteaBot added the lgtm/need 2This PR needs two approvals by maintainers to be considered for merging.label Feb 9, 2024
@delvhdelvh changed the title enhancement: Show blocked_by and blocking in issue-list-view Show depending and blocked PRs in the PR list Feb 9, 2024
delvh
delvh previously approved these changes Feb 9, 2024

Choose a reason for hiding this comment

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

Two things I noticed while testing:
grafik

  1. We should swap the order of blocks/depends on around, as the depends on is much more important in most cases, so should be the first info you see.
  2. There's no clear separator when both are present, making the display slightly confusing. Not a blocker.

Apart from that, there might be some performance problems when querying a repo with many PRs/ a large dependency network. However, I don't know if they are (easily) fixable.
Otherwise LGTM.

@GiteaBotGiteaBot added lgtm/need 1This PR needs approval from one additional maintainer to be merged.and removed lgtm/need 2This PR needs two approvals by maintainers to be considered for merging.labels Feb 9, 2024
@silverwind

I would lowercase the text. All other text on the line is lowercase, so it looks out of place to title-case.

@zokkis

Two things I noticed while testing: grafik

1. We should swap the order of `blocks`/`depends on` around, as the `depends on` is much more important in most cases, so should be the first info you see.

2. There's no clear separator when both are present, making the display slightly confusing. Not a blocker.

Apart from that, there might be some performance problems when querying a repo with many PRs/ a large dependency network. However, I don't know if they are (easily) fixable. Otherwise LGTM.

I fixed it with a "|" between the list-items

@delvhdelvh mentioned this pull request Feb 9, 2024
@Greg-NetDuma

This is my subjective opinion as someone who made the feature request, items in priority order (this PR might already do some of them in that case ignore those points):

  • If the dependency is resolved (closed I guess) I don't want to see it in the depends_on list.
  • The PR refs (#1) should be clickable
  • I don't care if a PR blocks and for me it's not necessary to display it in the list, I only care if a PR is blocked since one way is enough to work out the dependency tree, and depends_on directly affects when that PR might get reviewed while blocks affects the other PRs instead.
  • depends on is a bit soft wording because effectively that PR is blocked by other PR's and I would prefer that if said PR would indicate the blocked state more prominently. You could
    • Change the wording to blocked by (might be too similar to blocks)
    • Change the icon of the PR as well
  • You might need to handle a big number of dependencies differently in the frontend, we sometimes have PRs with 5-10 dependencies. One option is that with 3+ dependencies you could just display #1, #2, #3 and more, but I'm not confident of a solution here.

With the exception of the first point if the things above sound unreasonable or they might not align with the majority of the people I'm ok with the current state of this PR.

@zokkis

This is my subjective opinion as someone who made the feature request, items in priority order (this PR might already do some of them in that case ignore those points):

* If the dependency is resolved (closed I guess) I don't want to see it in the `depends_on` list.

* The PR refs (`#1`) should be clickable

* I don't care if a PR `blocks` and for me it's not necessary to display it in the list, I only care if a PR is blocked.

* `depends on` is a bit soft wording because effectively that PR is blocked by other PR's and I would prefer that if said PR would indicate the blocked state more prominently. You could
  
  * Change the wording to `blocked by` (might be too similar to `blocks`)
  * Change the icon of the PR as well

* You might need to handle a big number of dependencies differently in the frontend, we sometimes have PRs with 5-10 dependencies. One option is that with 3+ dependencies you could just display `#1, #2, #3 and more`, but I'm not confident of a solution here.

With the exception of the first point if the things above sound unreasonable or they might not align with the majority of the people I'm ok with the current state of this PR.

  1. Good idea, I will try to implement it
  2. all links are clickable
  3. You don't need this feature, but its now in the list and maybe someone needs it
  4. 'depends on' is the official wording
  5. Yes its correct, a large number ob dependencies is a bit ugly in the list, but how often is the number of dependencies so large? And when I do #1, #2, #3 and more its no longer possible to click the link directly. I will look into it, but currently I don't have a good solution.

lunny
lunny previously requested changes Feb 10, 2024
@GiteaBotGiteaBot added lgtm/blockedA maintainer has reservations with the PR and thus it cannot be mergedand removed lgtm/need 1This PR needs approval from one additional maintainer to be merged.labels Feb 10, 2024
@pull-request-sizepull-request-size bot added size/L and removed size/M labels Feb 13, 2024
@silverwind

UI looks good. Only thing that maybe could be improved is to manually emit the separators in HTML instead of CSS because I fear that the CSS selector may have false matches, but at least from the screenshots, it looks alright.

@zokkis

Ah, it works now after enabling that. I hadn't expected it to be locked behind a setting.

Its because of #29117 (comment)

@silverwind

I think the rendering could be made similar to milestone, with an icon before an no text besides issue numbers joined by ", ". The question is just which icons to use for "depends on" and "blocks". I don't think octicons has anything suitable, but https://www.svgrepo.com/ might.

image

I noticed the dot we added causes unsatisfactory rendering for link hover. We need to fix that, maybe just remove the dot after all. A bit of whitespace should be enough between items.

image

@silverwind

These two are pretty good already, We could switch the box to a rounded outlined square:

https://primer..io/octicons/package-dependencies-16
https://primer..io/octicons/package-dependents-16

There is also a fitting icon for a "issue blocks":

https://primer..io/octicons/issue-tracked-by-16

The idea is that if the arrow in the icon indicates the direction of the dependency, pointing from icon to the numbers for "depends on" and from the numbers to icon for "blocks".

@zokkis

@lunny I need the join repo for the link

func (issue *Issue) Link() string {
and this is important for
const {owner, repo, index} = parseIssueHref(refIssue.getAttribute('href'));

@silverwind

Or even more simplified icons:

https://primer..io/octicons/arrow-right-16 for "depends on"
https://primer..io/octicons/arrow-left-16 for "blocks"

Each with a tooltip over the icon. If you agree I can do the necessary changes directly on the branch.

@zokkis

These two are pretty good already, We could switch the box to a rounded outlined square:

https://primer..io/octicons/package-dependencies-16 https://primer..io/octicons/package-dependents-16

There is also a fitting icon for a "issue blocks":

https://primer..io/octicons/issue-tracked-by-16

The idea is that if the arrow in the icon indicates the direction of the dependency, pointing from icon to the numbers for "depends on" and from the numbers to icon for "blocks".

I think this Icons are better
And should this be a Icon per dep or just one Icon on the front?

@Gr3q

One last thing, is it possible to change the color of the PR icon based on if the PR has unresolved dependencies? Creating a brand new icon for it is probably a stretch, even if Gitea currently does not use the PR closed icon for closed PRs.

To indicate more clearly that the PR cannot be merged so it's not worth reviewing (or it's dependencies are worth reviewing instead). If the dependency is a PR and is in the same repo it might be that the PR contains it's dependency, in that case a review is worth even less.

I'm not sure on the color (not green, red or gray because they are taken) and I don't know how hard it would be to do that. Also doesn't have anything similar.

@silverwind

And should this be a Icon per dep or just one Icon on the front?

One icon instead of the current text imho, to conserve space.

Meanwhile I've seen this icon in GitLab suite:

https://gitlab.com/gitlab-org/gitlab-svgs/-/blob/main/sprite_icons/issue-block.svg

image

So I would make two icons, similar to that one. Represent the issue/pr with a square and have arrows pointing in and out at bottom right, similar to package octicons above. If you can operate Inkscape or similar tools, please try creating these SVGs. Otherwise I will try 😆.

@github-actions-actions bot added modifies/goPull requests that update Go codemodifies/templatesThis PR modifies the template filesand removed modifies/migrations labels Apr 1, 2024
@zokkis

And should this be a Icon per dep or just one Icon on the front?

One icon instead of the current text imho, to conserve space.

Meanwhile I've seen this icon in GitLab suite:

https://gitlab.com/gitlab-org/gitlab-svgs/-/blob/main/sprite_icons/issue-block.svg
image

So I would make two icons, similar to that one. Represent the issue/pr with a square and have arrows pointing in and out at bottom right, similar to package octicons above. If you can operate Inkscape or similar tools, please try creating these SVGs. Otherwise I will try 😆.

Can someone do this for me? I'm not the best to edit/create SVGs

@silverwind

Sorry, haven't gotten around to making these SVGs yet, I will soon.

@silverwind

@zokkis SVGs added.

imageimage

Want to integrate them? I'd say wrap them in a <div data-tooltip-content="Depends on"></div> and <div data-tooltip-content="Blocks"></div>.

{{template "shared/issue_dependency" (dict
"Dependencies" (index $.BlockedByDependenciesMap .ID)
"TitleKey" "repo.issues.dependency.blocked_by_following")}}
{{end}}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{{end}}
{{end}}

Comment on lines +110 to +111
IssueID int64 `xorm:"UNIQUE(issue_dependency) NOT NULL index"`
DependencyID int64 `xorm:"UNIQUE(issue_dependency) NOT NULL index"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Two nits:

  1. Does it need a migration? For long time I have the question:
    • If a migration is a must, it should be documented in guideline and explained.
    • If a migration is not a must, then we do not need to require to write migrations
  2. The index for IssueID seems redundant, because issue_dependency should already provide the index for it.

@Gr3q

@zokkis SVGs added.
imageimage

Want to integrate them? I'd say wrap them in a <div data-tooltip-content="Depends on"></div> and <div data-tooltip-content="Blocks"></div>.

In my opinion these are way too similar and hard to tell apart, but I prefer it to be merged, then someone else can raise it in case if it's just me.

@lunnylunny modified the milestones: 1.23.0, 1.24.0 Sep 24, 2024
@lunnylunny modified the milestones: 1.24.0, 1.25.0 Apr 10, 2025
@lunny

Rethinking this PR, I believe two points need to be considered: first, whether it’s necessary to consume CPU resources to list these blocks; second, whether the UI will become too cluttered especially in the mobile UI.

Sign up for free to join this conversation on . Already have an account? Sign in to comment
lgtm/need 1This PR needs approval from one additional maintainer to be merged.modifies/goPull requests that update Go codemodifies/templatesThis PR modifies the template filesmodifies/translation type/featureCompletely new functionality. Can only be merged if feature freeze is not active.
None yet

Successfully merging this pull request may close these issues.

In the PR list page show if a PR is blocked by another issue/PR.