Page MenuHomePhabricator

Phriction document "edited" notifications should include inline diff
Closed, DuplicatePublic

Description

currently, the "[Phriction] [Edited] {DOCNAME}" notification emails only include a link to display the diff, but it would be tremendously useful to inline the diff if it is short enough. i would be more than willing to implement that, but i need some guidance:

i wrote a quick test using the included external lib "diff_match_patch":

CODE
$dmp = new diff_match_patch();
$body->addTextSection(
  pht('DOCUMENT DIFF'),
  $dmp->patch_toText($dmp->patch_make($text_old, $text_new)));

but its output is quite terrible. for example, given the input file:

SAMPLE DOCUMENT
Doc Title
=========

Lorem ipsum dolor sit amet, consectetur adipiscing elit. Phasellus placerat sem sed erat maximus varius.
Another line of stuff.

Sub Title
---------

Proin et ligula laoreet, vulputate nisi vitae, maximus lectus. Suspendisse potenti.

and changing the line Another line of stuff to Another line of text, it rendered:

OUTPUT using diff_match_patch
DOCUMENT DIFF
  @@ -139,13 +139,12 @@
    of 
  -stuff
  +text
   .%0A%0AS

yuck! this is what i would want (based on years of using unified diff...):

OUTPUT using `diff --unified`
DOCUMENT DIFF
  @@ -2,7 +2,7 @@
   =========

   Lorem ipsum dolor sit amet, consectetur adipiscing elit. Phasellus placerat sem sed erat maximus varius.
  -Another line of stuff.
  +Another line of text.

   Sub Title
   ---------

so, would you accept a patch to enable inline phriction diffs? and if so, do you have any ideas on how to get better diff output?

Event Timeline

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

https://secure.phabricator.com/book/phabcontrib/article/feature_requests/ covers what we look for in feature requests.
https://secure.phabricator.com/book/phabcontrib/article/contrib_intro/ covers how to contribute to Phabricator.

The TDLR; version is this. Code is "cheap", and lifetime of documentation, support, testing, and updating is "expensive".

The correct path here is to make "inline diffs" a user setting (it's currently an admin setting I believe) and extend that to applications other than Differential. This isn't a quick or easy task.