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

Add a new section named development in issue view sidebar to interact with branch/pr #31899

Draft
wants to merge 59 commits into
base: main
Choose a base branch
from

Conversation

lunny
Copy link
Member

@lunny lunny commented Aug 21, 2024

Partially Resolve #20226

This PR add a new section on the sidebar of issue named Development. It supports creating one or multiple branches and pull requests from the sidebar.

  1. No related branch or PR
    image

  2. Create new branch from the issue

image
  1. Related branch created
image
  1. Prefill title and content when creating pull request from the issue development link
    图片

  2. Realted PR created
    image

  3. Related PR merged
    image

  4. Mayge fixed near the issue title
    image

TODO:

  • Delete dev_link records from the database when deleting repos/issues
  • Delete dev_link records from the database when deleting pull requests/branch
  • Allow creating multiple branches/pull requests from UI
  • Bring issue information into the pull request when creating the pull request.
  • Adjust the toast z-index to 9999 so that it will display in front of the modal.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 21, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 21, 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 modifies/migrations labels Aug 21, 2024
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2024
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 22, 2024
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 29, 2024
@lunny lunny marked this pull request as ready for review August 29, 2024 19:46
@lunny lunny added this to the 1.23.0 milestone Aug 29, 2024
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Aug 29, 2024
@yp05327
Copy link
Contributor

yp05327 commented Dec 3, 2024

Wouldn't it be expected?

Personally, I don't think it is good.
If you only edited the form and closed the form without creating a new branch, the warning will display.

Can you post an screenshot? I can't reproduce it. See my screenshots in updated issue content.

image

@lunny
Copy link
Member Author

lunny commented Dec 4, 2024

Wouldn't it be expected?

Personally, I don't think it is good. If you only edited the form and closed the form without creating a new branch, the warning will display.

The logic is derived from all other forms. All other forms have the same behaviour to avoid user losing their content accidentally.

Can you post an screenshot? I can't reproduce it. See my screenshots in updated issue content.

image

Looks like it's a common bug which caused almost all the dropdown list.
图片
This has been fixed in 029a444

@lunny
Copy link
Member Author

lunny commented Dec 4, 2024

image The UI seems not good, if there's many PRs or branches, it looks disorganized

Spaces added.

@yp05327
Copy link
Contributor

yp05327 commented Dec 4, 2024

image

image
Locale is missing

@lunny
Copy link
Member Author

lunny commented Dec 4, 2024

image

image Locale is missing

Fixed in f2a28d0

@yp05327

This comment was marked as resolved.

@lunny
Copy link
Member Author

lunny commented Dec 5, 2024

Fixed in f2a28d0

Not only one. All of them are missing.

These locale strings should be there.
图片

IssueID int64 `xorm:"INDEX"`
LinkType IssueDevLinkType
LinkedRepoID int64 `xorm:"INDEX"` // it can link to self repo or other repo
LinkIndex string // branch name, pull request number or commit sha
Copy link
Contributor

Choose a reason for hiding this comment

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

We have branch table, so this can be int64?
ps: Commit SHA seems meaningless

Copy link
Member Author

Choose a reason for hiding this comment

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

In future PRs, we can link a commit to this issue. So it will be kept to support that. BTW: Azure supports linking to a commit. I think for some development model, they just push commits with no feature branches or pull requests, they can use this feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it can be split into to columns. LinkIndex(int64) and LinkRef(string)?
Using string type means you need to convert it from/to int64 type every time if it is a PR ID or branch ID which seems not good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then it can be split into to columns. LinkIndex(int64) and LinkRef(string)? Using string type means you need to convert it from/to int64 type every time if it is a PR ID or branch ID which seems not good.

The conversion is on the memory and we don't know what data we will store according to different LinkType. And this column will never be used as index, so we don't need to consider performance or as search condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also agree it doesn't seem to be a good name or proper design. There are already too many "index" in code: search index, database index, issue index, etc.

And using "string" to store "int" in database would cause problems, if you write ("where str_field = ?", intVal), then it can't use the index and would cause imperceptible performance problems.

@yp05327
Copy link
Contributor

yp05327 commented Dec 10, 2024

This section should be hidden or disabled when repo is empty or repo is archive or repo is mirror.

@lunny
Copy link
Member Author

lunny commented Dec 10, 2024

This section should be hidden or disabled when repo is empty or repo is archive or repo is mirror.

The links are still useable. Only create branch link should be hide.

type IssueDevLinkType int

const (
IssueDevLinkTypeBranch IssueDevLinkType = iota + 1
Copy link
Contributor

@wxiaoguang wxiaoguang Dec 12, 2024

Choose a reason for hiding this comment

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

Just tried to play with GitHub's development sidebar for a while. I can see there could be a lot of edge cases:

  1. create a new branch 123-my-issue, rename the branch to 123-my-issue-other, the link disappears, rename another branch to 123-my-issue, the link won't appear again.
  2. create a new branch 123-my-issue, create a PR from 123-my-issue, then the link is updated to PR, the branch link is replaced.

I believe these details need enough documents(comments) and tests.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 12, 2024

I guess it won't catch 1.23 milestone since this PR doesn't seem to be complete enough.


LinkIndex: I also agree it doesn't seem to be a good name or proper design. There are already too many "index" in code: search index, database index, issue index, etc.

And using "string" to store "int" in database would cause problems, if you write ("where str_field = ?", intVal), then it can't use the index and would cause imperceptible performance problems.


Just tried to play with GitHub's development sidebar for a while. I can see there could be a lot of edge cases:

  1. create a new branch 123-my-issue, rename the branch to 123-my-issue-other, the link disappears, rename another branch to 123-my-issue, the link won't appear again.
  2. create a new branch 123-my-issue, create a PR from 123-my-issue, then the link is updated to PR, the branch link is replaced.

I believe these details need enough documents(comments) and tests.

@lunny lunny modified the milestones: 1.23.0, 1.24.0 Dec 12, 2024
@lunny lunny mentioned this pull request Dec 17, 2024
@lunny lunny marked this pull request as draft December 27, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/go Pull requests that update Go code modifies/migrations modifies/templates This PR modifies the template files modifies/translation size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI topic/ux How users interact with Gitea 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.

Create a branch from an issue, with pull/merge request
6 participants