I like phab, but I need better diffs in email, and will code it myself if you show me the code...how hard could it be

https://github.com/facebook/phabricator/

There's an old implementation that just doesn't have any calls but probably mostly works:

https://secure.phabricator.com/diffusion/P/browse/master/src/applications/differential/parser/changeset/DifferentialChangesetParser.php;900190b2fe3a3720$1639

Alternatively we could manually mangle the diff by inserting code here and making the root call $mail->setIsHTML(true) or setHTML(true) or whatever the method is:

https://secure.phabricator.com/diffusion/P/browse/master/src/applications/differential/mail/reviewrequest/DifferentialReviewRequestMail.php;900190b2fe3a3720$136

The major problem this will cause is that replying to these mails is very unlikely to work well. Separating plain text reply content from quoted and HTML content is probably extremely challenging (there's some confirmation of this from people who have tried, on T185). We should probably drop the reply capability entirely when mail is marked HTML.

I'm okay with pursuing this myself soonish since I don't think it's too complicated, but that's likely to be the price for HTML mail.

Thanks for the analysis. Will think about it and get back to you on the
ticket.

Although maybe it's not really that bad, I assume most users just want to !accept stuff and we can probably make that work okay even if we have to have some heuristic that gives up on the rest of it.

yeah, that's my guess as well, they just want !accept/comments at the top
to work.

In case you all want to follow along, or dive in and knock off the task...

D9375 works reasonably well, but need to verify that it doesn't break reply-by-email in popular email client (see differential review for details)

◀ Merged tasks: T5783.

I think we can move this forward, but want to be slightly more gradual about it:

• In the new Settings > Email Format screen, add a new user preference.
  • Options are: Use Server Default (Plain Text) -- this is not configurable for now, Send Plain Text Email and Send HTML Email.
• For v1, let's just do some simple HTML to get the basic infrastructure in, like making the headers stand out and making links work. The infrastructure stuff in D9375 mostly looks good to me, although I have some minor feedback once we get there.
• For v2, we should render remarkup blocks.
• For v3, we can add the actual diffs and inlines and whatever.

• Cool, I'll add the formatting option.
• Correctly rendering remarkup blocks might be a little hairy, as it requires inlining CSS. Maybe we need to teach PhutilRemarkupBlockRule how to render tags for email, or figure out some post-processing css inline pass?

Yeah, it's probably going to be a mess. I think adding a third rendering target (Email With Inline CSS vs HTML vs Text) might be easier than trying to do post-processing safely, although maybe post-processing isn't too bad. XSS isn't too threatening in email, and we can trust that the input is well-formed, so maybe we can just look for class="..." inside tags and replace that with style="...". Maybe worth taking a shot at first, and we can add a third target if it's a huge gross mess?

Yeah, this needs some poking around, although I'm leaning towards teaching PhutilRemarkupBlockRule how to render things for email.
There's just one render target right now I think. The text rendering just dumps the remarkup as is.

I have yet to see a post-processing solution that I was happy with though. We may also need to render certain things a little differently for emails.
This is actually why I started off with the just doing the diff. It was easy and it was immediately useful.

What if we initially rolled it out without remarkup rendering, so we can gather some feedback?

Yeah, I think a v1 with no remarkup rendering is totally fine.

I mean, make v1 also do diffs (because they are so easy compared to remarkup), which is pretty much D9375 plus minor fixes plus per-user config. I can also rip that part out if you'd prefer, although we found it to be useful.

If you want to do the diff thing before remarkup that's OK, but I think it should be separate from the basic infrastructure patch.

Cool. I'll chop em up and send patches.

Add myself as a follower.

◀ Merged tasks: T5972.