For design reference, here's the GitHub version of this mail (each comment arrives in a separate mail):

Spacing/padding is definitely pretty iffy in Mail.app, at least.

This is getting somewhere functionally; we're going to iterate on the design a bit more internally.

These are the reply-to-comment cases which are a bit rougher still:

(Installs should be cautious about deploying these changes because they're sitting on top of Diffusion changes described in T10922.)

@epriestley This is kind of random but this looks like it will look really cool in email. Is there any chance of bringing these snippets into the discussion view on a review instead of just the links to the inline comments? Kind of like how github does this?

See this pic: https://git-scm.com/book/en/v2/book/06-github/images/blink-05-general-comment.png

@jcarrillo7, something in that vein will split out of T1460 eventually but we don't have a specific plan for it yet. I don't intend to address it as part of this task.

We iterated on the design a bit and ended up here:

Reply to an existing comment:

This appears to render reasonably in most clients, although D15864 still has some issues in Airmail, but they seem to be problems with the client's rendering engine.

We're open to feedback / bugs / client issues now.

Next steps:

• Make HTML mail the default.
• Remove the metamta.differential.unified-comment-context configuration option and document this change.

Overall it looks a *lot* better now :)

A user brought up that it's not super clear who is responding and who is being responded to (and I tend to agree). The identify of the person being replied *to* is available (but I spent several minutes being unsure whether it was the person being replied to or not, though probably because they were the same person in this case). However, the identify of the person who is replying is not available right there next to the comment iself.

The "epriestley added a comment" and/or "epriestly added inline comments" while technically having the appropriate information, is pretty much visual noise that as a reader of a huge number of these emails you just sift over and want to jump straight to the meat of the matter. These changes will make said meat of the matter a lot more prominent and trivial to jump to, but it means you want the identity to be *right there* as well.

(This tangents greater concerns about signal/noise ratio and ordering of information. But I'm refraining from mixing that in here and just addressing what's relevant in this context.)

Thoughts?

Is metamta.can-send-as-user not configured in your environment? My expectation is that the mail comes "From" the user writing all the text in the mail, which makes it clear who they are. I expect that this information is normally prominent in the UI of most mail clients.

There was an earlier iteration with this information (this is simulating two different mail messages):

At least to me, this felt redundant and low-signal. Every inline is always added by the user who the mail is "From", which is also the user called out in the first few lines of the message.

If you're currently ignoring these cues as visual noise in practice, how do you figure out who normal (non-inline) comments are from?

The "epriestley added a comment" and/or "epriestly added inline comments" while technically having the appropriate information, is pretty much visual noise that as a reader of a huge number of these emails you just sift over and want to jump straight to the meat of the matter. These changes will make said meat of the matter a lot more prominent and trivial to jump to, but it means you want the identity to be *right there* as well.

These are the transactions that occur, which mirror notifications on the site, and in the timeline. It's how we consistently frame the user content following in every case. I'm not sure there is anything gained here by removing it, and it's likely to cause user confusion (or pitchforks). When I get a DM on Twitter, I get an email saying "You have a new Direct Message" as the first sentence. Given the large number, and varied types of email we send, framing the content with a short sentence seems a reasonable practice.

Like Evan notes above, we also want to frame the entire email as from this person, and what they did.

This feedback is also sort of hard for me to understand.

On the one hand, you raise some concerns about "visual noise", difficulty jumping to "the meat of the matter", and content not being "right there". I interpret these a generally a request for more concise, efficient emails.

But then your request (and, to some degree, this task as a whole) seems to be to add redundant information to the mail (information already available in the mail body and metadata), and to add multiple copies of that information. That seems like a contradictory request to the general tone of the feedback.

I wonder if maybe you're having one kind of experience and users you're talking to are having a different experience, or something is being lost in translation?

That is, I'd expect feedback like this:

Too much visual noise: remove useless sections X, Y, Z.

Or like this:

Not enough information in the email: add X, Y, Z.

But this is roughly:

