Here's what this looks like so far when rendered in-browser with slightly more realistic data:

I think that's better, but doesn't feel nearly as good as your mock.

Let me see if placeholder profile icons just fix everything and then maybe I'll just give in and try to build that out.

(Those would be two different emails, but are a slightly more realistic sequence of events than earlier test data was.)

Here's a with-images version:

I don't immediately love it so much that I want to go implement email images.

Probably just use two lines:

person1 wrote:
yada yada yada

person2 wrote:
yada yada yada

Also, I'd assume every conversation would have the code in context? Unclear what's going on here:

The way I've currently implemented this is to drop the code context if you're replying to an inline, under the assumption that the inline you're replying to has enough context on its own and that the use case here is "follow activity from email as a passive observer", not "jump in somewhere random in the middle of the thread and get full context".

That is, every new inline gets exactly one piece of context:

• If it's a top-level comment, that piece of context is a small diff from the code.
• If it's a reply, that piece of context is the comment it is replying to.

In particular, I wanted to avoid including the context, one or more replies, and then the new comment, because then we'll be sending email that has much more redundant/old information than new information as soon as threads build up. This could be particularly bad if we choose to include the entire thread and threads get long.

We can revisit this if you think it's a bad direction, I'm just worried that active participants who have been following the thread are going to find email less useful if what used to be one line of new content is now a diff they've already seen, one or more replies they've already read, and then the new comment.

The description in T10694 has more details. Short version is: if you reply to an inline, we currently render no code above your comment in the email, and that's explicitly by design.

Variant with "x wrote":

But there, the "x" in "x wrote" is always the same user the mail came from, because of the "exactly one piece of context / no code if that context is a comment" behavior.

It feels slightly odd to me to have code context in one inline, but not the other. I didn't pick up on why that was done. I agree that we don't need to replicate the entire conversation. Can we do both, maybe?

code snippet

(n previous comments (link?))
last comment
latest comment

It's fairly straightforward to do that technically, I just worry it's really lowering the signal-to-noise value of the mail.

Over the years, a lot of users have filed requests that amount to "put everything available on the revision page into the mail", and "context + link to thread + parent inline + new inline" feels like a big step in that direction to me. Maybe I just don't really understand this use case, but it would make the mail much less useful to me.

(One possible legitimate use case is "Phabricator is on a VPN, and I can't (or can't easily) VPN from my phone, so I can't click the link", but I've never actually seen a user report this, even though it was something that happened at Facebook for some period of time.)

I generally think you should just click the link if you want to see the revision, and the mail should primarily serve as a notification of what new information is available, so I'm trying to find a compromise here that keeps the focus on new information.

But maybe this is the wrong thing to focus on and we should just dump tons of context into every mail.

In particular, the old feature (from D10146) that I replaced in D15850 basically did this -- it rendered every inline on any line of context in every mail. Here's a simple example of what it looked like:

http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160502/157566.html

This is totally crazy to me. T10283 had a similar screenshot:

These aren't extreme edge cases or anything either with many deeply nested comments or anything -- they're fairly simple cases.

Obviously, we can do a little better than this (at least in HTML mail) but it feels like this pathway leads to ruin to me.

Maybe this is a little better?

Roughly;

• Border around the whole thing.
  • Round corners a touch; try to make it look a bit like the actual inline element in the web UI, visually (maybe even use the same colors? Maybe drop the "@" too?).
  • Improve visual clarity/parseability when an inline includes code blocks, quoted blocks, etc.
• Put a little more whitespace around inline text to pull it out better, visually.
• Greyer text for quoted text to de-emphasize it a bit and distinguish quoted from new text (maybe too grey? But the next step down looked too black to me).
• Link directly to the inline.
• Only show the filename instead of the whole path (I think this is ok 98% of the time and should be better on phones).
• Tweak colors a little, might be a bit too "epriestley design" now where everything is a grey box though?

I dropped the @ locally since it seems weird/inconsistent the more I look at it.

This probably still won't look quite right in actual mail clients but hopefully I got most of the weirdness fixed.