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

Multi line review comments like github #12640

Open
3 tasks
a1012112796 opened this issue Aug 29, 2020 · 6 comments
Open
3 tasks

Multi line review comments like github #12640

a1012112796 opened this issue Aug 29, 2020 · 6 comments
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active.

Comments

@a1012112796
Copy link
Member

a1012112796 commented Aug 29, 2020

Description

As title, I think it's usefull for code review. thanks.
It's also usefull for #10007

example in gh:

tmp1

...

TODO List:

@silverwind
Copy link
Member

Also works using click on first line + shift-click on last line, it's what I use.

@6543 6543 added the type/proposal The new feature has not been accepted yet but needs to be discussed first. label Sep 6, 2020
@socketwench

This comment was marked as duplicate.

@generalludd

This comment was marked as duplicate.

@delvh
Copy link
Member

delvh commented Mar 6, 2023

As #21150, which I found originally is now closed, here are my thoughts again:

I've already gathered some implementation thoughts on this as I think GitHub's approach has some major drawbacks:
At the moment, we only gather one number +/-$line for a review comment, where + means on new and - means on old diff, and $line is the line number.

My proposal is to limit multi-line comments to one side: Either you mark multiple lines on the old side, or multiple lines on the new side, but no mixture in between.
Reasoning: You very rarely want a multi-line message that has both old and new content (speaking from my experience as a "serial reviewer"), and it has a couple of benefits:

  • it is (at least for the backend) far easier to implement: Simply store another signed number meant as a relative counter:
    • Case 1 - |x| = 0: It's a single-line message, so use the current algorithm including the corresponding settings.
    • Case 2 - |x| = 1: Show only the selected line without the additional context lines. Can be useful from time to time to clarify exactly which line you're talking about, and GitHub doesn't offer this functionality. Should be the same case as case 3, but is listed here for completeness.
    • Case 3 - |x| > 1: Show the next/last x lines, depending on the sign of x. Only show exactly the requested lines, and nothing more or less.
    • it might even make sense to switch the behavior of case 1 with the one of case 2 if an edge case can be avoided that way
  • it is far less error-prone (see i.e. GitHubs problems with suggestions)
  • the (backend) implementation is straightforward, hence much easier to maintain
  • it is a much better user experience (I always have to hope that GitHub interprets my dragging correctly which it often just… doesn't)

@delvh delvh changed the title [Feature suggestion] Multiple lines code comment like github Multi line review comments like github Mar 6, 2023
@delvh delvh added type/feature Completely new functionality. Can only be merged if feature freeze is not active. and removed type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Mar 6, 2023
@hiifong
Copy link
Member

hiifong commented Jun 23, 2023

Maybe we can refer to gitlab's multi line review comments:https://github.com/gitlabhq/gitlabhq/blob/897fe87f08c8819ba6b7baa8024c8141acf290f5/app/assets/javascripts/diffs/components/diff_view.vue

@silverwind
Copy link
Member

On a sidenote, I like how these vue files are ordered with <script> first, we should adopt that.

a1012112796 added a commit to a1012112796/gitea that referenced this issue Jun 26, 2023
first step of go-gitea#12640

- add `side` and `line` to replace `new/old_position` in api like
  github
- add `start_line` in api like github for `multi-line comment` creating
- for multi-line comment, will cut all lines in range [start_line, line]
  for patch generate, then user can view all lines in this range on
  commnet list ui.
- not suport `start_side` now, `line` and `start_line` should in same
  side

todos:
- [ ] suport creating multi-line comments by ui & incomming mail
- [ ] more ui updates

Signed-off-by: a1012112796 <1012112796@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

No branches or pull requests

7 participants