Page MenuHomePhabricator

Differential should preserve inline comments in code through revision updates
Closed, DuplicatePublic

Description

Currently, if there is some discussion in code and the revision is updated the comments from the previous revision are hidden from the top level UI - you have to navigate to the table of contents and click on the link for the inline comment to open the diff in a separate window. This makes it very hard to see the full extent of the review after it is completed if there was some iteration (because most of the comments will be hidden either by collapsed table of contents or by the newest revision). It also makes it hard to iterate, especially if there is more than one place in code being commented on per file. Consider the following scenario:

  1. Author submits a review
  2. Reviewer makes some lengthy comments on line 1 and line 10 of some file and requests some changes for both. A lengthy discussion follows in both places
  3. Author comments back with something like "let me work on the first one and see if we can get it right".
  4. Author submits a new diff with changes to address comments from line 1, but leaves line 10 unchanged.

Now the discussion on the second point is broken up across revisions and there is no way at all to see it fully.
This seems like a corner case, but actually happens quite a bit with reviews that have a couple of small issues and a larger more contentious issue where more discussion or clarification is needed, and in this kind of scenario it's the more complex issue that ends up being broken up,

It would be awesome if updating a revision could keep the threaded comments visible and figure out what line they moved to, if necessary. Obviously there's some corner cases here (like undoing the entire change being commented on), but even getting this somewhat working would make the view so much more comprehensive.

Event Timeline

anton.vladimirov raised the priority of this task from to Needs Triage.
anton.vladimirov updated the task description. (Show Details)
anton.vladimirov added a subscriber: anton.vladimirov.

I feel like this is also T1460 (and it's subtasks). At least, do issues exist post those tasks being properly completed? Can you read through those tasks and simply comment your additional thoughts for improvement there?

The closest I could find by reading through this stuff is T5654, which funnily enough is exactly what I was asking for in T6873.
There is a mention of this in that task, but that's about it. It's not really obvious that fixing either T1460 or T5654 will also address this. T1460 seems like it has grown into a monster that will address all inline comment issues/concerns/suggestions, though.

T1460 still seems like it solves the basic problem, when coupled with T5654. There is a list of inline comments, and the author can click whether they are done or not. There is a place to reference the discussion (via dialog) that does not include loading up a previous revision. When checking the 'completeness' of a review before final sign-off, our expectation would be to read the 'timeline' history as well as the final diff. It isn't clear what additional problems this request solves after those tasks complete (they are 2.5 years old, sadly).

In general, we have a strong preference for feature requests that state problems, not solutions. We have a very small team and try to stay focused on making sure we're as impactful with our time as possible. T4778 covers a bit how we prioritize.

A revision is composed of one or more diffs. Inline comments are against specific diffs. Inline comments show up when you are viewing the specific diff within the revision.

Obviously there's some corner cases here (like undoing the entire change being commented on), but even getting this somewhat working would make the view so much more comprehensive.

These corner cases are a real problem and would create far more confusion than the simple logic above.

As @chad said, T1460 is the task for solving the problem of "these inline comments are really a queue of things I need to get done." It is indeed a monster task because it involves transforming the simple "inline comments are about specific diffs" logic into something far more automagical and handling all sorts of corner cases, etc.

Sure, but the problem my team is having is actually the opposite of “these inline comments are a queue of things to get done”. Differential actually works really well for us when we’re converging a feature and doing a bunch of small targeted changes, but things tend to fall apart for larger reviews that take some online discussion and multiple iterations, like rewriting a component, for example. Unfortunately, my team tends to not incrementally iterate when doing large changes and we often get reviews in 100s of lines of code. I realize that this isn’t the best practice, but this is what they’re used to doing and Review Board supported this habit.

To put this as a problem, as @chad requested, there is a significant discoverability issue with longer, larger reviews that take more than 2 iterations. The reason is that there is no single place to see the entire discussion along with the (most recent) resulting code. This applies both general comments and inline comments, but is far worse for inline comments because individual comments about the same part of the change are spread across multiple diffs and are not viewable in the same page at all.
If T1460 is meant to address this - great! I just got the sense that it was, as I said, more about the opposite pattern when doing reviews.

These corner cases are a real problem and would create far more confusion than the simple logic above.

It sounds like you guys have thought about this. Is there a task that documents these corner cases and how/why they would be confusing in this scenario? Deletion and revert seem to be about the only 2 tricky cases from the UI perspective, but I don’t spend my whole day thinking about this :).

Sounds like this article might be helpful -- https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/

T1460 is otherwise our best shot at addressing this problem space.

The reason is that there is no single place to see the entire discussion along with the (most recent) resulting code.

When you load a given revision, by default it should be showing you the latest code and a collapsed timeline, which includes the entire discussion (and things like "User updated test plan."). You can click "Show older changes" to get to the older stuff, or use the keyboard shortcut "@" to show all older changes.

Sounds like this article might be helpful -- https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/

No doubt! And I personally agree, but the team has been doing it this way for the better part of a decade, and trying to convince a group of people that they need to change the way they do things to fit the new better tool we installed is basically a non-starter :). Just getting them to try switching to a different tool was a challenge.

When you load a given revision, by default it should be showing you the latest code and a collapsed timeline, which includes the entire discussion (and things like "User updated test plan."). You can click "Show older changes" to get to the older stuff, or use the keyboard shortcut "@" to show all older changes.

Yes, I understand. The issue my team is having is with the grouping of comments (I also filed T6874 and T6873 that are, I guess, variants of the same issue). So I’m not saying you can’t see all of content, I’m saying as your review gets longer it gets harder and harder to parse the state of the review.

Put this in the wrong task, sorry :).

Here’s an example. There are 2 conversations and some one-offs going on here, but trying to follow what is being discussed is already fairly difficult. This is a mild example - it fit on one screen.

Screen_Shot_2015-01-06_at_12.34.13.png (1×246 px, 142 KB)