Too much visual noise, add X, Y, Z.

It's not clear to me what the root problem here is, I guess?

Of course, it's possible to have both too much useless information and not enough useful information, but there just isn't all that much to these mails that isn't pure, mandatory content.

And this task as a whole has made getting to the meat of the matter harder: we replaced a block of purely new content, which was approximately the most concise, efficient possible representation, with a series of larger and less informationally dense UI elements.

As a thought experiment, if we sent one email per inline comment like GitHub does (and just repeated the author information several times in the body or something), I think that would technically satisfy all the points you raise:

This would get directly to the meat of the matter, present all the information immediately, provide all identity/context information for every action, remove the need to jump anywhere, etc. If I step back and think about how to solve the problem you describe outside of the context of this task, "send one email for every individual action" seems like a pretty good fit and like it ticks all the boxes. Is that actually desirable?

(I think this is broadly similar to the original discussion on T10283, which also framed problems in terms of efficiency but also appeared to enable every inefficient/low-signal option available.)

A very specific example of my confusion is the decision to place the link to the revision first in the mockup on T10283, and how it contrasts with the stated problem of triage efficiency:

This link has zero information on it own and can never help you triage an incoming mail, but it is moved to the most prominent location.

The actual layout of the mail is basically already aimed at the goal of making triage efficient. Broadly, mail looks like:

• One-word summary of the content in the mail subject.
• One-line summary of each contained action at the top of the mail.
• Content of each contained action.
• Link to object.
• Common footer material that is rarely looked at.

Each section builds on the previous section by providing you more details and information to help you triage. As currently constructed, some mail can be triaged by looking only at the action in the subject; some can be triaged by reading only the first few sentences; almost all can be triaged without reading as far as the link. There is essentially no case where I skip over sections when working with mail: the sections are already presented in order from most general to most specific, finally culminating in a link to the full object.

When you get through all the sections, you've failed to triage based on available information in the mail so you're given a link to the full object to continue with full context and history.

In contrast, the mock above and discussion on T10283 wants to remove the one-word summary from the subject (so mail must always be opened to be triaged) and move the one-line summaries to the bottom of the mail (so you can never make a triage decision based only on the opening lines).

It moves the link from after the specifics section of the mail ("you've gotten this far without triaging, continue to the full object") to the very first line, but the link never has triaging power on its own.

It disables (?) "Summary", which normally provides a concise, human-readable description of a revision, and enables the entire content of the change, which is extremely inefficient as a triage tool.

I'm not sure what the story behind T10283 is, since it configures a lot of options which modify how mail is laid out, but the expectation is that mail is already organized in the best order for making triaging decisions easy. But the triage flow is to read the whole mail from the beginning, top to bottom, starting with the subject and ending with a link to the object.

It sounds like this flow is very different from the flow you may be used to, which I guess is to jump to the bottom of the mail and then jump back up to somewhere in the middle? If you're used to doing this, maybe this also explains why the comment byline is missed? This sort of layout objectively doesn't make any sense unless you constrain "everyone is already used to doing it that way", but it sort of seems like the mock in T10283 is optimizing for it? Summary information is at the bottom, other information is placed arbitrarily in the middle of the mail but called out with a color, the top has mostly-useless information, and overall the mail reads backwards with the most general information at the bottom and the link to the full object in the top?

To the degree that this is really the problem we're solving -- "everyone is used to reading mail backwards because we've been using ReviewBoard which has a nonsense order of section specificity, but Phabricator mail is organized forward" -- maybe some larger change in the vein of mail templates so you can reverse the order of the sections is actually a better fit for the problem.

I'm one of the users whose feedback Peter has been relaying, so let me explain my situation.

First of all, the reason I felt it was confusing not to see who wrote a comment was because I didn't realize that these emails would be marked as sent by the comment author.

Unlike the workflow Evan mentioned, I don't want to have to read linearly through a summary email before going to the website. I want to go to the website as soon as I decide that the code review action is something I care about right now. I expect the email to offer me the following things:

