Page MenuHomePhabricator

Create comments in diff views is not possible
Closed, ResolvedPublic


Trying to comment in a diff view does not work at all.

For example if you go to:

You can not comment. And if there was already a comment, we would not be able to answer that comment.

Event Timeline

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

The expectation is you can comment on Commits and Revisions. Is there a reason to comment on a diff view instead of the Revision in Differential?

Diff 28205 in your example is commentable in D11718.

You can not *add* comments, but the problem I see is that you can not answer a comment, which is usefull on an already updated diff.

It's more work to go to a diff view and leave comments vs. just staying on the Revision and leaving your comment there, so we need more of a reason to accept this as a feature request. Specifically, we need to understand what problem adding a commenting structure there solves that it doesn't already solve on the Revision view. Is there some point we're missing on your workflow? covers a bit more in detail on what we look for in feature requests.

Ok, so here is the workflow:

  1. Tom creates a revision for his branch to be reviews
  2. Wilfried (the reviewer) has some comments, so he adds it inline (directly from the revision view)
  3. Tom updates some patches but still has some pending question about one particular comment

-> Tom should be able to go to the Diff view where the comment was made and respond asking his question here so we have all the context about the particular question from Wilfried

I thought that was a bug rather than a feature request, this is not that important but it felt anti natural to me not to be able to do that.

It's not a bug, but understandable to ask about it. We don't intend that workflow you suggest and Diff View is just Diff View. For your workflow example, it's significantly simpler to just use the links provided in the inline-comment box: (for example)

pasted_file (173×701 px, 41 KB)

We can probably make this a little more obvious in the UI and link the copy On Diff #1234 to the appropriate place.

We can probably make this a little more obvious in the UI and link the copy On Diff #1234 to the appropriate place.

That would be nice indeed, I still did not understand how to do it for 'closed' diffs actually :)

btrahan added a subscriber: btrahan.

I think there's a few things we can do...

  • link "On Diff #1234" to pull up the Revision, with the proper diff loaded, right at the comment
  • link diff ids under the "ID" column in the "Revision Update History" table link to pull up the revision with the proper diff loaded and NOT link to the diff view page like it does today
    • as far as I know this is how users are getting there? it seems useless to link to and confusing to users
  • make the standalone diff page have more text
    • e.g. "This diff belongs to revision DXX. Please visit DXX to leave comments and / or respond to existing comments." from "This diff belongs to revision DXX."
    • note the standalone page is useful when creating a diff from the web ui; this explanatory text is the 'select a revision' UI in this case...
  • make the standalone diff page NOT show edit / reply to inline comments
    • these links don't work as is anyway

Lemme know if any of this sounds bad, else I'll tackle this tomorrow. (Though no promises on finishing tomorrow.)

Sounds good, thanks a lot for looking into that!

Ended up going a different way than the earlier plan... Basically, you can't get to the offending view anymore, and instead end up redirected to the revision page with the proper diff loaded.