From 9564b0a40e881f62404de9958bd72a9abb299a5a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 5 Mar 2015 14:11:51 -0800 Subject: [PATCH] Improve behavior of inline rendering with unified views Summary: Ref T2009. This reduces how buggy inlines are. They're still buggy. Specifically, the inline endpoint didn't know how to scaffold inlines before, so some of them ended up rendering in the wrong rows or breaking layouts. This passes the current renderer through to the inline editor endpoint, so it can at least get the layout correct. Test Plan: Interacted with inlines in unified and side-by-side views. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T2009 Differential Revision: https://secure.phabricator.com/D11988 --- resources/celerity/map.php | 42 +++++++------- .../DifferentialInlineCommentEditView.php | 31 ++++++++-- .../view/DifferentialInlineCommentView.php | 33 +++++++++-- .../PhabricatorInlineCommentController.php | 58 +++++++++++-------- .../DifferentialInlineCommentEditor.js | 6 +- .../behavior-edit-inline-comments.js | 3 + 6 files changed, 117 insertions(+), 56 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 4bc2103883..e8c1dbe220 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ return array( 'core.pkg.js' => 'a77025a1', 'darkconsole.pkg.js' => '8ab24e01', 'differential.pkg.css' => '6641cdd5', - 'differential.pkg.js' => 'e260829c', + 'differential.pkg.js' => '3fab5259', 'diffusion.pkg.css' => '591664fa', 'diffusion.pkg.js' => 'bfc0737b', 'maniphest.pkg.css' => '68d4dd3d', @@ -361,13 +361,13 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63', 'rsrc/js/application/differential/ChangesetViewManager.js' => 'fce415a0', - 'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => '6a049cf7', + 'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => '41060c54', 'rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js' => 'e10f8e18', 'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d', 'rsrc/js/application/differential/behavior-comment-preview.js' => '6932def3', 'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1', 'rsrc/js/application/differential/behavior-dropdown-menus.js' => '2035b9cb', - 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => 'f159658c', + 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => 'ae8e9b44', 'rsrc/js/application/differential/behavior-keyboard-nav.js' => '2c426492', 'rsrc/js/application/differential/behavior-populate.js' => '8694b1df', 'rsrc/js/application/differential/behavior-show-field-details.js' => 'bba9eedf', @@ -520,7 +520,7 @@ return array( 'conpherence-widget-pane-css' => '3d575438', 'differential-changeset-view-css' => 'bad09138', 'differential-core-view-css' => '7ac3cabc', - 'differential-inline-comment-editor' => '6a049cf7', + 'differential-inline-comment-editor' => '41060c54', 'differential-results-table-css' => '181aa9d9', 'differential-revision-add-comment-css' => 'c478bcaa', 'differential-revision-comment-css' => '48186045', @@ -569,7 +569,7 @@ return array( 'javelin-behavior-differential-comment-jump' => '4fdb476d', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', 'javelin-behavior-differential-dropdown-menus' => '2035b9cb', - 'javelin-behavior-differential-edit-inline-comments' => 'f159658c', + 'javelin-behavior-differential-edit-inline-comments' => 'ae8e9b44', 'javelin-behavior-differential-feedback-preview' => '6932def3', 'javelin-behavior-differential-keyboard-navigation' => '2c426492', 'javelin-behavior-differential-populate' => '8694b1df', @@ -1065,6 +1065,14 @@ return array( 'phuix-action-list-view', 'phuix-action-view', ), + '41060c54' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-request', + 'javelin-workflow', + ), 42126667 => array( 'javelin-behavior', 'javelin-dom', @@ -1238,14 +1246,6 @@ return array( '69adf288' => array( 'javelin-install', ), - '6a049cf7' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-request', - 'javelin-workflow', - ), '6c2b09a2' => array( 'javelin-install', 'javelin-util', @@ -1644,6 +1644,14 @@ return array( 'javelin-util', 'phabricator-prefab', ), + 'ae8e9b44' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-util', + 'javelin-vector', + 'differential-inline-comment-editor', + ), 'b1f0ccee' => array( 'javelin-install', 'javelin-dom', @@ -1874,14 +1882,6 @@ return array( 'javelin-install', 'javelin-util', ), - 'f159658c' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-util', - 'javelin-vector', - 'differential-inline-comment-editor', - ), 'f24f3253' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/differential/view/DifferentialInlineCommentEditView.php b/src/applications/differential/view/DifferentialInlineCommentEditView.php index 453175d75a..dabf8144f7 100644 --- a/src/applications/differential/view/DifferentialInlineCommentEditView.php +++ b/src/applications/differential/view/DifferentialInlineCommentEditView.php @@ -8,6 +8,16 @@ final class DifferentialInlineCommentEditView extends AphrontView { private $onRight; private $number; private $length; + private $renderer; + + public function setRenderer($renderer) { + $this->renderer = $renderer; + return $this; + } + + public function getRenderer() { + return $this->renderer; + } public function addHiddenInput($key, $value) { $this->inputs[] = array($key, $value); @@ -60,10 +70,17 @@ final class DifferentialInlineCommentEditView extends AphrontView { $this->renderBody(), )); - return phutil_tag('table', array(), phutil_tag( - 'tr', - array('class' => 'inline-comment-splint'), - array( + if ($this->renderer == '1up') { + $cells = array( + phutil_tag('th', array()), + phutil_tag('th', array()), + phutil_tag( + 'td', + array('colspan' => 3, 'class' => 'right3'), + $content), + ); + } else { + $cells = array( phutil_tag('th', array()), phutil_tag( 'td', @@ -74,7 +91,11 @@ final class DifferentialInlineCommentEditView extends AphrontView { 'td', array('colspan' => 3, 'class' => 'right3'), $this->onRight ? $content : null), - ))); + ); + } + + $row = phutil_tag('tr', array('class' => 'inline-comment-splint'), $cells); + return phutil_tag('table', array(), $row); } private function renderInputs() { diff --git a/src/applications/differential/view/DifferentialInlineCommentView.php b/src/applications/differential/view/DifferentialInlineCommentView.php index 074d202405..dc67e7945f 100644 --- a/src/applications/differential/view/DifferentialInlineCommentView.php +++ b/src/applications/differential/view/DifferentialInlineCommentView.php @@ -10,6 +10,7 @@ final class DifferentialInlineCommentView extends AphrontView { private $editable; private $preview; private $allowReply; + private $renderer; public function setInlineComment(PhabricatorInlineCommentInterface $comment) { $this->inlineComment = $comment; @@ -52,6 +53,15 @@ final class DifferentialInlineCommentView extends AphrontView { return $this; } + public function setRenderer($renderer) { + $this->renderer = $renderer; + return $this; + } + + public function getRenderer() { + return $this->renderer; + } + public function render() { $inline = $this->inlineComment; @@ -251,11 +261,20 @@ final class DifferentialInlineCommentView extends AphrontView { return $markup; } - $left_markup = !$this->onRight ? $markup : ''; - $right_markup = $this->onRight ? $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 : ''; - return phutil_tag('table', array(), - phutil_tag('tr', array(), array( + $cells = array( phutil_tag('th', array()), phutil_tag('td', array('class' => 'left'), $left_markup), phutil_tag('th', array()), @@ -263,7 +282,11 @@ final class DifferentialInlineCommentView extends AphrontView { 'td', array('colspan' => 3, 'class' => 'right3'), $right_markup), - ))); + ); + } + + $row = phutil_tag('tr', array(), $cells); + return phutil_tag('table', array(), $row); } } diff --git a/src/infrastructure/diff/PhabricatorInlineCommentController.php b/src/infrastructure/diff/PhabricatorInlineCommentController.php index 09f3528ff3..be165f2063 100644 --- a/src/infrastructure/diff/PhabricatorInlineCommentController.php +++ b/src/infrastructure/diff/PhabricatorInlineCommentController.php @@ -19,6 +19,7 @@ abstract class PhabricatorInlineCommentController private $commentText; private $operation; private $commentID; + private $renderer; public function getCommentID() { return $this->commentID; @@ -52,6 +53,14 @@ abstract class PhabricatorInlineCommentController return $this->isNewFile; } + public function setRenderer($renderer) { + $this->renderer = $renderer; + return $this; + } + + public function getRenderer() { + return $this->renderer; + } public function processRequest() { $request = $this->getRequest(); @@ -113,7 +122,6 @@ abstract class PhabricatorInlineCommentController return id(new AphrontAjaxResponse()) ->setContent($edit_dialog->render()); case 'create': - $text = $this->getCommentText(); if (!$request->isFormPost() || !strlen($text)) { @@ -157,6 +165,7 @@ abstract class PhabricatorInlineCommentController $edit_dialog->addHiddenInput('is_new', $is_new); $edit_dialog->addHiddenInput('number', $number); $edit_dialog->addHiddenInput('length', $length); + $edit_dialog->addHiddenInput('renderer', $this->getRenderer()); $text_area = $this->renderTextArea($this->getCommentText()); $edit_dialog->appendChild($text_area); @@ -171,27 +180,29 @@ abstract class PhabricatorInlineCommentController // NOTE: This isn't necessarily a DifferentialChangeset ID, just an // application identifier for the changeset. In Diffusion, it's a Path ID. - $this->changesetID = $request->getInt('changeset'); + $this->changesetID = $request->getInt('changeset'); - $this->isNewFile = (int)$request->getBool('is_new'); - $this->isOnRight = $request->getBool('on_right'); - $this->lineNumber = $request->getInt('number'); - $this->lineLength = $request->getInt('length'); - $this->commentText = $request->getStr('text'); - $this->commentID = $request->getInt('id'); - $this->operation = $request->getStr('op'); + $this->isNewFile = (int)$request->getBool('is_new'); + $this->isOnRight = $request->getBool('on_right'); + $this->lineNumber = $request->getInt('number'); + $this->lineLength = $request->getInt('length'); + $this->commentText = $request->getStr('text'); + $this->commentID = $request->getInt('id'); + $this->operation = $request->getStr('op'); + $this->renderer = $request->getStr('renderer'); } private function buildEditDialog() { $request = $this->getRequest(); $user = $request->getUser(); - $edit_dialog = new DifferentialInlineCommentEditView(); - $edit_dialog->setUser($user); - $edit_dialog->setSubmitURI($request->getRequestURI()); - $edit_dialog->setOnRight($this->getIsOnRight()); - $edit_dialog->setNumber($this->getLineNumber()); - $edit_dialog->setLength($this->getLineLength()); + $edit_dialog = id(new DifferentialInlineCommentEditView()) + ->setUser($user) + ->setSubmitURI($request->getRequestURI()) + ->setOnRight($this->getIsOnRight()) + ->setNumber($this->getLineNumber()) + ->setLength($this->getLineLength()) + ->setRenderer($this->getRenderer()); return $edit_dialog; } @@ -200,7 +211,7 @@ abstract class PhabricatorInlineCommentController return id(new AphrontAjaxResponse()) ->setContent( array( - 'markup' => '', + 'markup' => '', )); } @@ -222,13 +233,14 @@ abstract class PhabricatorInlineCommentController $handles = $this->loadViewerHandles($phids); - $view = new DifferentialInlineCommentView(); - $view->setInlineComment($inline); - $view->setOnRight($on_right); - $view->setBuildScaffolding(true); - $view->setMarkupEngine($engine); - $view->setHandles($handles); - $view->setEditable(true); + $view = id(new DifferentialInlineCommentView()) + ->setInlineComment($inline) + ->setOnRight($on_right) + ->setBuildScaffolding(true) + ->setMarkupEngine($engine) + ->setHandles($handles) + ->setEditable(true) + ->setRenderer($this->getRenderer()); return id(new AphrontAjaxResponse()) ->setContent( diff --git a/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js b/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js index d47ec93b13..c3e0dd9bd4 100644 --- a/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js +++ b/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js @@ -35,7 +35,8 @@ JX.install('DifferentialInlineCommentEditor', { is_new : this.getIsNew(), length : this.getLength(), changeset : this.getChangeset(), - text : this.getText() || '' + text : this.getText() || '', + renderer: this.getRenderer() }; }, _draw : function(content, exact_row) { @@ -288,7 +289,8 @@ JX.install('DifferentialInlineCommentEditor', { isNew : null, text : null, templates : null, - originalText : null + originalText : null, + renderer: null } }); diff --git a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js index 940f1e0b56..20b5e74785 100644 --- a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js +++ b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js @@ -150,6 +150,8 @@ JX.behavior('differential-edit-inline-comments', function(config) { insert = target.parentNode; } + var view = JX.ChangesetViewManager.getForNode(root); + editor = new JX.DifferentialInlineCommentEditor(config.uri) .setTemplates(config.undo_templates) .setOperation('new') @@ -160,6 +162,7 @@ JX.behavior('differential-edit-inline-comments', function(config) { .setOnRight(isOnRight(target) ? 1 : 0) .setRow(insert.nextSibling) .setTable(insert.parentNode) + .setRenderer(view.getRenderer()) .start(); set_link_state(true);