Page MenuHomePhabricator

Bring inline comments forward across revision updates
Closed, ResolvedPublic

Description

A common feature request is for inline comments to move forward across diffs as a revision is updated.

For example, if a user leaves a comment on line 10 of "example.c" in diff 1, it's desirable for that comment to still appear on line 10 of "example.c" in diff 2.

Why is this hard?

In the general case, it is impossible to port inline comments forward.

Diff 1 and Diff 2 can affect completely different files. In a more extreme case, the files they affect can be in completely different repositories.

For example, Diff 1 might affect "example.js", making a behavioral change. After discussion, perhaps a decision is made to document the behavior instead of changing it, so Diff 2 affects only "feature.md". We clearly can't bring comments on "example.js" forward to "feature.md".

In many common cases, it is difficult to port inline comments forward.

The lines may not exist in updates. For example, Diff 1 may change a behavior in "example.js". A user makes a comment on line 115, in the middle of the changed lines. After discussion, a decision is made to simply remove the entire feature. In Diff 2, lines 50-300 are deleted and the total file is now only 55 lines long. There is no great place to put this inline, and implementing a heursitic to place it at the beginning of the removed block (which is only arguably the right behavior) is nontrivial.

The lines may also move or change between diffs, including moves or changes caused by rebases. It's not clear how well we can track lines across arbitrary changes with additional intermediate changes, especially if diffs are missing context. This is likely difficult and complex to do well. In extreme cases, it's ambiguous: if a line is replaced with three lines, and none have the same text as the original line, we can not know which of the new lines is the "new" version of the old line. We are unlikely to be able to guess nearly as well as a human can, either, which will make the result seem wrong.

Why not just do a best effort?

We could try our best, get it right in the easy cases and accept suboptimal results in the harder cases.

However, we're worried this feature will be confusing and frustrating if the behavior is inconsistent and hard to predict. If inlines sometimes port forward and sometimes disappear, that's potentially a very bad user experience.

We're also worried that this feature will frequently produce results which a human thinks are wrong (because the human reviewer understands the underlying document), even though a computer can not reasonably figure it out. For example, Diff 1 might have a change like this:

