Page MenuHomePhabricator

When Replying to an Inline Comment, provide original context in email?
Closed, ResolvedPublic

Description

This makes it very unpleasant UX when someone is answering multiple comments by saying "Done", "Ok", "sure" etc. It'd be great if one can actually see what is "done" or "ok"

Event Timeline

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

This is exceedingly difficult to pursue as you've stated. The inline comments (of which could be dozens) are inlined after the body of the email. Replies to that email have no context unless you expressly write the context in Remarkup. I can't see any automated way for us to know the reply is for the body of the email or any of the inline comments.

T1460 is likely the most closely related task to improving this experience (but there is no email component).

chad claimed this task.

I think there is a misunderstanding. I'm not talking about inbound emails and parsing them in order to answer comments. I'll give an example to be more clear:

Currently I got the following email:

[...]  
INLINE COMMENTS
  .arcconfig:2 Done

REPOSITORY
  rR SomeRepo
[...]

I'd like to have:

[...]  
INLINE COMMENTS
  .arcconfig:2
     > @some-user:  Please fix this line
     Done

REPOSITORY
  rR SomeRepo
[...]

That way I know what is "Done".

I believe the algorithm could be pretty straight-forward:

for each new comment:
  list all old comments on this line
  append new comment
chad triaged this task as Wishlist priority.Feb 24 2015, 2:28 PM

Ah, that makes a little more sense. This should be considered when T1460 is built, but that task has been around for years as it is. :/

chad renamed this task from When answering to inline comment email notification does not quote the original comment to When Replying to an Inline Comment, provide original context in email?.Feb 24 2015, 2:35 PM

I'm not entirely sold on this, mostly because the "Done!" case is easily solved via email on T1460, but the real lengthy discussions are going to be quite cumbersome over email and may make the feature a net zero. We'll keep it around to see if anyone else finds it interesting.

I think taking a look at this post T1460 makes sense, but I'd guess this feature becomes a clear "wontfix" at that time.

Also, in general, putting threaded conversations into emails in a well-formatted way is pretty hard and I'd rather users visit the website for that sort of view.

Although I think that T1460 is related I don't think it affects this task much too much. This feature is to improve emails readability by providing context to inline comments that almost always refer to the previous one on a given line.

I'm also having trouble seeing what exactly is hard here. In which cases my simple algorithm (i.e. for all new comments add all comments on the affected lines) wouldn't work?

I'm also having trouble seeing what exactly is hard here. In which cases my simple algorithm (i.e. for all new comments add all comments on the affected lines) wouldn't work?

Unfortunately the same thing that is hard about every feature request we get here. It has to be weighed against thousands of others to be prioritized. Cost to build, document, and maintain for life a feature is quite high and Phabricator is quite large. There are 3 people working on this project and as far as we can tell, only one person who sees this feature as a benefit. It's just as likely if it were built someone else would come and file a task here to turn it off. All those need to be considered. Having more people think this is a neat idea also helps consideration - leaving this task around will help that.

T4778 covers how we prioritize and ship code (bugs and features).

https://secure.phabricator.com/book/phabcontrib/article/feature_requests/ covers what we look for in good feature requests.

Phabricator is Open Source, as you state this is easy to code (for what you want) so maintaining a fork or local patch is the likely the best immediate option for you.

chad removed chad as the assignee of this task.Apr 7 2015, 5:47 PM

This appears to be a Differential feature request , and is roughly the "show thread context for reply comments" suggestion on T10694 is about.

epriestley claimed this task.
epriestley added a subscriber: epriestley.

Yes, this was implemented in T10694.