1. An easy way to access the code review without reading the whole email. If I'm going to read the code review on the web anyway, I don't want to have to read through information in the email that I'll see on the web anyway. The current standard Phabricator email format makes this hard because I have to either read the whole email or remember to scroll down and click on the second to last link.

2. A quickly scannable preview that lets me decide whether to click on the link to the code review to actually catch up on the recent comments/changes.
   • If I know based on the title that this is something I want to look at right away, then I shouldn't have to read the rest.
   • On the other hand, if the action didn't involve changes to or comments on the code, I should know this right away, so I don't bother going to the website, since there's nothing new to see there.
   • In general, the better the email is visually structured, the less I have to read to decide whether to go to the website.

3. If I don't currently have access to the code review website (e.g., I'm at lunch or riding public transit), I want to be able to read a bit more about what happened, so I can start thinking about it, or possibly even send a (perhaps preliminary) reply via email if necessary. This is why the list of comments comes after the link to the website for me.

On the subject of multiple emails, the problem is multiple comments in a single batch of comments may be inter-related, and obviously there's more visual overhead in dealing with multiple emails.

Would putting a large clickable "view thing" button in HTML email like the mock in T6600 potentially help address points (1) and (2) -- something like this?

                         New button, links to revision ------+
                                                             |
                                                             V
+----------------------------------------------------------------------+
| alincoln created a revision.                       [ VIEW REVISION ] |
| alincoln added reviewers: usgrant, gwashington.                      |
+----------------------------------------------------------------------+

I'm not sure if we can do that in a way that works gracefully on both phones and desktops since I think our ability to do responsive design in HTML email is pretty limited, but at least on desktops we usually have a lot of extra room in that area of the mail.

In T10694#174460, @epriestley wrote:

Would putting a large clickable "view thing" button in HTML email like the mock in T6600 potentially help address points (1) and (2) -- something like this?

Yes, something like that which makes it easy, predictable and obvious how to get to the review would be nice.

@epriestley Thats good to know. Also, will this change include formatting the Change Details section of emails? That one is currently a blob of visually unparseable text even in HTML emails.

I'm not sure what the story behind T10283 is, since it configures a lot of options which modify how mail is laid out, but the expectation is that mail is already organized in the best order for making triaging decisions easy. But the triage flow is to read the whole mail from the beginning, top to bottom, starting with the subject and ending with a link to the object.

The problem is that a user will not read the entire email from top to bottom. There is far too much information to do that on every email that gets sent, and most of it is essentially boiler plate.

Specifically about the order - we have very consistent feedback from several different users (including "ourselves") that they want the link to the Differential revision towards the top. This is the reason for it being on top in T10283.

While it can be argued that we're used to it being on top due to ReviewBoard, several of us have now been using Phabricator almost exclusively for a couple of months or more and it is still constantly annoying. Easy triage is important, but for the cases where you do decide that you want to hit the web UI, you want to do that trivially. This is exacerbated by particular intricacies of gmail which means that in order to actually get to the revision you end up having to:

• scroll down until gmail starts folding the rest of the email
• click to unfold the rest of the email
• click the link

Outside of the initial push notification, this can hit in cases where you use your email inbox to search for a recent change. Once you find the thread, you go through that process to jump over to the revision. To the extent that for at least myself, I find myself sometimes just opening a tab and typing the Dnnn number manually instead of mousing around. That will quickly stop working well however once Dnnn numbers grow large enough :)

In contrast, the mock above and discussion on T10283 wants to remove the one-word summary from the subject (so mail must always be opened to be triaged) and move the one-line summaries to the bottom of the mail (so you can never make a triage decision based only on the opening lines).

That is not the intent. The intent in T10283 was to focus specifically on *keeping* the summary information while still cutting down on noise. The ad-hoc example given there was to turn this:

[Differential] [Request, 4 lines] D69: [phabricator] important stuff

Into this:

[Diff,D69] important stuff

