Trying to comment in a diff view does not work at all.
For example if you go to:
https://secure.phabricator.com/differential/diff/28205/
You can not comment. And if there was already a comment, we would not be able to answer that comment.
Description
Description
Revisions and Commits
Revisions and Commits
Related Objects
Related Objects
- Mentioned Here
- D11718: PHUIDataKit - Bar Chart
Event Timeline
Comment Actions
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?
Comment Actions
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.
Comment Actions
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?
https://secure.phabricator.com/book/phabcontrib/article/feature_requests/ covers a bit more in detail on what we look for in feature requests.
Comment Actions
Ok, so here is the workflow:
- Tom creates a revision for his branch to be reviews
- Wilfried (the reviewer) has some comments, so he adds it inline (directly from the revision view)
- 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.
Comment Actions
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:
https://secure.phabricator.com/D11723#112877 (for example)
We can probably make this a little more obvious in the UI and link the copy On Diff #1234 to the appropriate place.
Comment Actions
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 :)
Comment Actions
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.)
Comment Actions
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.