From 1352be827e12e18a7f7b97262e16d5b9ed94123c Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 6 Mar 2015 05:44:36 -0800 Subject: [PATCH] Begin separating inline comment scaffolding from other renderers Summary: Ref T2009. Inline comments have "scaffolding", which is basically some empty table cells/rows around them to get the layout correct. The scaffolding depends on the renderer, since the cells are different for side-by-side vs unified diffs. This is currently duplicated all over the place: - Edit view has 1up/2up. - Detail view has 1up/2up. - 1up renderer has 1up. - 2up renderer has four separate copies of the 2up logic. These all have subtle differences, which are mostly bugs. Start making the scaffolding more composable so we can get rid of that mess. Test Plan: Added, edited, and removed inline comments on unified and side-by-side diffs. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T2009 Differential Revision: https://secure.phabricator.com/D11997 --- src/__phutil_library_map__.php | 8 +++ .../DifferentialChangesetHTMLRenderer.php | 13 ++++ .../DifferentialChangesetOneUpRenderer.php | 6 +- .../DifferentialChangesetTwoUpRenderer.php | 5 ++ .../PhabricatorInlineCommentController.php | 12 +++- .../view/PHUIDiffInlineCommentDetailView.php | 45 ++---------- .../view/PHUIDiffInlineCommentEditView.php | 4 ++ .../view/PHUIDiffInlineCommentRowScaffold.php | 23 ++++++ .../PHUIDiffInlineCommentTableScaffold.php | 22 ++++++ .../diff/view/PHUIDiffInlineCommentView.php | 6 +- .../PHUIDiffOneUpInlineCommentRowScaffold.php | 33 +++++++++ .../PHUIDiffTwoUpInlineCommentRowScaffold.php | 72 +++++++++++++++++++ 12 files changed, 204 insertions(+), 45 deletions(-) create mode 100644 src/infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php create mode 100644 src/infrastructure/diff/view/PHUIDiffInlineCommentTableScaffold.php create mode 100644 src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php create mode 100644 src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f04277b708..107f61b16d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1138,7 +1138,11 @@ phutil_register_library_map(array( 'PHUICrumbsView' => 'view/phui/PHUICrumbsView.php', 'PHUIDiffInlineCommentDetailView' => 'infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php', 'PHUIDiffInlineCommentEditView' => 'infrastructure/diff/view/PHUIDiffInlineCommentEditView.php', + 'PHUIDiffInlineCommentRowScaffold' => 'infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php', + 'PHUIDiffInlineCommentTableScaffold' => 'infrastructure/diff/view/PHUIDiffInlineCommentTableScaffold.php', 'PHUIDiffInlineCommentView' => 'infrastructure/diff/view/PHUIDiffInlineCommentView.php', + 'PHUIDiffOneUpInlineCommentRowScaffold' => 'infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php', + 'PHUIDiffTwoUpInlineCommentRowScaffold' => 'infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php', 'PHUIDocumentExample' => 'applications/uiexample/examples/PHUIDocumentExample.php', 'PHUIDocumentView' => 'view/phui/PHUIDocumentView.php', 'PHUIFeedStoryExample' => 'applications/uiexample/examples/PHUIFeedStoryExample.php', @@ -4367,7 +4371,11 @@ phutil_register_library_map(array( 'PHUICrumbsView' => 'AphrontView', 'PHUIDiffInlineCommentDetailView' => 'PHUIDiffInlineCommentView', 'PHUIDiffInlineCommentEditView' => 'PHUIDiffInlineCommentView', + 'PHUIDiffInlineCommentRowScaffold' => 'AphrontView', + 'PHUIDiffInlineCommentTableScaffold' => 'AphrontView', 'PHUIDiffInlineCommentView' => 'AphrontView', + 'PHUIDiffOneUpInlineCommentRowScaffold' => 'PHUIDiffInlineCommentRowScaffold', + 'PHUIDiffTwoUpInlineCommentRowScaffold' => 'PHUIDiffInlineCommentRowScaffold', 'PHUIDocumentExample' => 'PhabricatorUIExample', 'PHUIDocumentView' => 'AphrontTagView', 'PHUIFeedStoryExample' => 'PhabricatorUIExample', diff --git a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php index dc6ab107fa..5ced1963ce 100644 --- a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php @@ -3,7 +3,20 @@ abstract class DifferentialChangesetHTMLRenderer extends DifferentialChangesetRenderer { + public static function getHTMLRendererByKey($key) { + switch ($key) { + case '1up': + return new DifferentialChangesetOneUpRenderer(); + case '2up': + default: + return new DifferentialChangesetTwoUpRenderer(); + } + throw new Exception(pht('Unknown HTML renderer "%s"!', $key)); + } + abstract protected function getRendererTableClass(); + abstract public function getRowScaffoldForInline( + PHUIDiffInlineCommentView $view); protected function renderChangeTypeHeader($force) { $changeset = $this->getChangeset(); diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php index 29a3654a74..c7cc8890c6 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -95,7 +95,6 @@ final class DifferentialChangesetOneUpRenderer $inline = $this->buildInlineComment( $p['comment'], $p['right']); - $inline->setBuildScaffolding(false); $out[] = phutil_tag( 'tr', @@ -160,4 +159,9 @@ final class DifferentialChangesetOneUpRenderer throw new PhutilMethodNotImplementedException(); } + public function getRowScaffoldForInline(PHUIDiffInlineCommentView $view) { + return id(new PHUIDiffOneUpInlineCommentRowScaffold()) + ->addInlineView($view); + } + } diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index a7b22b9d20..ebf12ba439 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -397,4 +397,9 @@ final class DifferentialChangesetTwoUpRenderer return $this->renderChangesetTable($output); } + public function getRowScaffoldForInline(PHUIDiffInlineCommentView $view) { + return id(new PHUIDiffTwoUpInlineCommentRowScaffold()) + ->addInlineView($view); + } + } diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index 912628c823..8a9bf5271c 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -114,6 +114,7 @@ abstract class PhabricatorInlineCommentController $edit_dialog->addHiddenInput('id', $this->getCommentID()); $edit_dialog->addHiddenInput('op', 'edit'); + $edit_dialog->addHiddenInput('renderer', $this->getRenderer()); $edit_dialog->appendChild( $this->renderTextArea( @@ -236,11 +237,16 @@ abstract class PhabricatorInlineCommentController $view = id(new PHUIDiffInlineCommentDetailView()) ->setInlineComment($inline) ->setOnRight($on_right) - ->setBuildScaffolding(true) ->setMarkupEngine($engine) ->setHandles($handles) - ->setEditable(true) - ->setRenderer($this->getRenderer()); + ->setEditable(true); + + $renderer = DifferentialChangesetHTMLRenderer::getHTMLRendererByKey( + $this->getRenderer()); + + $view = $renderer->getRowScaffoldForInline($view); + $view = id(new PHUIDiffInlineCommentTableScaffold()) + ->addRowScaffold($view); return id(new AphrontAjaxResponse()) ->setContent( diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index 55a35c64f4..72564df27d 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -5,7 +5,6 @@ final class PHUIDiffInlineCommentDetailView private $inlineComment; private $onRight; - private $buildScaffolding; private $handles; private $markupEngine; private $editable; @@ -13,6 +12,10 @@ final class PHUIDiffInlineCommentDetailView private $allowReply; private $renderer; + public function getIsOnRight() { + return $this->onRight; + } + public function setInlineComment(PhabricatorInlineCommentInterface $comment) { $this->inlineComment = $comment; return $this; @@ -23,11 +26,6 @@ final class PHUIDiffInlineCommentDetailView return $this; } - public function setBuildScaffolding($scaffold) { - $this->buildScaffolding = $scaffold; - return $this; - } - public function setHandles(array $handles) { assert_instances_of($handles, 'PhabricatorObjectHandle'); $this->handles = $handles; @@ -254,40 +252,7 @@ final class PHUIDiffInlineCommentDetailView phutil_tag_div('phabricator-remarkup', $content)), )); - return $this->scaffoldMarkup($markup); - } - - private function scaffoldMarkup($markup) { - if (!$this->buildScaffolding) { - return $markup; - } - - if ($this->renderer == '1up') { - $cells = array( - phutil_tag('th', array()), - phutil_tag('th', array()), - phutil_tag( - 'td', - array('colspan' => 3, 'class' => 'right3'), - $markup), - ); - } else { - $left_markup = !$this->onRight ? $markup : ''; - $right_markup = $this->onRight ? $markup : ''; - - $cells = array( - phutil_tag('th', array()), - phutil_tag('td', array('class' => 'left'), $left_markup), - phutil_tag('th', array()), - phutil_tag( - 'td', - array('colspan' => 3, 'class' => 'right3'), - $right_markup), - ); - } - - $row = phutil_tag('tr', array(), $cells); - return phutil_tag('table', array(), $row); + return $markup; } } diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php index f2ba1962e2..57c0a8fa53 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentEditView.php @@ -11,6 +11,10 @@ final class PHUIDiffInlineCommentEditView private $length; private $renderer; + public function getIsOnRight() { + return $this->onRight; + } + public function setRenderer($renderer) { $this->renderer = $renderer; return $this; diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php new file mode 100644 index 0000000000..81753995ef --- /dev/null +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentRowScaffold.php @@ -0,0 +1,23 @@ +views; + } + + public function addInlineView(PHUIDiffInlineCommentView $view) { + $this->views[] = $view; + return $this; + } + +} diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentTableScaffold.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentTableScaffold.php new file mode 100644 index 0000000000..5da25389a5 --- /dev/null +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentTableScaffold.php @@ -0,0 +1,22 @@ +rows[] = $row; + return $this; + } + + public function render() { + return phutil_tag('table', array(), $this->rows); + } + +} diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php index 636312f754..71b328ba60 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentView.php @@ -1,3 +1,7 @@ getInlineViews(); + if (count($inlines) != 1) { + throw new Exception( + pht('One-up inline row scaffold must have exactly one inline view!')); + } + $inline = head($inlines); + + $attrs = array( + 'colspan' => 3, + 'class' => 'right3', + ); + + $cells = array( + phutil_tag('th', array()), + phutil_tag('th', array()), + phutil_tag('td', $attrs, $inline), + ); + + return phutil_tag('tr', array(), $cells); + } + +} diff --git a/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php b/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php new file mode 100644 index 0000000000..29ae88daa4 --- /dev/null +++ b/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php @@ -0,0 +1,72 @@ +getInlineViews(); + + if (!$inlines) { + throw new Exception( + pht('Two-up inline row scaffold must have at least one inline view.')); + } + + if (count($inlines) > 2) { + throw new Exception( + pht('Two-up inline row scaffold must have at most two inline views.')); + } + + if (count($inlines) == 1) { + $inline = head($inlines); + if ($inline->getIsOnRight()) { + $left_side = null; + $right_side = $inline; + } else { + $left_side = $inline; + $right_side = null; + } + } else { + list($u, $v) = $inlines; + + if ($u->getIsOnRight() == $v->getIsOnRight()) { + throw new Exception( + pht( + 'Two-up inline row scaffold must have one comment on the left and '. + 'one comment on the right when showing two comments.')); + } + + if ($v->getIsOnRight()) { + $left_side = $u; + $right_side = $v; + } else { + $left_side = $v; + $right_side = $u; + } + } + + $left_attrs = array( + 'class' => 'left', + ); + + $right_attrs = array( + 'colspan' => 3, + 'class' => 'right3', + ); + + $cells = array( + phutil_tag('th', array()), + phutil_tag('td', $left_attrs, $left_side), + phutil_tag('th', array()), + phutil_tag('td', $right_attrs, $right_side), + ); + + return phutil_tag('tr', array(), $cells); + } + +}