From 35c1dbf1f8a4245296bf6229d1d36e46444b5edd Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 5 Mar 2015 14:03:57 -0800 Subject: [PATCH] Unify changeset line ID rendering and bring it to unified diffs Summary: Ref T2009. Currently, lines don't get their "C123NL456" IDs set in the unified view. This is the major way that inlines are glued to changesets. Simplify this rendering and bring it into the HTML renderer, then use it in the OneUp renderer. Test Plan: - Interacted with side-by-side inlines (hovered, added, edited, deleted), saw unchanged behavior. - Interacted with unified inlines. They still don't work, but the error that breaks them is deeper in the stack. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T2009 Differential Revision: https://secure.phabricator.com/D11983 --- .../DifferentialChangesetHTMLRenderer.php | 43 +++++++++++++++++++ .../DifferentialChangesetOneUpRenderer.php | 29 +++++++++++-- .../DifferentialChangesetTwoUpRenderer.php | 24 +++-------- 3 files changed, 73 insertions(+), 23 deletions(-) diff --git a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php index b03497560b..2520658722 100644 --- a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php @@ -525,5 +525,48 @@ abstract class DifferentialChangesetHTMLRenderer $text); } + /** + * Build the prefixes for line IDs used to track inline comments. + * + * @return pair Left and right prefixes. + */ + protected function getLineIDPrefixes() { + // These look like "C123NL45", which means the line is line 45 on the + // "new" side of the file in changeset 123. + + // The "C" stands for "changeset", and is followed by a changeset ID. + + // "N" stands for "new" and means the comment should attach to the new file + // when stored. "O" stands for "old" and means the comment should attach to + // the old file. These are important because either the old or new part + // of a file may appear on the left or right side of the diff in the + // diff-of-diffs view. + + // The "L" stands for "line" and is followed by the line number. + + if ($this->getOldChangesetID()) { + $left_prefix = array(); + $left_prefix[] = 'C'; + $left_prefix[] = $this->getOldChangesetID(); + $left_prefix[] = $this->getOldAttachesToNewFile() ? 'N' : 'O'; + $left_prefix[] = 'L'; + $left_prefix = implode('', $left_prefix); + } else { + $left_prefix = null; + } + + if ($this->getNewChangesetID()) { + $right_prefix = array(); + $right_prefix[] = 'C'; + $right_prefix[] = $this->getNewChangesetID(); + $right_prefix[] = $this->getNewAttachesToNewFile() ? 'N' : 'O'; + $right_prefix[] = 'L'; + $right_prefix = implode('', $right_prefix); + } else { + $right_prefix = null; + } + + return array($left_prefix, $right_prefix); + } } diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php index cdefbc303b..f9768766a8 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -30,6 +30,8 @@ final class DifferentialChangesetOneUpRenderer $primitives = $this->buildPrimitives($range_start, $range_len); + list($left_prefix, $right_prefix) = $this->getLineIDPrefixes(); + $out = array(); foreach ($primitives as $p) { $type = $p['type']; @@ -43,18 +45,37 @@ final class DifferentialChangesetOneUpRenderer } else { $class = 'left'; } - $out[] = phutil_tag('th', array(), $p['line']); + + if ($left_prefix) { + $left_id = $left_prefix.$p['line']; + } else { + $left_id = null; + } + $out[] = phutil_tag('th', array('id' => $left_id), $p['line']); + $out[] = phutil_tag('th', array()); $out[] = phutil_tag('td', array('class' => $class), $p['render']); - } else if ($type == 'new') { + } else { if ($p['htype']) { $class = 'right new'; $out[] = phutil_tag('th', array()); } else { $class = 'right'; - $out[] = phutil_tag('th', array(), $p['oline']); + if ($left_prefix) { + $left_id = $left_prefix.$p['oline']; + } else { + $left_id = null; + } + $out[] = phutil_tag('th', array('id' => $left_id), $p['oline']); } - $out[] = phutil_tag('th', array(), $p['line']); + + if ($right_prefix) { + $right_id = $right_prefix.$p['line']; + } else { + $right_id = null; + } + $out[] = phutil_tag('th', array('id' => $right_id), $p['line']); + $out[] = phutil_tag('td', array('class' => $class), $p['render']); } $out[] = hsprintf(''); diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index 91193fba8e..a7b22b9d20 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -55,19 +55,8 @@ final class DifferentialChangesetTwoUpRenderer $new_lines = $this->getNewLines(); $gaps = $this->getGaps(); $reference = $this->getRenderingReference(); - $left_id = $this->getOldChangesetID(); - $right_id = $this->getNewChangesetID(); - // "N" stands for 'new' and means the comment should attach to the new file - // when stored, i.e. DifferentialInlineComment->setIsNewFile(). - // "O" stands for 'old' and means the comment should attach to the old file. - - $left_char = $this->getOldAttachesToNewFile() - ? 'N' - : 'O'; - $right_char = $this->getNewAttachesToNewFile() - ? 'N' - : 'O'; + list($left_prefix, $right_prefix) = $this->getLineIDPrefixes(); $changeset = $this->getChangeset(); $copy_lines = idx($changeset->getMetadata(), 'copy:lines', array()); @@ -234,14 +223,14 @@ final class DifferentialChangesetTwoUpRenderer $html[] = $context_not_available; } - if ($o_num && $left_id) { - $o_id = 'C'.$left_id.$left_char.'L'.$o_num; + if ($o_num && $left_prefix) { + $o_id = $left_prefix.$o_num; } else { $o_id = null; } - if ($n_num && $right_id) { - $n_id = 'C'.$right_id.$right_char.'L'.$n_num; + if ($n_num && $right_prefix) { + $n_id = $right_prefix.$n_num; } else { $n_id = null; } @@ -251,9 +240,6 @@ final class DifferentialChangesetTwoUpRenderer // clipboard. See the 'phabricator-oncopy' behavior. $zero_space = "\xE2\x80\x8B"; - // NOTE: The Javascript is sensitive to whitespace changes in this - // block! - $html[] = phutil_tag('tr', array(), array( phutil_tag('th', array('id' => $o_id), $o_num), phutil_tag('td', array('class' => $o_classes), $o_text),