From 90364cafdcedace0ee6adbaa2550362feceff4ab Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 10 May 2011 08:29:28 -0700 Subject: [PATCH] Add comment previews to Maniphest Summary: Moves shared code from Differential and Maniphest comment previews into PhabricatorShapedRequest, and then implements Maniphest previews. This doesn't implement comment drafts, I'll follow up with that but it requires this and is completely separable. This also always shows the preview as "commented" rather than previewing the actual transaction. I'll follow up with that but I think it will require a little factoring and this is useful even without transaction details. I need to tweak the styling a bit too. Test Plan: Typed text in Maniphest and Differential. Toggled Differential action. Made comments. Reviewed By: rm Reviewers: rm, tuomaspelkonen, jungejason, aran CC: aran, rm Differential Revision: 258 --- src/__celerity_resource_map__.php | 88 ++++++++++++------- src/__phutil_library_map__.php | 2 + ...AphrontDefaultApplicationConfiguration.php | 1 + .../ManiphestTaskDetailController.php | 20 ++++- .../ManiphestTransactionPreviewController.php | 59 +++++++++++++ .../transactionpreview/__init__.php | 20 +++++ .../ManiphestTransactionDetailView.php | 16 +++- .../ManiphestTransactionListView.php | 7 ++ .../css/application/maniphest/task-detail.css | 12 +++ .../rsrc/js/application/core/ShapedRequest.js | 88 +++++++++++++++++++ .../differential/behavior-comment-preview.js | 49 ++++------- .../maniphest/behavior-transaction-preview.js | 29 ++++++ 12 files changed, 323 insertions(+), 68 deletions(-) create mode 100644 src/applications/maniphest/controller/transactionpreview/ManiphestTransactionPreviewController.php create mode 100644 src/applications/maniphest/controller/transactionpreview/__init__.php create mode 100644 webroot/rsrc/js/application/core/ShapedRequest.js create mode 100644 webroot/rsrc/js/application/maniphest/behavior-transaction-preview.js diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 7d29cdb861..c511556d8e 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -61,6 +61,16 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/css/aphront/headsup-action-list-view.css', ), + 0 => + array( + 'uri' => '/res/39de799e/rsrc/js/javelin/docs/Base.js', + 'type' => 'js', + 'requires' => + array( + 0 => 'javelin-install', + ), + 'disk' => '/rsrc/js/javelin/docs/Base.js', + ), 'aphront-list-filter-view-css' => array( 'uri' => '/res/e6cff171/rsrc/css/aphront/list-filter-view.css', @@ -284,16 +294,6 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/js/application/core/behavior-tokenizer.js', ), - 0 => - array( - 'uri' => '/res/39de799e/rsrc/js/javelin/docs/Base.js', - 'type' => 'js', - 'requires' => - array( - 0 => 'javelin-install', - ), - 'disk' => '/rsrc/js/javelin/docs/Base.js', - ), 'javelin-behavior-dark-console' => array( 'uri' => '/res/044c171f/rsrc/js/application/core/behavior-dark-console.js', @@ -350,7 +350,7 @@ celerity_register_resource_map(array( ), 'javelin-behavior-differential-feedback-preview' => array( - 'uri' => '/res/79e7f18d/rsrc/js/application/differential/behavior-comment-preview.js', + 'uri' => '/res/ab8a7d60/rsrc/js/application/differential/behavior-comment-preview.js', 'type' => 'js', 'requires' => array( @@ -359,6 +359,7 @@ celerity_register_resource_map(array( 2 => 'javelin-dom', 3 => 'javelin-request', 4 => 'javelin-util', + 5 => 'phabricator-shaped-request', ), 'disk' => '/rsrc/js/application/differential/behavior-comment-preview.js', ), @@ -462,6 +463,19 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/js/application/maniphest/behavior-transaction-controls.js', ), + 'javelin-behavior-maniphest-transaction-preview' => + array( + 'uri' => '/res/b211faf2/rsrc/js/application/maniphest/behavior-transaction-preview.js', + 'type' => 'js', + 'requires' => + array( + 0 => 'javelin-behavior', + 1 => 'javelin-dom', + 2 => 'javelin-util', + 3 => 'phabricator-shaped-request', + ), + 'disk' => '/rsrc/js/application/maniphest/behavior-transaction-preview.js', + ), 'javelin-behavior-owners-path-editor' => array( 'uri' => '/res/9cf78ffc/rsrc/js/application/owners/owners-path-editor.js', @@ -720,7 +734,7 @@ celerity_register_resource_map(array( ), 'mainphest-task-detail-css' => array( - 'uri' => '/res/dbefc148/rsrc/css/application/maniphest/task-detail.css', + 'uri' => '/res/fa248d0b/rsrc/css/application/maniphest/task-detail.css', 'type' => 'css', 'requires' => array( @@ -851,6 +865,18 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/css/core/remarkup.css', ), + 'phabricator-shaped-request' => + array( + 'uri' => '/res/1f0ef02b/rsrc/js/application/core/ShapedRequest.js', + 'type' => 'js', + 'requires' => + array( + 0 => 'javelin-install', + 1 => 'javelin-util', + 2 => 'javelin-request', + ), + 'disk' => '/rsrc/js/application/core/ShapedRequest.js', + ), 'phabricator-standard-page-view' => array( 'uri' => '/res/9c468a70/rsrc/css/application/base/standard-page-view.css', @@ -936,20 +962,6 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/33f413ef/typeahead.pkg.js', 'type' => 'js', ), - '711616db' => - array ( - 'name' => 'differential.pkg.js', - 'symbols' => - array ( - 0 => 'javelin-behavior-differential-feedback-preview', - 1 => 'javelin-behavior-differential-edit-inline-comments', - 2 => 'javelin-behavior-differential-populate', - 3 => 'javelin-behavior-differential-show-more', - 4 => 'javelin-behavior-differential-diff-radios', - ), - 'uri' => '/res/pkg/711616db/differential.pkg.js', - 'type' => 'js', - ), '7d23deb1' => array ( 'name' => 'javelin.pkg.js', @@ -993,6 +1005,20 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/ac70e6b7/core.pkg.css', 'type' => 'css', ), + 'b17926e1' => + array ( + 'name' => 'differential.pkg.js', + 'symbols' => + array ( + 0 => 'javelin-behavior-differential-feedback-preview', + 1 => 'javelin-behavior-differential-edit-inline-comments', + 2 => 'javelin-behavior-differential-populate', + 3 => 'javelin-behavior-differential-show-more', + 4 => 'javelin-behavior-differential-diff-radios', + ), + 'uri' => '/res/pkg/b17926e1/differential.pkg.js', + 'type' => 'js', + ), ), 'reverse' => array ( @@ -1016,11 +1042,11 @@ celerity_register_resource_map(array( 'diffusion-commit-view-css' => '03ef179e', 'javelin-behavior' => '7d23deb1', 'javelin-behavior-aphront-basic-tokenizer' => '33f413ef', - 'javelin-behavior-differential-diff-radios' => '711616db', - 'javelin-behavior-differential-edit-inline-comments' => '711616db', - 'javelin-behavior-differential-feedback-preview' => '711616db', - 'javelin-behavior-differential-populate' => '711616db', - 'javelin-behavior-differential-show-more' => '711616db', + 'javelin-behavior-differential-diff-radios' => 'b17926e1', + 'javelin-behavior-differential-edit-inline-comments' => 'b17926e1', + 'javelin-behavior-differential-feedback-preview' => 'b17926e1', + 'javelin-behavior-differential-populate' => 'b17926e1', + 'javelin-behavior-differential-show-more' => 'b17926e1', 'javelin-behavior-workflow' => '122a6b6d', 'javelin-dom' => '7d23deb1', 'javelin-event' => '7d23deb1', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 60cefd3aa6..571c13726f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -261,6 +261,7 @@ phutil_register_library_map(array( 'ManiphestTransactionDetailView' => 'applications/maniphest/view/transactiondetail', 'ManiphestTransactionEditor' => 'applications/maniphest/editor/transaction', 'ManiphestTransactionListView' => 'applications/maniphest/view/transactionlist', + 'ManiphestTransactionPreviewController' => 'applications/maniphest/controller/transactionpreview', 'ManiphestTransactionSaveController' => 'applications/maniphest/controller/transactionsave', 'ManiphestTransactionType' => 'applications/maniphest/constants/transactiontype', 'Phabricator404Controller' => 'applications/base/controller/404', @@ -684,6 +685,7 @@ phutil_register_library_map(array( 'ManiphestTransaction' => 'ManiphestDAO', 'ManiphestTransactionDetailView' => 'AphrontView', 'ManiphestTransactionListView' => 'AphrontView', + 'ManiphestTransactionPreviewController' => 'ManiphestController', 'ManiphestTransactionSaveController' => 'ManiphestController', 'Phabricator404Controller' => 'PhabricatorController', 'PhabricatorAuthController' => 'PhabricatorController', diff --git a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php index 0d04ec9f42..070875138b 100644 --- a/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/default/configuration/AphrontDefaultApplicationConfiguration.php @@ -159,6 +159,7 @@ class AphrontDefaultApplicationConfiguration ), 'transaction/' => array( 'save/' => 'ManiphestTransactionSaveController', + 'preview/(?P\d+)/$' => 'ManiphestTransactionPreviewController', ), 'select/search/$' => 'ManiphestTaskSelectorSearchController', ), diff --git a/src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php index 412be558e6..98e9027f12 100644 --- a/src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/taskdetail/ManiphestTaskDetailController.php @@ -268,7 +268,7 @@ class ManiphestTaskDetailController extends ManiphestController { id(new AphrontFormTextAreaControl()) ->setLabel('Comments') ->setName('comments') - ->setValue('')) + ->setID('transaction-comments')) ->appendChild( id(new AphrontFormSubmitControl()) ->setValue('Avast!')); @@ -301,10 +301,27 @@ class ManiphestTaskDetailController extends ManiphestController { ), )); + + Javelin::initBehavior('maniphest-transaction-preview', array( + 'uri' => '/maniphest/transaction/preview/'.$task->getID().'/', + 'preview' => 'transaction-preview', + 'comments' => 'transaction-comments', + )); + + $comment_panel = new AphrontPanelView(); $comment_panel->appendChild($comment_form); $comment_panel->setHeader('Leap Into Action'); + $preview_panel = + '
+
+
+ Loading preview... +
+
+
'; + $transaction_view = new ManiphestTransactionListView(); $transaction_view->setTransactions($transactions); $transaction_view->setHandles($handles); @@ -316,6 +333,7 @@ class ManiphestTaskDetailController extends ManiphestController { $panel, $transaction_view, $comment_panel, + $preview_panel, ), array( 'title' => 'T'.$task->getID().' '.$task->getTitle(), diff --git a/src/applications/maniphest/controller/transactionpreview/ManiphestTransactionPreviewController.php b/src/applications/maniphest/controller/transactionpreview/ManiphestTransactionPreviewController.php new file mode 100644 index 0000000000..ec8082b19e --- /dev/null +++ b/src/applications/maniphest/controller/transactionpreview/ManiphestTransactionPreviewController.php @@ -0,0 +1,59 @@ +id = $data['id']; + } + + public function processRequest() { + + $request = $this->getRequest(); + $user = $request->getUser(); + + $comments = $request->getStr('comments'); + + $handles = id(new PhabricatorObjectHandleData(array($user->getPHID()))) + ->loadHandles(); + + $transaction = new ManiphestTransaction(); + $transaction->setAuthorPHID($user->getPHID()); + $transaction->setComments($comments); + $transaction->setTransactionType(ManiphestTransactionType::TYPE_NONE); + + $transactions = array(); + $transactions[] = $transaction; + + $factory = new DifferentialMarkupEngineFactory(); + $engine = $factory->newDifferentialCommentMarkupEngine(); + + $transaction_view = new ManiphestTransactionListView(); + $transaction_view->setTransactions($transactions); + $transaction_view->setHandles($handles); + $transaction_view->setUser($user); + $transaction_view->setMarkupEngine($engine); + $transaction_view->setPreview(true); + + return id(new AphrontAjaxResponse()) + ->setContent($transaction_view->render()); + } + +} diff --git a/src/applications/maniphest/controller/transactionpreview/__init__.php b/src/applications/maniphest/controller/transactionpreview/__init__.php new file mode 100644 index 0000000000..75371bc120 --- /dev/null +++ b/src/applications/maniphest/controller/transactionpreview/__init__.php @@ -0,0 +1,20 @@ +transactions = $transactions; @@ -38,6 +39,11 @@ class ManiphestTransactionDetailView extends AphrontView { return $this; } + public function setPreview($preview) { + $this->preview = $preview; + return $this; + } + public function renderForEmail($with_date) { $this->forEmail = true; @@ -107,7 +113,7 @@ class ManiphestTransactionDetailView extends AphrontView { if (strlen($comments)) { $comments = $this->markupEngine->markupText($comments); $comment_transaction->setCache($comments); - if ($comment_transaction->getID()) { + if ($comment_transaction->getID() && !$this->preview) { $comment_transaction->save(); } } @@ -120,6 +126,12 @@ class ManiphestTransactionDetailView extends AphrontView { $comment_block = null; } + if ($this->preview) { + $timestamp = 'COMMENT PREVIEW'; + } else { + $timestamp = phabricator_format_timestamp($transaction->getDateCreated()); + } + return phutil_render_tag( 'div', array( @@ -129,7 +141,7 @@ class ManiphestTransactionDetailView extends AphrontView { '
'. '
'. '
'. - phabricator_format_timestamp($transaction->getDateCreated()). + $timestamp. '
'. $descs. '
'. diff --git a/src/applications/maniphest/view/transactionlist/ManiphestTransactionListView.php b/src/applications/maniphest/view/transactionlist/ManiphestTransactionListView.php index 4c71d090a9..7ee4b38af5 100644 --- a/src/applications/maniphest/view/transactionlist/ManiphestTransactionListView.php +++ b/src/applications/maniphest/view/transactionlist/ManiphestTransactionListView.php @@ -22,6 +22,7 @@ class ManiphestTransactionListView extends AphrontView { private $handles; private $user; private $markupEngine; + private $preview; public function setTransactions(array $transactions) { $this->transactions = $transactions; @@ -43,6 +44,11 @@ class ManiphestTransactionListView extends AphrontView { return $this; } + public function setPreview($preview) { + $this->preview = $preview; + return $this; + } + public function render() { $views = array(); @@ -76,6 +82,7 @@ class ManiphestTransactionListView extends AphrontView { $view->setTransactionGroup($group); $view->setHandles($this->handles); $view->setMarkupEngine($this->markupEngine); + $view->setPreview($this->preview); $views[] = $view->render(); } diff --git a/webroot/rsrc/css/application/maniphest/task-detail.css b/webroot/rsrc/css/application/maniphest/task-detail.css index 602840d9e6..79a65d56f6 100644 --- a/webroot/rsrc/css/application/maniphest/task-detail.css +++ b/webroot/rsrc/css/application/maniphest/task-detail.css @@ -42,3 +42,15 @@ .maniphest-task-detail-core { margin-right: 265px; } + +.maniphest-transaction-preview { + background: #f0f0f0; + border-top: 1px solid #dddddd; + border-bottom: 1px solid #aaaaaa; + margin: -1em 2em 2em; + padding: 15px 20px; +} + +.maniphest-loading-text { + color: #666666; +} diff --git a/webroot/rsrc/js/application/core/ShapedRequest.js b/webroot/rsrc/js/application/core/ShapedRequest.js new file mode 100644 index 0000000000..5decf2b387 --- /dev/null +++ b/webroot/rsrc/js/application/core/ShapedRequest.js @@ -0,0 +1,88 @@ +/** + * @requires javelin-install + * javelin-util + * javelin-request + * @provides phabricator-shaped-request + * @javelin + */ + +/** + * Send requests with rate limiting and retries, in response to some application + * trigger. This is used to implement comment previews in Differential and + * Maniphest. + */ +JX.install('PhabricatorShapedRequest', { + + construct : function(uri, callback, data_callback) { + this._uri = uri; + this._callback = callback; + this._dataCallback = data_callback; + }, + + members : { + _callback : null, + _dataCallback : null, + _request : null, + _min : null, + _defer : null, + _last : null, + start : function() { + this.trigger(); + }, + + trigger : function() { + + if (this._request) { + // Waiting on a request, rate-limit. + return; + } + + if (this._min && (new Date().getTime() < this._min)) { + // Just got a request back, rate-limit. + return; + } + + this._defer && this._defer.stop(); + + var data = this._dataCallback(); + + if (this.shouldSendRequest(this._last, data)) { + this._last = data; + var request = new JX.Request(this._uri, JX.bind(this, function(r) { + this._callback(r); + + this._min = new Date().getTime() + this._rateLimit; + this._defer && this._defer.stop(); + this._defer = JX.defer(JX.bind(this, this.trigger), this._rateLimit); + })); + request.listen('finally', JX.bind(this, function() { + this._request = null; + })); + request.setData(data); + request.setTimeout(this.getFrequency()); + request.send(); + } else { + this._defer = JX.defer(JX.bind(this, this.trigger), this._frequency); + } + }, + + shouldSendRequest : function(last, data) { + if (last === null) { + return true; + } + + for (var k in last) { + if (data[k] !== last[k]) { + return true; + } + } + return false; + } + + }, + + properties : { + rateLimit : 250, + frequency : 750 + } +}); diff --git a/webroot/rsrc/js/application/differential/behavior-comment-preview.js b/webroot/rsrc/js/application/differential/behavior-comment-preview.js index 3bb48b0da5..054d598f17 100644 --- a/webroot/rsrc/js/application/differential/behavior-comment-preview.js +++ b/webroot/rsrc/js/application/differential/behavior-comment-preview.js @@ -5,52 +5,33 @@ * javelin-dom * javelin-request * javelin-util + * phabricator-shaped-request */ JX.behavior('differential-feedback-preview', function(config) { var action = JX.$(config.action); var content = JX.$(config.content); - var preview = JX.$(config.preview); - var aval = null;//action.value; - var cval = null;//content.value; - var defer = null; - var min = null; - var request = null; + var callback = function(r) { + JX.DOM.setContent(JX.$(config.preview), JX.$H(r)); + }; - function check() { - if (request || (min && (new Date().getTime() < min))) { - // Waiting on an async or just got one back, rate-limit. - return; - } + var getdata = function() { + return { + content : content.value, + action : action.value + }; + }; - defer && defer.stop(); + var request = new JX.PhabricatorShapedRequest(config.uri, callback, getdata); + var trigger = JX.bind(request, request.trigger); - if (action.value !== aval || content.value !== cval) { - aval = action.value; - cval = content.value; + JX.DOM.listen(content, 'keydown', null, trigger); + JX.DOM.listen(action, 'change', null, trigger); - request = new JX.Request(config.uri, function(r) { - preview && JX.DOM.setContent(preview, JX.$H(r)); - min = new Date().getTime() + 500; - defer && defer.stop(); - defer = JX.defer(check, 500); - }); - request.listen('finally', function() { request = null; }); - request.setData({action : aval, content : cval}); - // If we don't get a response back soon, retry on the next action. - request.setTimeout(2000); - request.send(); - } else { - defer = JX.defer(check, 2000); - } - } + request.start(); - JX.DOM.listen(content, 'keydown', null, check); - JX.DOM.listen(action, 'change', null, check); - - check(); function refreshInlinePreview() { diff --git a/webroot/rsrc/js/application/maniphest/behavior-transaction-preview.js b/webroot/rsrc/js/application/maniphest/behavior-transaction-preview.js new file mode 100644 index 0000000000..8977e8b59e --- /dev/null +++ b/webroot/rsrc/js/application/maniphest/behavior-transaction-preview.js @@ -0,0 +1,29 @@ +/** + * @provides javelin-behavior-maniphest-transaction-preview + * @requires javelin-behavior + * javelin-dom + * javelin-util + * phabricator-shaped-request + */ + +JX.behavior('maniphest-transaction-preview', function(config) { + + var comments = JX.$(config.comments); + + var callback = function(r) { + JX.DOM.setContent(JX.$(config.preview), JX.$H(r)); + }; + + var getdata = function() { + return { + comments : comments.value + }; + } + + var request = new JX.PhabricatorShapedRequest(config.uri, callback, getdata); + var trigger = JX.bind(request, request.trigger); + + JX.DOM.listen(comments, 'keydown', null, trigger); + + request.start(); +});