No way to visualize comments from previous revisions, or to see fixes in-context

[Gabriel Morin]

I'm noticing that a few things hamper reviews with ReviewBoard a lot (especially compared to Swarm and CodeCollab):

• There's no option to see the comments from all revisions of the review on the diff. Competing products (Swarm in particular) let you do that, and to give you context, they accompany each comment with a small code excerpt from the revision they were made on to give context.
• When reading through the replies to your review, when you see something has been "fixed", there's no way to see right there what the fix was. Which brings me to my next point...
• Links leading to specific lines don't work, for instance http://reviewboard.mycompany.com/r/8/diff/5/?file=743#file743line65 doesn't go to line 65 - actually it doesn't even scroll to the file, it stays at the beginning of the page.

I was wondering if there was a roadmap to fix or improve those aspects.

Thanks!

[Gabriel Morin]

Here's a screenshot from Swarm to show what I mean:

[Christian Hammond]

Hi Gabriel,

Let me start with the easy one: Linking to a line. This certainly used to work, but I can confirm that it seems to have regressed. We'll look into it, figure out what may have changed.

Regarding showing old changes on a line, we don't plan to introduce that right now, but might later. I'll get into the reasoning today, and the work we're doing going forward.

The reason we don't have it today is that, while carrying over comments (and even marking them up to show that they're for an older revision) works well in some use cases, it adds confusion in others. It really depends on the workflow, how people manage their review requests and how they evolve over time. (We've seen a lot of variance in this, and try to design the product accordingly.)

For instance, if reviewers file a bunch of comments in a file (say, a mixture of architectural concerns and code style nits), and that file gets heavily rewritten to the point where it hardly resembles its former self (or has had large parts of it rearranged), those comments are not going to be useful in the context of the new version of the file. One can argue that that's fine, just discard the ones that don't at all seem to apply to the file anymore, but it's very common for reviewers to reference other comments in their review (for instance, providing some feedback on corrections that need to be made to a line in one comment, and then creating subsequent comments saying something like "Same applies here."). Losing that first comment means the following comments are no longer just wrong, they're problematic, as they'd incorrectly appear to pertain to some prior comment.

(Plus multi-line comments make this all even more challenging.)

For long-running changes with many reviews, carrying over comments just brings a lot of noise. The method we have of grouping comments into reviews is there so that people can more easily see the evolution of a change without living in the diff viewer. You can see the comments that were made across files by the reviewers, see when new revisions were made (and use interdiffs to see those changes), and really follow the entire review process right there. You can jump into the diff viewer (though as noted, links are currently broken), or you can get more context on those lines by hovering over the snippet of the diff and expanding right from the review without ever opening the diff viewer.

By separating out reviews from the diff viewer (in most tools, you just have comments inline in the diff viewer), and by providing interdiffs and expandable context, reviewers can get a pretty thorough understanding of the evolution of a review request. I don't think carrying old comments forward within the diff viewer buys us much more there.

Now we are working on a major change to the review experience for Review Board 5.0. We have designs and prototypes of this, and I think it's going to make much of the overall review process easier and more informative. It would also give us a better foundation for showing older reviews and how they pertain to the current diff you're looking at. It's something we can consider. We won't start full development on this until Review Board 4.0 is complete, though.

Regarding the second point about not showing how the code was fixed, there's not much we can automatically do to show this. While some comments are simply "You have a typo in this line," others are far more complex and far-reaching. Determining the changes made to a commented line and showing them is not necessarily enough. We do have some thoughts on how we can better tie fixed issues to review request updates themselves, but it's really the same problems as above. New versions of a file do not necessarily map well enough to older comments to be helpful here. The code in question may be completely removed or reworked or move to a new file.

These are hard problems. I do want to make improvements as we go forward, and we are going to continue to improve the code review workflow, but a lot of this comes down to really just differences between tools and workflows, and I don't want to attempt to match another product's bullet point without really carefully thinking through what it is we're trying to solve.

Christian

On Wed, Jul 10, 2019 at 9:33 AM Gabriel Morin <gabrie...@gmail.com> wrote:

Here's a screenshot from Swarm to show what I mean:

--
Supercharge your Review Board with Power Pack: https://www.reviewboard.org/powerpack/
Want us to host Review Board for you? Check out RBCommons: https://rbcommons.com/
Happy user? Let us know! https://www.reviewboard.org/users/
---
You received this message because you are subscribed to the Google Groups "Review Board Community" group.
To unsubscribe from this group and stop receiving emails from it, send an email to reviewboard...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/reviewboard/47b2d39a-1c0d-4406-942b-91ab954a1346%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Christian Hammond

President/CEO of Beanbag

Makers of Review Board