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 <gabrielmo...@gmail.com>
wrote:

> Here's a screenshot from Swarm to show what I mean:
>
> [image: 2019-07-10 12_26_36-Swarm - Review 1078084.png]
>
> --
> 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+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/reviewboard/47b2d39a-1c0d-4406-942b-91ab954a1346%40googlegroups.com
> <https://groups.google.com/d/msgid/reviewboard/47b2d39a-1c0d-4406-942b-91ab954a1346%40googlegroups.com?utm_medium=email&utm_source=footer>
> .
> For more options, visit https://groups.google.com/d/optout.
>


-- 
Christian Hammond
President/CEO of Beanbag <https://www.beanbaginc.com/>
Makers of Review Board <https://www.reviewboard.org/>

-- 
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+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/reviewboard/CAE7VndkzaaTwsbN0a19C-4q0nMvwANc60ex4NGRKB71_c%3DDD0g%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to