Comment on multiple lines in merge request diff
Problem to solve
Commenting on a single line is great for simple kinds of code review feedback, but frequently a comment is actually addressing multiple lines in the diff, perhaps a portion of a logic block, a paragraph of prose or an entire function. This forces users to chose a single line to provide feedback, but the feedback may in fact be resolved with a change to a different line.
Further details
Commenting on multiple lines does increase the complexity of performing a code review because it changes the decision making process for each discussion.
- Currently, a user need only decide which line is most relevant, click and add their comment.
- Multiline commenting, instead, requires a user to decide if this should be a single/multiline comment, then decide where to start of the range, and then the end of the range, and finally add their comment.
The question is, what percent of the time is a multi line comment preferable, and for that benefit is the increased complexity worth it?
Historic discussions
Some questions posed by Douwe earlier, with answers:
- How do we select that? Click 3, drag and hold until 8? What happens if someone wants a bigger range that doesn't fit inside the viewport?
- Click and drag. We shouldn't encourage selecting entire files, so whatever we get when we implement this (be that, you can't do it, or the viewport slowly scrolls down, as happens with selections)
- How do we display what lines a specific discussion applies to?
- State explicitly and show the range up to X lines
- Can people comment on a range that spans a collapsed region?
- Yes, I think this should be possible (another issue relates to this)
- Can people comment on intersecting ranges? (3-8 and 6-12)? How would they specify their range? How do we display that?
- Maybe for the first iteration not? In general I think this is easy to represent in the db, just hard in the interface.
- Can people comment on a line inside a range that has a discussion? How do we allow that to be created? How do we display that?
- Ideally yes, same as previous question. Otherwise skip for first iteration. Depends on UX more than anything.
- Can people comment on the last line of a range that has a discussion? How do we allow that to be created? How do we display that?
- What happens if someone adds a line in the middle of the range. Does the discussion become outdated, or remain active?
- What happens if someone removes a line in the middle of the range. Does the discussion become outdated, or remain active?
- What happens if someone changes a line in the middle of the range. Does the discussion become outdated, or remain active?
Proposal
Selecting a range
Range selection is by performed by clicking the start of the range, and shift clicking the end of the range. This is consistent with typical text/range selection patterns.
- Click the line number of the first or last line of the range
- Shift-Click the line number of the last or first line of the range respectively
Edge cases:
- selecting over collapsed regions: Optional. Support will likely eventually added, but the there is no requirement for this in the first iteration.
Visualizing the range, and overlapping ranges
- Discussions, single or multiple line, are shown at the bottom of the range to which they apply
- Hovering over a discussion thread shows the range to which it applies. Hovering over a line of code does not trigger a highlight event as there may be multiple matching comments.
-
Clicking on a discussion thread shows the range to which it applied. This will be sticky, preventing further hover events, until:
- clicking the same discussion again, to remove the selection, and restore hover highlighting
- clicking a different discussion twice - first to change the selection, then to remove the selection, and restore hover highlighting
- Clicking next unresolved discussion button should jump to the next unresolved discussion and highlight the range to which it relates.
| Hover/click a comment to see selection range | Over lapping ranges |
|---|---|
 |
 |
Marking discussions outdated
All discussions relating to a line that is changed are marked out of date.
We should measure the number of overlapping discussions with ~"telemetry