While that was a fairly ad-hoc example, the point is that it would retain the most useful information (the human-written description), and cut down on the horizontal screen real estate used to communicate boilerplate while still keeping the Dnnn number in the subject.

Finally, I just want to emphasize that the mock in T10283 was not meant to be a suggestion for the ultimate format - it was limited to some low-hanging fruit mostly dictated by my own lack of familiarity with tools you'd normally use for a more professional mock-up. As a result, it was mostly about re-ordering and filtering and slight visual changes on the diff (which is already covered much better here by the work you've already done).

In conclusion, I'm not sure to what extent things are still confusion/unclear given the above plus Sameer's earlier chime-in.

Finally:

Would putting a large clickable "view thing" button in HTML email like the mock in T6600 potentially help address points (1) and (2) -- something like this?

As I understand it - yes. However, merely re-ordering fields would get us pretty far ahead with respect to this issue, though I understand you may have tensions there that cause it not to be desirable.

The problem is that a user will not read the entire email from top to bottom. There is far too much information to do that on every email that gets sent, and most of it is essentially boiler plate.

Maybe this is the root of the disconnect -- I read every email in every Phabricator application from the top toward the bottom, stopping when I have enough information to act on it (click the link into the web UI or move on to the next email). I consider email already well-optimized for this use case already and feel like my own workflow which does this is efficient.

I can't immediately identify any boilerplate or low-density information in email.

• scroll down until gmail starts folding the rest of the email
• click to unfold the rest of the email
• click the link

I can definitely understand that this could make things feel slow and inefficient. I don't use Gmail as my primary client, but I can't immediately reproduce this, I think -- for example, here's a very long email which has not been folded by Gmail:

I can get it to fold some stuff, but it looks like only (?) non-head messages in a thread, so you should already have scrolled past at least one un-folded link?

We might be able to change the markup to try to trick Gmail into not folding where it currently folds, although I can't immediately find any information on the algorithm.

Actually, it looks like it may actually be folding identical message suffixes, so we could possibly just put some random invisible text after the object link to make sure that no duplicate suffix of the mail body includes the object link.

While that was a fairly ad-hoc example, the point is that it would retain the most useful information (the human-written description), and cut down on the horizontal screen real estate used to communicate boilerplate while still keeping the Dnnn number in the subject.

The information that I consider useful which the proposal drops is [Request], which becomes [Requested Changes], [Accepted], [Closed], etc. In particular, [Accepted] and [Closed] are strong signals that the message doesn't need immediate attention, especially, say, if you get a desktop notification about it. In other applications, notifications like [Changed Project Column], and [Retitled] identify simple, low-content notifications that can be quickly scanned and archived/ignored.

In my client, I actually see the subject action before I see the subject:

merely re-ordering fields would get us pretty far ahead with respect to this issue

What's your ideal field order?

In T10694#174693, @jcarrillo7 wrote:

@epriestley Thats good to know. Also, will this change include formatting the Change Details section of emails? That one is currently a blob of visually unparseable text even in HTML emails.

See D15901.

• Patches from metamta.differential.inline-patches should be formatted more nicely in HTML mail at HEAD.
• metamta.differential.unified-comment-context has been removed and we now raise setup guidance about it.
• The View Revision button is functional locally, but waiting on design once @chad gets back from vacation next week, since my version of it looks a lot like I designed it (D15884).
• Swapping HTML mail to become the default is ready (D15885) but waiting on the button and maybe a few other designey tweaks we want to consider.
• There's an hour or so of time across all of that which I haven't tabulated yet, but I'll settle it up once those changes actually land.

Inlined patches are now highlighted more nicely in HTML mail.

Revision mail now has a "View Revision" button. We'll extend this to other mail eventually and probably remove the standalone body links, but want to see how it feels for a bit first.

HTML mail is now sent by default. Users who prefer to continue receiving plain text mail can configure their preference in Settings.

These changes are live on this server (secure).

I don't think we have any further changes in this vein planned at this time, so I'm pausing this to let it settle and collect feedback.