+ if (call(a, b, c)) {

Diff 2 might look like this:

+ var = call(a, b, c);
+ if (var) {
+   log(var);

Our heuristic might identify the new line 1 as the most similar to the old line 1, but a human reviewer would likely prefer the new line 2, which retains the "if" statement. For humans, the "if" statement is probably a stronger indicator of the line being "the same", even though the call has more similar text. We can't know about this rule without encoding a lot of programming language details into the heursitic.

But, worse than that, humans would actually choose a new line based on the natural language text content of the inline comment. For example, if the text is "consider using a temporary variable to make this clause easier to read", humans would choose line 2. If the text is "the parameter b should actually be q", humans would choose line 1. In 2015, we can not possibly hope to get this right.

So this potentially leaves us with a feature which appears to human users like it frequently produces the wrong results.

We are very hesitant to build features like this. They make the software seem bad because it feels janky and buggy, and they create a high support burden for administrators and for the upstream: users believe the program behavior is wrong, and file bugs about it. Because the underlying algorithms for comment placement will necessarily be complex, it will not be easy for users to intuitively understand why comments are placed where they are, nor will it be easy for us to explain the placement.

Moving Forward

Implementing this feature is mostly about coming up with ways to mitigate the unpredictability, so the user experience is more understandable and consistent and the feature feels intentional rather than random and wrong.

Some ways to do this might include:

  • We can create some sort of "appendix" on changesets where inline comments end up if we can't figure out where they go. This guarantees that they show up somewhere, so they never disappear into thin air.
  • Similarly, we can create some sort of appendix for the diff as a whole, where comments which don't port forward into any of the files end up.
  • We might be able to have some kind of explanation of why comments were placed where they are, but it's hard for me to imagine what this might look like.
  • We could let users move comments which get placed "wrong", although this is a lot of work.
  • We can visually make it clear that forward-ported inlines are forward-ported.

Blockers

  • These changes are substantial and it most likely depend on completing T1460 first or concurrently, as that also implies a number of UI changes to how inlines work.
  • We should also fix outstanding inline bugs before making inlines more complex: T6464, T3669 (I have some plans for this as part of T1460), T4999.
  • Some amount of T2009 represents a technical blocker (particularly, we need to get test coverage for the giant ball of heuristics we're going to produce) and may have to happen first or concurrently, too.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • T7999 suggested something along the lines of the "minimal UI".
  • Via IRC, "(btw, the 'moving comments over' feature has been a big hit here.)"
  • Via Twitter, our heroism was recognized.

As a reviewer, I think bringing "Done" comments forward is quite important: it makes it a lot easier for me to verify that they're really done. This is the #1 benefit I've reaped from this feature so far.

As a reviewer, I think bringing "Done" comments forward is quite important: it makes it a lot easier for me to verify that they're really done. This is the #1 benefit I've reaped from this feature so far.

I agree with this, as we quite liked this idea in general, as mentioned. But not at the expense of being able to properly, distraction-free reviewing of the newest diff/code itself. :)

If I'm a reviewer that has previously requested a change it can be useful to see the "Done" comments. If I'm a second reviewer coming along after two or more diffs it's not terribly useful to me and serves more as a distraction from the code I'm trying to review. That seems to be the consensus amongst my team anyway.

@alancrisp - I'd rather have whole history of what was requested and done. That's why minimal ui for showing old done inlines is better in the long run: If I don't care, it's just one line I can skip. If I care it's visible and I can click to expand :-)

jparise added a comment.EditedApr 30 2015, 6:21 PM

Perhaps a reviewer could flag a comment as "Changes Requested" or similar to indicate whether the comment should be inherited by future revisions (until it is marked "Done" by the owner).

Comment threads that aren't requesting changes wouldn't have a "Done" flag at all and would be considered purely informational / conversational.

If a reviewer adds a comment flagged as "Changes Requested", it could also change their default review action to "Request Changes" as well.

chad added a comment.Apr 30 2015, 7:52 PM

@jparise Our thoughts on that are if an individual piece of code is "bad", the entire diff is "bad" and should be rejected as a whole - to keep things simple.

Looking at @chad's comment, I see that we do code reviews vastly differently than you guys do. I don't think I've ever rejected an entire diff in my 3 years here.

I think it's pretty important to differentiate discussion from requesting a change to a specific piece of code.
'Wow, I had no idea that we handled this case' is different from 'This needs to change, it's a performance problem'.

Other comments:

  • It's not super clear who can mark something as 'Done' because 'Done' always looks like a button --even when it's not actionable by you.
  • Sometimes, as a reviewer, I'd like to be able to mark a change I requested as Done.
  • The comments persist even when the code that you're talking about is deleted.
chad added a comment.Apr 30 2015, 8:23 PM

@scohen T1460 is the task for the "Done" feature.

The comments persist even when the code that you're talking about is deleted.

As in the case above, this is very helpful for reviewers who want to verify that their "Oh, just get rid of this, we don't need it."-style comment was actually addressed.

I don't know @epriestley, I found it really confusing to have my comment on a different line entirely. Coupled with the "I can't mark this as done" thing, it made for a confusing experience.

I don't know @epriestley, I found it really confusing to have my comment on a different line entirely.

+1

chad added a comment.Apr 30 2015, 8:44 PM

To me it seems confusing either way, but in those cases we usually over-inform. If we remove them, someone will come and complain on the other side. Are there precedents maybe in other review platforms?

I don't see any examples anywhere that have a whole inline "conversation" on an older diff. Do those conversations get folded to one line, or does each message get folded individually?

At HEAD, the messages are folded individually.

asteinlein added a comment.EditedApr 30 2015, 10:32 PM

I'm sorry to say I don't for the life of me understand how "this feature (as it exists today) is a net improvement to Differential". See this screenshot from one of our reviews, which both hightlights the fact that the diff is terribly difficult to read, and even more important, has major issues with where the forward-moving comments appear:

None of the comments in this example are in their proper place, as should be evident by this example. Bringing it forward makes no sense when it's this broken, and as you can also probably see, ref. my earlier comments: Do you find it easy to read the diff itself with the comments breaking it up? No.

After having used Differential a week now after upgrading and getting the forward-moving comments, we're now more annoyed than we were when I wrote my original comment. As is, Differential is almost broken for us.

I don't know @epriestley, I found it really confusing to have my comment on a different line entirely.

+1

+1^10... Can't emphasize this enough. I understand the difficilty with this feature as described in the task itself. But it's better left out than being basically broken, IMHO.

epriestley added a comment.EditedMay 4 2015, 3:24 PM

Are people who have had chance to use the auto-folding inlines finding that they're an improvement?

After using the collapsed inlines for about a day, I found that I was expanding every single one of them because I never remembered what they said and always wanted to reread the text. I also briefly watched another user run into the feature and also click every single inline to expand them, so I'm curious if this is actually as good for most users as the first poll implied or if it isn't actually as useful as it seemed like it would be.

I used auto folding and I can honestly say - it's a mixture. When working on same group of code when I remember context - it's time saver. When I forget context - It's OK. So I guess that's for me it's 75% good.

If you could magically confirm that a ghost inline still makes sense on the updated diff, then auto-expanding it makes more sense to me. The main utility of folding it by default would be to minimize the damage of bringing forward "stale" inlines. @avivey made a good point that this could be integrated more tightly into the "Done" state.

What about expand it if it is not done?

avivey added a comment.May 4 2015, 5:13 PM

I find that I expand everything I see, even things I "know" I wouldn't find interesting, but I blame this on my private OCD.

chad added a comment.May 4 2015, 5:16 PM

Other random ideas:

  • Fold “Done” comments only. Leave “Not Done” open.
  • Leave previous first comments open by default, but hide their replies with JS.
  • Just make it a preference to see older/future comments, no folding.
avivey added a comment.May 4 2015, 5:19 PM

Also, I'm missing a button to "see comment in original context", in cases where the current context is confusing.

Fold “Done” comments only. Leave “Not Done” open.

For me, making it easy to review "Done" comments to make sure they're actually done is the single most valuable thing this feature provides by a wide margin. In some ways, this would be worse for me than the opposite behavior (expanding only the "Done" comments).

Also, I'm missing a button to "see comment in original context", in cases where the current context is confusing.

I do think this is reasonable. If we un-fold, we could make "Older Comment" / "Newer Comment" be links to the comment in its original context.

Also, I'm missing a button to "see comment in original context", in cases where the current context is confusing.

I do think this is reasonable. If we un-fold, we could make "Older Comment" / "Newer Comment" be links to the comment in its original context.

I actually use 2-column view with diff between revisions so that I could see context. I bet that most good of ghost inlines comes actually from being able to locate context.

Combining a coversation to a single box would also help.
+1 to links to the original context.

I don't know @epriestley, I found it really confusing to have my comment on a different line entirely.

To circle back to this:

In the general case, it is impossible to bring comments forward on the same line (the line may have been deleted, split into multiple lines, duplicated, completely changed, etc.). The description of this task has some examples of cases where we definitely can not do this correctly and where humans will always think the algorithm is "wrong", no matter how smart we try to make it.

Of course, the current algorithm is quite dumb. We can try to make it smarter and do a better job in more cases, by adjusting the line numbers to account for added and removed lines between the diffs and possibly adding heuristics for matching diff text. However, I worry that doing this may be very complex, potentially very slow (we need to load a lot more data and generate intra-diff diffs in order to reposition the comments) and only produce marginally better results, which may often still appear to be on the "wrong" line (that is, it might make us wrong by 3 lines instead of 6, but human users probably don't care much about that, even though we're technically twice as accurate).

We can also drop more inline comments. For example, we could bring inlines forward only if the original lines still existed in the new diff with exactly the same text. But this will tend to make a lot of inlines disappear between updates. To me, it's much better to bring them forward (even if they aren't on the right lines) than to drop them.

Maybe the best way to move forward is to clarify the goal: what problem are you are trying to solve with this feature? For me, the #1 benefit of this feature is that it makes it much easier for me to verify that changes I requested have actually been made. I can look through an updated diff and see all my original comments, and then check the surrounding code to make sure that the changes I suggested were actually implemented. Often, the new code isn't the same as the old code (after all, I requested changes, so I'm expecting it to be different), so these inlines wouldn't appear if we were very strict about only bringing them forward if the code was the same. Being stricter (or doing some of the other suggestions, like hiding "Done" comments) would tend to make this feature worse (and maybe much worse) for verifying that changes have been made.

From some of this feedback, it sounds like some of you have a different goal in mind in asking for this feature. Is your actual goal just something like reading comment threads, not tracking changes across diffs? For example, if we had a mode which just showed comment threads (something like this earlier mock):

...would that be a better approach to the problem? Or perhaps imagine a "hide all code" button which hid all the code and just showed the inline comments.

Generally, can you explain why you want inlines to come forward, but not inlines on deleted code and not "done" inlines? I don't understand the use case behind these suggestions, because these suggestions make the feature much worse for my use case, and my use case seems like the most obvious/common/useful one. In particular, this feature is much more useful to me in this use case than I anticipated it would be before we built it.

(In that mock, imagine that all the "reply" inlines appear in a thread, even though there's technically only one on the mock.)

Also, to summarize the recent changes:

  • Inlines no longer fold.
  • The "Rewind"/"Fast-Forward" button now takes you to the original comment when clicked instead of toggling the inline.
  • You can disable this feature completely in SettingsDiff Preferences.

Somewhat already mentioned a few times, but these are the two things me and co-workers noticed after using this feature for a while:

  • The comments seem way too large to me. (We have a lot of inline discussion, probably a lot more than happens on this install)
  • The "older"/"newer" comments appear on wrong lines a large chunk of the time. This is the most annoying part, I'd say, because a lot of inlines are really specific about what happens on a line.

Those two points aside, this feature is a huge win.

In the general case, it is impossible to bring comments forward on the same line (the line may have been deleted, split into multiple lines, duplicated, completely changed, etc.)

That actually sounds like several different cases to me. I think the problem is that the algorithm is, as you put it 'dumb', or more accurately, the dumbest thing that can possibly work. Github actually has an algorithm for exactly this that's mostly good. Does it work perfectly in every case? No, but I've used it for years and have never really thought about the bad cases like I am now.

I've thought about this feature, and would be quite satisfied if I could mark things that I've requested as 'done', which would collapse the comment into a *very* small amount of text. Then, I can go over the comments that are clearly missing any context because they've been taken care of and get the 'noise' out of my present view. Right now, I'm forced to see unnecessary, out of date comments.

I also think that one of the problems is that there are several use-cases rolled into one and that it'll be impossible to make this feature work for all of them. It's clear to me that you guys use phabricator vastly differently than we do (there was mention above about how you guys routinely reject diffs, which *never* happens here) and this might be contributing to the problem.

Github actually has an algorithm for exactly this that's mostly good.
I've thought about this feature, and would be quite satisfied if I could mark things that I've requested as 'done'

On this, GitHub doesn't have "Done" at all, right? What's different about Phabricator which makes the ability to mark your own comments as done important here but not important on GitHub?

One of my main use cases is making sure that inlines survive a rebase, which we use frequently due to how unforgiving arc patch is. If that results in actual hunk changes, I'd still like to see the inline, and decide for myself if (1) it's still relevant, and (2) it still targets the right lines. If it's no longer relevant, it would be nice if I could "dismiss" it, which would stop it from showing up. If it no longer targets the right lines, it would be nice if I could "re-target" it.

What's different about Phabricator which makes the ability to mark your own comments as done important here but not important on GitHub?

What's different is that github has an algorithm that removes old comments that have been taken care of. Phabricator doesn't, from what I can tell. You could add that, but you didn't seem to think it was possible. Marking my own requests as 'Done' would be an alternate path to coming up with an algorithm.

Actually, I'm wondering if it's possible to do what github does. I might be incredibly ignorant here, but since Phabricator abstracts over several version control systems, finding the correct context might not be possible in light of git rebases. This might explain why github can collapse (or not) across fairly drastic changes.

We can definitely do what GitHub does (or some reasonable approximation, at least), I just don't want to spend a lot of time implementing it without understanding what problem we're trying to solve here. I thought I understood the problem, but I think that I don't actually have a good understanding of the use cases. I'd like to step back, discuss the problem more to understand exactly what you're after and why what we have isn't working well, and then move forward from there. Specifically, what I'm trying to do is:

  • Actually understand everyone's use case, which I definitely don't right now.
  • Choose the best solution which really solves the underlying problems -- this may not be comment porting at all.
  • Implement that solution.

I suspect the best solution is not just different heuristics for bringing comments forward, even though we may implement them eventually.

In particular, removing old comments makes this feature much worse for my "review comments to make sure they've been addressed" use case. To this point:

What's different is that github has an algorithm that removes old comments that have been taken care of.

It can not possibly do this. It can remove old comments on code which was deleted. This is not the same as comments that have been taken care of. It can not know that they have been taken care of.

For example, it will remove a comment like "It is absolutely critical that we retain this block, but you should call x() instead of y().". If the author updates by removing the block of code, the comment will be dropped.

This is an extreme example, but I've hit several cases already like "move this to X", "combine this into Y", etc., where bringing comments on deleted code forward is helpful in making sure things were addressed. I've hit zero cases so far where bringing comments forward felt distracting, confusing, etc. This is why I think I don't understand your use case: you're making suggestions which make the feature worse for me, and I don't understand why they make it better for you.

It can not possibly do this

It does some reasonable approximation of this, like I said before:

Does it work perfectly in every case? No, but I've used it for years and have never really thought about the bad cases like I am now.

For example, it will remove a comment like "It is absolutely critical that we retain this block, but you should call x() instead of y().". If the author updates by removing the block of code, the comment will be dropped.

That's about right.

This is why I think I don't understand your use case: you're making suggestions which make the feature worse for me

Like I mentioned before, I think you and I have drastically different working styles and expectations from code review tools. If I marked something as "absolutely critical", I'll come back to it and reopen the collapsed comment. Like you said, this rarely happens. I'm more interested in not being annoyed in the 99% case. A few days ago, I saw a PR that was so chock-full of comments that weren't relevant that I couldn't really see the code any more. I find that incredibly annoying --you may not. Making this better for me may very well make this worse for your case. I don't have an answer.

Can someone show me how to use this feature on GitHub? For example, my comment here:

https://github.com/epriestley/libphutil/commit/35f5206976b485bd976ba6a20200e046c7af8213

Is not porting forward to here:

https://github.com/epriestley/libphutil/commit/ee73d178a1510921f86041f0a67746a08db6aa06

...although I expect it to, as they're part of the same pull request:

https://github.com/phacility/libphutil/pull/45

My comments here also don't port forward:

https://github.com/epriestley/libphutil/commit/814f3c2d8966f39268175147a56439d45b33099f

...although given GitHub's commit-by-commit review this isn't too surprising.

None of the comments are visible on this view, either:

https://github.com/phacility/libphutil/pull/45/files

Chad's comment on the diff view didn't port into the file, and didn't port forward when I updated the diff.

I really suck at online communication, i was referring to Github _enterprise_ in my previous comments. I rarely use github.com, and can't speak to the behavior.

Actually, knowing what I know about github, the current behavior probably means that they've changed how it works.

Given your experience with GitHub Enterprise, would you expect the comments on the pull requests I linked above to be porting across updates if GitHub had behavior equivalent to GHE?

Can you show me a screenshot of an inline comment which ported between views in GHE? We don't have a copy of GHE so I can't easily test the behavior myself, but my impression was that GHE was mostly similar to GitHub in behavior.

The second blog post above (https://github.com/blog/785-pull-request-diff-comments) seems to suggest that comments "keep their position", but materially they don't here:

https://github.com/phacility/libphutil/pull/45/files

Chad commented on the line "the file", which is unchanged, but the comment has not ported forward.

(That link may not point at anything useful in a second.)

Ah, I got some of them to port forward.

@epriestly, it's doing it here:
https://github.com/phacility/libphutil/pull/45

See how your comments show up, and there is one comment on an 'outdated diff'?

Here, I changed line 7:

That stopped the comment from porting forward:

Is that a desired behavior?

Here, I directly addressed an inline by making a one-cahracter change:

That stopped the comment from porting forward:

Is that a desired behavior?

Short answer: Yes.

I think i understand what's going on here. If you make a change in the git hunk then it stops porting forward. Otherwise, the comment stays. There are probably other heuristics here as well (what happens if you split a hunk, etc).

If you make a change after the comment, it still ports forward, but any change in the hunk prior to the comment seems sufficient to stop it.

That's interesting, and kind of makes sense. You usually comment on the line that you want to discuss.

I think that behavior + the done button is actually an improvement over github. After all, If I accidentally comment on the previous line, then my comment will stay, which isn't good. The done button fixes this.

You could probably do more interesting things with 'commit regions' or whatever you call it when you drag over several lines to comment.

Just to double check, you definitely don't want the example in T7447#112231 (where only one character was changed, directly addressing an inline) to port forward in an ideal implementation?

Here's a similar example: adding one leading space also stops this:

...from porting forward:

That's desirable, and you don't want it to port forward even if the line was only affected by whitespace changes?

Do you care about the "stuff prior to the line in the hunk" rule? Is your ideal rule exactly GitHub's (earlier changes matter; later changes don't)? Generally, GitHub's implementation is completely ideal and you want no behavioral differences at all?

Let me be clear: I agree with you that there's no ideal implementation here, and it's quite possible to come up with something better/more natural.

if you make a change after the comment, it still ports forward,

I swore I read that "If you make a change *before* the comment, it still ports forward", so no, this doesn't seem to be ideal. I think I'd be satisfied about not porting forward if anywhere in the hunk is changed, though I'm sure there are edge cases that I'm not thinking about.

Not porting forward on a single-character whitespace change is probably ok as well, since we have a lot of comments like: "Remove leading space" or "Too many line breaks here" because we use python and python users are pedantic as shit about whitespace. If we used ruby, this might not be as good. Python people really give a shit about the difference between:

my_fn(a,b,c)

and

my_fn(a, b, c)

I'm curious what you think. Your head is much more into these issues than mine. I'm just a dumb user of these things.

I think we're just coming at this from very different use cases. If a diff has this line:

23 x_function();

...and I leave this comment:

You should use y_function() instead, ...

...and the update has this on the same line:

23 y_function();

That is the most valuable kind of comment to port forward to me. It makes it really easy for me to verify that the author fixed the problem in the right way. This is so valuable to me that it's even a net win with the HEAD behavior, where it sometimes ports forward somewhere not-all-that-nearby after rebases or other changes (I think we should get it closer more often after D12741 lands, although there's still room for improvement). This comment porting is so useful to me that I think this would still be a net win for me if the behavior was so completely broken that it always ported to the beginning or end of the file or even to a random line in the file. The only way to make this porting less useful is to not port the comment.

For me, the feature in HEAD is relatively good already. My ideal behavior is basically that everything ports forward in essentially every case, and ends up on whatever line I as a human reviewer would have chosen as the "most similar" line.

I've been enjoying the combination of the "Done" flag and inherited comments, but one thing that I've found bothersome is that I need to click "Done" for each comment in a "thread" to "close out" that line of conversation (once I've actually done the requested thing).

I feel like I'm doing something wrong. Perhaps "Done" should only appear once for each group of comments? Or clicking "Done" on a comment should mark all earlier comments as done, allowing new comments to be added that are still "undone".

gabe added a comment.May 8 2015, 7:12 PM

Played a bit with the most recent implementation of this (7c96ba4ced4825b659286f1ecd55e954408b685c) and it seems to work quite well for my use case. We had a few reviews with various comments on different versions, and for the most part, I'm pretty happy with how the current code is pulling them forward. It's not perfect, there are a few cases where stuff is out of context, but I'm partial to having the comment there anyway so I don't forget about it completely, and can go back to the previous diff to read it in context if needed.

gabe awarded a token.May 8 2015, 7:13 PM

In situations where ghost comments wind up doing more harm than good, it might be nice to hide them via a keyboard shortcut.

Most of the issues our team run into stem from the fact that we can't toggle the comments temporarily in order to get a distraction free view of the code. Some of our inlines spark some quite lengthy discussions which can sometimes get in the way when you only need to see the code.

We do use the diff of diffs feature heavily which helps this, but it's not always an option for a reviewer due to the fact we generally require two accepts on a diff before it lands. It's not uncommon for a diff to be in a second or third iteration by the time a second reviewer starts their own review from scratch. For the second reviewer, seeing previous discussions isn't always useful - it can be, but we also find that often it's desirable just to hide the noise for a minute to view the current code in its entirety before continuing with the review.

Just to demonstrate, this one gave a couple of our devs some grief earlier today. The comments create so much noise you can barely see the loop being reviewed.

Most of the issues our team run into stem from the fact that we can't toggle the comments temporarily in order to get a distraction free view of the code. Some of our inlines spark some quite lengthy discussions which can sometimes get in the way when you only need to see the code.

We do use the diff of diffs feature heavily which helps this, but it's not always an option for a reviewer due to the fact we generally require two accepts on a diff before it lands. It's not uncommon for a diff to be in a second or third iteration by the time a second reviewer starts their own review from scratch. For the second reviewer, seeing previous discussions isn't always useful - it can be, but we also find that often it's desirable just to hide the noise for a minute to view the current code in its entirety before continuing with the review.

I just want to give a big +1 to this. Even though we don't practice two reviewers, I find it very useful to not be distracted with inline comments when doing a final, read-the-code-undistracted review. Just being able to globally disable forward-transferring comments isn't very useful to us: It totally disables a generally useful feature. Being able to temporarily disable inline comments would help a lot, i.e. as I mentioned before by adding a "view options" for the diff (preferably in addition to a keyboard shortcut, but just a keyboard shortcut is too obscure IMHO).

I just want to give a big +1 to this. Even though we don't practice two reviewers, I find it very useful to not be distracted with inline comments when doing a final, read-the-code-undistracted review. Just being able to globally disable forward-transferring comments isn't very useful to us: It totally disables a generally useful feature. Being able to temporarily disable inline comments would help a lot, i.e. as I mentioned before by adding a "view options" for the diff (preferably in addition to a keyboard shortcut, but just a keyboard shortcut is too obscure IMHO).

I second the suggestion to hide all inline comments in a diff view. This does give a easy reading if there are many comments one-after-one.
Something on the lines of D12650.

jpoehls removed a subscriber: jpoehls.May 15 2015, 8:56 PM

I feel like I'm doing something wrong. Perhaps "Done" should only appear once for each group of comments?

My current thinking is that "Done" should be completely orthogonal to ghost inlines. I think "Done" also has significant room for improvement, but that it's a separate interaction (covered in T1460). There are a handful of behaviors related to "Done" where the way forward is clear and we just haven't had time to address them -- I'll copy your comments there.

Particularly, I don't currently intend to make "Done" affect comment porting. A major driver for this is that I think porting "Done" comments forward is important for reviewers to verify that they're really done.

Just being able to globally disable forward-transferring comments isn't very useful to us: It totally disables a generally useful feature.

In V28, you chose the response suggesting that Differential is worse with this feature than it was without it. Can you help me understand how this feature is both generally useful and a net negative?


I'm thinking about approaches for hiding comments. In the last week or so I've had a couple of similar experiences to some of the comments above, where some revisions with major changes across a long period of time end up with addressed, obsolete comments behind substantial rewrites.

We can implement a keyboard toggle for all comments (and D12650 does this), but this isn't very discoverable and feels fairly crude to me. It might still be a step forward.

In the cases I've run into, hiding individual comments feels like it might be a better fit. D9678 is a specific example I hit where the change had undergone major revision and some of the comments were now obsolete, while others were not, and I could imagine hiding the comments I was no longer concerned with could be helpful.

Somewhere between these is hiding threads, although making threads more formal than they are now feels premature until "Done" (T1460) is further along.

So my current thinking is to try to find a UI approach to let users hide individual comments (this would persist per-user) and toggle all comments on/off. I'd like both of these features to be accessible/discoverable through the normal UI (not just a keyboard shortcut). I have a few ideas for this, but need to play around with the UI interactions a bit to see how they feel.

At HEAD, each inline can now be individually hidden:

Hiding a comment only hides it for you. Comments stay hidden across reloads unless you explicitly show them again.

I've been using this for a few days and am pretty happy with it. Particularly, having control over which comments I still consider relevant feels good and it doesn't feel like I'm doing too much work to manage comment state, but I'm interested in feedback about how well this works for others. See D13009 for some additional discussion.

This is really helping to improve the code readability. A big welcome change :)
A small learning curve to first discover the "hide comments" feature was needed in the team. [Maybe "X" was making people hesitant].
Not many noises to have the Hide All/Show All feature, but as you said it is a personal opinion.

Looking forward to this coming to Diffusion/Audit application too as a couple of teams are heavy on audit workflow.

epriestley closed this task as Resolved.Jun 21 2015, 2:01 PM

I'm going to close this since we seem to have made it to a state that no one hates too much. There are some followups in T8420 and T7870, and we'll adjust the UI a bit more and bring hiding to Diffusion in the future.

(The separate "Done" checkbox still has a lot of work to do, but see T1460 as an entrypoint into all the done/threading/context/etc stuff.)