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 multi-line comment support in api #25523

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

Conversation

a1012112796
Copy link
Member

first step of #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. example:
    image

  • 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

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>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 26, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 26, 2023
@a1012112796 a1012112796 added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jun 26, 2023
@KN4CK3R KN4CK3R changed the title add multi-line comment suport in api add multi-line comment support in api Jun 26, 2023
@denyskon
Copy link
Member

denyskon commented Aug 1, 2023

@a1012112796 Could you resolve conflicts?

@denyskon denyskon added this to the 1.21.0 milestone Aug 1, 2023
@delvh
Copy link
Member

delvh commented Aug 1, 2023

I have architectural objections to this PR:
At the moment, you store StartLine (+/-) and Line (+/-) (let's ignore the confusing naming for now).
But with your approach, you must to store 4 lines: Start (old), End (old), Start (new), End (new).
Otherwise you confuse everyone trying to maintain the code, and create technical debt.

There is a reason why I recommended a one-way multiline comment that only stores a relative counter.
It is much easier to implement, and more user-friendly.
You don't need any convoluted mechanism to calculate which line is included in your drag, thus you make it predictable for your user, and as a user, you very rarely need a multiline comment both on the old side as well as the new side.

@a1012112796
Copy link
Member Author

a1012112796 commented Aug 2, 2023

But with your approach, you must to store 4 lines: Start (old), End (old), Start (new), End (new).
Otherwise you confuse everyone trying to maintain the code, and create technical debt.

why need four lines? do you means in database? now only add two new colums named as start_line and is_multi_line, the end_line is line in db. and in fact start_line only will be used in patch generation step, which willn't break curent ui logic. or maybe show it on ui in future.

then, not support start_side and line in different sides now also, because it's hard to generate patch for this usage.

@lunny lunny modified the milestones: 1.21.0, 1.22.0 Sep 21, 2023
@anbraten
Copy link
Contributor

anbraten commented Jan 16, 2024

I guess what delvh wants to say is that instead of adding start_line, you could add a next_lines property. In case next_lines is 0 it would just highlight the current line (as currently implemented). By using a number like next_lines>=1 it would hightlight the next x lines starting by line.

@lunny lunny modified the milestones: 1.22.0, 1.23.0 Mar 4, 2024
@lunny lunny modified the milestones: 1.23.0, 1.24.0 Sep 21, 2024
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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.

6 participants