This feature is very important to me, so I've been following along. However perhaps I'm missing something and cannot get the new functionality to work as I expect.

What I'm seeing:

• email inline diff is html
• there is no colouring, eg.

  <table><tr><td style="">ari committed rC43055: Remove unused old code.</td></tr></table><br /><div><div><p>Remove unused old code</p></div></div><br /><div><strong>AFFECTED FILES</strong><div><div>/angel/private-resources/scripts/cce/JournalExportScript.groovy</div></div></div><br /><div>PATCH<br />
<br />
  Index: angel/private-resources/scripts/cce/JournalExportScript.groovy<br />
  ===================================================================<br />
  --- angel/private-resources/scripts/cce/JournalExportScript.groovy	(revision 43054)<br />
  +++ angel/private-resources/scripts/cce/JournalExportScript.groovy	(revision 43055)<br />
  @@ -246,21 +246,6 @@<br />
     isNotZero ? &#039;SALE&#039; : &#039;FREE&#039;<br />
 }<br />
 <br />
  -def getAnalysisCode(context, t) {<br />

I'm on the master branch and updated just an hour ago, so from my reading of these commits I think I should have all the code in place. Is there an additional PHP dependency or something else that's needed?

The commits are in svn if that makes a difference, and I've tried both the git and unified types of patch (not sure if that effects only the attachment or also inline).

This task is for inline comments on a diff in Differential.

I thought I tried that too, but perhaps I'm confused between differential and diffusion (such very confusing terminology for people new to phabricator!). I'm making inline comments against the commits, not against code reviews.

Should I raise a new task for extending the work here to diffusion? Or is that part of the scope of this task?

It should probably go under T10978, I believe. I imagine it would get it for free, but worth mentioning over there nonetheless.

Note that this feature (text mode unified diff) is crucial for our deployment (requirement for text-only mails to contain reasonable diff snippets).

I don't understand part of the original argument here:
"this has limited utility when a comment is a reply, rather than a top-level comment: the parent comment is a more relevant piece of context."
The unified diff feature had the parent comment as primary context; it was merely "simulating" folks doing replies from their email programs instead of using the web UI.

Given that, we'll continue to develop this back into our own phab fork - I assume you'll not be willing to accept patches that (re-)introduce patches for folks that want review styles that look like emails going back and forth?

@klimek Your install is the only install we're aware of that is interested in a rendering mode like that (feedback about the old rendering was generally negative; feedback about the new rendering has been generally positive). Unless we become aware of much wider interest in such a rendering mode, I don't plan to reintroduce it upstream.

I think I phrased the "limited utility" note poorly and the primary concern was more large amount of potentially irrelevant information, "low signal-to-noise ratio" might be a better description.

@epriestley - makes sense; I've been approached by some folks about our setup because they wanted kernel devs to get an actual code review system, who apparently have a similar mindset to compiler developers, but that's probably a long shot anyway :)

The code is a little better factored now and it's possible that the patch to DifferentialInlineCommentMailView is a pretty small one (and it may work in both text and HTML modes, as a possible benefit, although I suspect your install's use of HTML mail is much lower than most).

Obviously more work than the code already being in the upstream, but I think you can possibly tweak this block a little bit to get a rendering mode out of it that's similar to the old one:

...
if ($parent_phid) {
  $parent = idx($parents, $parent_phid);
  if ($parent) {
    $context_text = $this->renderInline($parent, false, true);
    $context_html = $this->renderInline($parent, true, true);
  }
} else {
  $patch_text = $this->getPatch($hunk_parser, $comment, false);
  $context_text = $this->renderPatch($comment, $patch_text, false);

  $patch_html = $this->getPatch($hunk_parser, $comment, true);
  $context_html = $this->renderPatch($comment, $patch_html, true);
}
...

Reorganizing that to do something like:

• Always render the patch.
• For each parent, render the parent.
• Render the comment.

...with some tweaks around loading and indent levels might get you somewhere reasonable.

Of course, the old patch probably adapts back into the Editor fairly cleanly, too.