Page MenuHomePhabricator

Threaded comments
Closed, WontfixPublic

Description

There are two ways for a reviewer to leave feedback in Differential:

  • start a thread on a particular line
  • leave a comment on the ticket.

If someone posts a question to the ticket, I'd like to be able to respond specifically to that person/that question the same way I could if they left a line comment. (The reply should appear near the question, the person who asked should be notified of the reply, and either party should be able to mark that thread Done.)

Otherwise, ticket comments can be hard to follow, as multiple reviewers ask different questions, and the whole timeline is interspersed with commits.

Event Timeline

chad added a subscriber: chad.Jun 30 2016, 1:00 AM

I'd like to be able to respond specifically to that person/that question the same way I could if they left a line comment.

You can quote people if you're replying to comments that aren't inline.

It still seems weird that line comments are threaded but ticket comments aren't.

chad added a comment.Jun 30 2016, 1:03 AM

Why is a chronological order of events "wierd"?

There are (at least) two axes, time and topic. Line comments can be viewed on either. Generic comments can only be viewed by time, which makes them harder to read.

I can definitely see the utility in a high-level thread view, where you can see all of the issues raised and confirm they've all been addressed before landing a PR. If you have to manually follow multiple conversations in a single timeline, that's harder to do easily/correctly.

chad added a comment.Jun 30 2016, 1:10 AM

There is no perfect way to present this information, but chronological is universally understood and usually the most accurate. Having a threaded conversation splinters off topics, making it harder to follow the big picture. New comments can appear anywhere in the timeline.

We view the big picture, the "why" of why this code exists and being reviewed, the most important thing. Code Review to us is not about checking boxes or making sure arrows align, its about the fundamentals why the code is being written in the first place.

chad added a comment.EditedJun 30 2016, 1:13 AM

If you could look at Contributing Feature Requests and Describing Root Problems and help us better understand the problem that prompted this request, it would be really helpful. In general we prefer to not take feature requests that just say "build X, it's better". We need to keep focused (we're 2 people) on making sure we're solving commonly seen problems.

You've clearly used Phabricator a lot more than I have, but isn't it harder to follow those conversations when 3 of them are interleaved on a single timeline?

I'm not arguing that comments shouldn't appear on a timeline, but that it's easy to lose track of comments when they appear on a giant conveyor belt alongside commits and other topics. It would be nice to have a way to view conversations as entities to make sure nothing got overlooked before landing a PR. (Of course, this doesn't preclude those comments from also appearing on a timeline.)

chad added a comment.Jun 30 2016, 1:16 AM

T8610 T6874 T1460 are all sort of related here. We want to do a lot more with "Done" comments before exploring things like threading. I'm fairly hesitant that if we do "Done" right, it would be needed.

I promise I did read Contributing Requests before I posted this. ๐Ÿ˜€

Sounds like you understand my concern (it's hard to keep track of comments/conversations). If someone raises an issue, I want to be able to address the issue in a way where the conversation is easy to follow, and it's easy to see which conversations are still outstanding from a high level.

Thanks for your fast response. If you still need more clarification, let me know. ๐Ÿ˜ƒ

eadler added a project: Restricted Project.Jun 30 2016, 6:40 PM
scode added a subscriber: scode.Jun 30 2016, 7:03 PM
epriestley closed this task as Wontfix.Jul 7 2016, 10:42 PM
epriestley claimed this task.
epriestley added a subscriber: epriestley.

I don't plan to pursue this outside of the context of T1460, T8250, etc. If we did, it would be in the far future and after those changes land.

chad added a subscriber: lvital.