From b199f7066c9ce5f336a6ce493daa3e3dea913aae Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Thu, 30 Apr 2015 16:54:57 -0700 Subject: [PATCH] Conpherence - add support for linking directly to messages regardless of age of message Summary: Fixes T7757. Since anchor links can't be processed server side, we have to detect the message is old in javascript, then re-loaded the page. This opens up a new corner case where we have to paginate in newer messages, so this also adds support for that. Test Plan: - set main query limit to 8 and then visited ZXX#YYY. noted a second quick load of YYY, that YYY ended up highlighted and scrolled to. - used "show newer messages" and "show older messages" successfully, taking care to make sure transaction ids were all correct with no off by one errors, etc. - opened and closed durable column to make sure that still works too Reviewers: chad, epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T7757 Differential Revision: https://secure.phabricator.com/D12633 --- resources/celerity/map.php | 60 +++++++-------- .../ConpherenceTransactionRenderer.php | 65 ++++++++++++++--- .../PhabricatorConpherenceApplication.php | 2 + .../controller/ConpherenceViewController.php | 73 +++++++++++++++++-- .../view/ConpherenceDurableColumnView.php | 3 +- src/view/page/PhabricatorStandardPageView.php | 32 +++++--- .../application/conpherence/message-pane.css | 4 +- .../conpherence/ConpherenceThreadManager.js | 9 ++- .../application/conpherence/behavior-menu.js | 56 ++++++++++++-- 9 files changed, 237 insertions(+), 67 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index a36352f2d2..7dd232fa83 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -46,7 +46,7 @@ return array( 'rsrc/css/application/config/unhandled-exception.css' => '37d4f9a2', 'rsrc/css/application/conpherence/durable-column.css' => '2e68a92f', 'rsrc/css/application/conpherence/menu.css' => 'f389e048', - 'rsrc/css/application/conpherence/message-pane.css' => 'e7c09fda', + 'rsrc/css/application/conpherence/message-pane.css' => '3150e2a2', 'rsrc/css/application/conpherence/notification.css' => 'd208f806', 'rsrc/css/application/conpherence/transaction.css' => '25138b7f', 'rsrc/css/application/conpherence/update.css' => '1099a660', @@ -355,9 +355,9 @@ return array( 'rsrc/js/application/aphlict/behavior-aphlict-status.js' => 'ea681761', 'rsrc/js/application/auth/behavior-persona-login.js' => '9414ff18', 'rsrc/js/application/config/behavior-reorder-fields.js' => '14a827de', - 'rsrc/js/application/conpherence/ConpherenceThreadManager.js' => '0a5192c4', + 'rsrc/js/application/conpherence/ConpherenceThreadManager.js' => '6709c934', 'rsrc/js/application/conpherence/behavior-durable-column.js' => '657c2b50', - 'rsrc/js/application/conpherence/behavior-menu.js' => '077a1dab', + 'rsrc/js/application/conpherence/behavior-menu.js' => '804b0773', 'rsrc/js/application/conpherence/behavior-pontificate.js' => '21ba5861', 'rsrc/js/application/conpherence/behavior-quicksand-blacklist.js' => '7927a7d3', 'rsrc/js/application/conpherence/behavior-widget-pane.js' => '93568464', @@ -519,9 +519,9 @@ return array( 'config-welcome-css' => '6abd79be', 'conpherence-durable-column-view' => '2e68a92f', 'conpherence-menu-css' => 'f389e048', - 'conpherence-message-pane-css' => 'e7c09fda', + 'conpherence-message-pane-css' => '3150e2a2', 'conpherence-notification-css' => 'd208f806', - 'conpherence-thread-manager' => '0a5192c4', + 'conpherence-thread-manager' => '6709c934', 'conpherence-transaction-css' => '25138b7f', 'conpherence-update-css' => '1099a660', 'conpherence-widget-pane-css' => '2af42ebe', @@ -561,7 +561,7 @@ return array( 'javelin-behavior-audit-preview' => 'd835b03a', 'javelin-behavior-choose-control' => '6153c708', 'javelin-behavior-config-reorder-fields' => '14a827de', - 'javelin-behavior-conpherence-menu' => '077a1dab', + 'javelin-behavior-conpherence-menu' => '804b0773', 'javelin-behavior-conpherence-pontificate' => '21ba5861', 'javelin-behavior-conpherence-widget-pane' => '93568464', 'javelin-behavior-countdown-timer' => 'e4cc26b3', @@ -873,20 +873,6 @@ return array( 'javelin-stratcom', 'javelin-workflow', ), - '077a1dab' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-workflow', - 'javelin-behavior-device', - 'javelin-history', - 'javelin-vector', - 'javelin-scrollbar', - 'phabricator-title', - 'phabricator-shaped-request', - 'conpherence-thread-manager', - ), '07de8873' => array( 'javelin-install', 'javelin-util', @@ -902,16 +888,6 @@ return array( 'javelin-dom', 'javelin-router', ), - '0a5192c4' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - ), '0c6946e7' => array( 'javelin-install', 'javelin-dom', @@ -1311,6 +1287,16 @@ return array( 'phabricator-keyboard-shortcut', 'conpherence-thread-manager', ), + '6709c934' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-workflow', + 'javelin-router', + 'javelin-behavior-device', + 'javelin-vector', + ), '6882e80a' => array( 'javelin-dom', ), @@ -1440,6 +1426,20 @@ return array( 'javelin-behavior', 'javelin-history', ), + '804b0773' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-workflow', + 'javelin-behavior-device', + 'javelin-history', + 'javelin-vector', + 'javelin-scrollbar', + 'phabricator-title', + 'phabricator-shaped-request', + 'conpherence-thread-manager', + ), 82439934 => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/conpherence/ConpherenceTransactionRenderer.php b/src/applications/conpherence/ConpherenceTransactionRenderer.php index 616c0f79c5..d6be83fb65 100644 --- a/src/applications/conpherence/ConpherenceTransactionRenderer.php +++ b/src/applications/conpherence/ConpherenceTransactionRenderer.php @@ -5,17 +5,40 @@ final class ConpherenceTransactionRenderer { public static function renderTransactions( PhabricatorUser $user, ConpherenceThread $conpherence, - $full_display = true) { + $full_display = true, + $marker_type = 'older') { $transactions = $conpherence->getTransactions(); + $oldest_transaction_id = 0; + $newest_transaction_id = 0; $too_many = ConpherenceThreadQuery::TRANSACTION_LIMIT + 1; if (count($transactions) == $too_many) { - $last_transaction = end($transactions); - unset($transactions[$last_transaction->getID()]); - $oldest_transaction = end($transactions); - $oldest_transaction_id = $oldest_transaction->getID(); + if ($marker_type == 'olderandnewer') { + $last_transaction = end($transactions); + $first_transaction = reset($transactions); + unset($transactions[$last_transaction->getID()]); + unset($transactions[$first_transaction->getID()]); + $oldest_transaction_id = $last_transaction->getID(); + $newest_transaction_id = $first_transaction->getID(); + } else if ($marker_type == 'newer') { + $first_transaction = reset($transactions); + unset($transactions[$first_transaction->getID()]); + $newest_transaction_id = $first_transaction->getID(); + } else if ($marker_type == 'older') { + $last_transaction = end($transactions); + unset($transactions[$last_transaction->getID()]); + $oldest_transaction = end($transactions); + $oldest_transaction_id = $oldest_transaction->getID(); + } + // we need **at least** the newer marker in this mode even if + // we didn't get a full set of transactions + } else if ($marker_type == 'olderandnewer') { + $first_transaction = reset($transactions); + unset($transactions[$first_transaction->getID()]); + $newest_transaction_id = $first_transaction->getID(); } + $transactions = array_reverse($transactions); $handles = $conpherence->getHandles(); $rendered_transactions = array(); @@ -98,22 +121,24 @@ final class ConpherenceTransactionRenderer { 'latest_transaction' => $transaction, 'latest_transaction_id' => $latest_transaction_id, 'oldest_transaction_id' => $oldest_transaction_id, + 'newest_transaction_id' => $newest_transaction_id, ); } public static function renderMessagePaneContent( array $transactions, - $oldest_transaction_id) { + $oldest_transaction_id, + $newest_transaction_id) { - $scrollbutton = ''; + $oldscrollbutton = ''; if ($oldest_transaction_id) { - $scrollbutton = javelin_tag( + $oldscrollbutton = javelin_tag( 'a', array( 'href' => '#', 'mustcapture' => true, 'sigil' => 'show-older-messages', - 'class' => 'conpherence-show-older-messages', + 'class' => 'conpherence-show-more-messages', 'meta' => array( 'oldest_transaction_id' => $oldest_transaction_id, ), @@ -121,7 +146,27 @@ final class ConpherenceTransactionRenderer { pht('Show Older Messages')); } - return hsprintf('%s%s', $scrollbutton, $transactions); + $newscrollbutton = ''; + if ($newest_transaction_id) { + $newscrollbutton = javelin_tag( + 'a', + array( + 'href' => '#', + 'mustcapture' => true, + 'sigil' => 'show-newer-messages', + 'class' => 'conpherence-show-more-messages', + 'meta' => array( + 'newest_transaction_id' => $newest_transaction_id, + ), + ), + pht('Show Newer Messages')); + } + + return hsprintf( + '%s%s%s', + $oldscrollbutton, + $transactions, + $newscrollbutton); } } diff --git a/src/applications/conpherence/application/PhabricatorConpherenceApplication.php b/src/applications/conpherence/application/PhabricatorConpherenceApplication.php index c879d387a2..5d36022a04 100644 --- a/src/applications/conpherence/application/PhabricatorConpherenceApplication.php +++ b/src/applications/conpherence/application/PhabricatorConpherenceApplication.php @@ -41,6 +41,8 @@ final class PhabricatorConpherenceApplication extends PhabricatorApplication { '' => 'ConpherenceListController', 'thread/(?P[1-9]\d*)/' => 'ConpherenceListController', '(?P[1-9]\d*)/' => 'ConpherenceViewController', + '(?P[1-9]\d*)/(?P[1-9]\d*)/' + => 'ConpherenceViewController', 'columnview/' => 'ConpherenceColumnViewController', 'new/' => 'ConpherenceNewController', 'room/new/' => 'ConpherenceNewRoomController', diff --git a/src/applications/conpherence/controller/ConpherenceViewController.php b/src/applications/conpherence/controller/ConpherenceViewController.php index c78ba5b00c..849434f59f 100644 --- a/src/applications/conpherence/controller/ConpherenceViewController.php +++ b/src/applications/conpherence/controller/ConpherenceViewController.php @@ -3,6 +3,8 @@ final class ConpherenceViewController extends ConpherenceController { + const OLDER_FETCH_LIMIT = 5; + public function handleRequest(AphrontRequest $request) { $user = $request->getUser(); @@ -15,19 +17,43 @@ final class ConpherenceViewController extends ->withIDs(array($conpherence_id)) ->needParticipantCache(true) ->needTransactions(true) - ->setTransactionLimit(ConpherenceThreadQuery::TRANSACTION_LIMIT); + ->setTransactionLimit($this->getMainQueryLimit()); + $before_transaction_id = $request->getInt('oldest_transaction_id'); + $after_transaction_id = $request->getInt('newest_transaction_id'); + $old_message_id = $request->getURIData('messageID'); + if ($before_transaction_id && ($old_message_id || $after_transaction_id)) { + throw new Aphront400Response(); + } + if ($old_message_id && $after_transaction_id) { + throw new Aphront400Response(); + } + + $marker_type = 'older'; if ($before_transaction_id) { $query ->setBeforeTransactionID($before_transaction_id); } + if ($old_message_id) { + $marker_type = 'olderandnewer'; + $query + ->setAfterTransactionID($old_message_id - 1); + } + if ($after_transaction_id) { + $marker_type = 'newer'; + $query + ->setAfterTransactionID($after_transaction_id); + } + $conpherence = $query->executeOne(); if (!$conpherence) { return new Aphront404Response(); } $this->setConpherence($conpherence); - $transactions = $conpherence->getTransactions(); + $transactions = $this->getNeededTransactions( + $conpherence, + $old_message_id); $latest_transaction = head($transactions); $participant = $conpherence->getParticipantIfExists($user->getPHID()); if ($participant) { @@ -38,11 +64,14 @@ final class ConpherenceViewController extends $data = ConpherenceTransactionRenderer::renderTransactions( $user, - $conpherence); + $conpherence, + $full_display = true, + $marker_type); $messages = ConpherenceTransactionRenderer::renderMessagePaneContent( $data['transactions'], - $data['oldest_transaction_id']); - if ($before_transaction_id) { + $data['oldest_transaction_id'], + $data['newest_transaction_id']); + if ($before_transaction_id || $after_transaction_id) { $header = null; $form = null; $content = array('messages' => $messages); @@ -138,5 +167,39 @@ final class ConpherenceViewController extends return $form; } + private function getNeededTransactions( + ConpherenceThread $conpherence, + $message_id) { + if ($message_id) { + $newer_transactions = $conpherence->getTransactions(); + $query = id(new ConpherenceTransactionQuery()) + ->setViewer($this->getRequest()->getUser()) + ->withObjectPHIDs(array($conpherence->getPHID())) + ->setAfterID($message_id) + ->needHandles(true) + ->setLimit(self::OLDER_FETCH_LIMIT); + $older_transactions = $query->execute(); + $handles = array(); + foreach ($older_transactions as $transaction) { + $handles += $transaction->getHandles(); + } + $conpherence->attachHandles($conpherence->getHandles() + $handles); + $transactions = array_merge($newer_transactions, $older_transactions); + $conpherence->attachTransactions($transactions); + } else { + $transactions = $conpherence->getTransactions(); + } + + return $transactions; + } + + private function getMainQueryLimit() { + $request = $this->getRequest(); + $base_limit = ConpherenceThreadQuery::TRANSACTION_LIMIT; + if ($request->getURIData('messageID')) { + $base_limit = $base_limit - self::OLDER_FETCH_LIMIT; + } + return $base_limit; + } } diff --git a/src/applications/conpherence/view/ConpherenceDurableColumnView.php b/src/applications/conpherence/view/ConpherenceDurableColumnView.php index 80d27fd9bc..58e63e30ff 100644 --- a/src/applications/conpherence/view/ConpherenceDurableColumnView.php +++ b/src/applications/conpherence/view/ConpherenceDurableColumnView.php @@ -463,7 +463,8 @@ final class ConpherenceDurableColumnView extends AphrontTagView { $full_display = false); $messages = ConpherenceTransactionRenderer::renderMessagePaneContent( $data['transactions'], - $data['oldest_transaction_id']); + $data['oldest_transaction_id'], + $data['newest_transaction_id']); return $messages; } diff --git a/src/view/page/PhabricatorStandardPageView.php b/src/view/page/PhabricatorStandardPageView.php index 7e7bb4fae0..b0f8b5d0be 100644 --- a/src/view/page/PhabricatorStandardPageView.php +++ b/src/view/page/PhabricatorStandardPageView.php @@ -97,15 +97,27 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView { return false; } + if ($this->isQuicksandBlacklistURI()) { + return false; + } + + return true; + } + + private function isQuicksandBlacklistURI() { + $request = $this->getRequest(); + if (!$request) { + return false; + } + $patterns = $this->getQuicksandURIPatternBlacklist(); $path = $request->getRequestURI()->getPath(); foreach ($patterns as $pattern) { if (preg_match('(^'.$pattern.'$)', $path)) { - return false; + return true; } } - - return true; + return false; } public function getDurableColumnVisible() { @@ -365,12 +377,14 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView { } } - Javelin::initBehavior( - 'scrollbar', - array( - 'nodeID' => 'phabricator-standard-page', - 'isMainContent' => true, - )); + if (!$this->isQuicksandBlacklistURI()) { + Javelin::initBehavior( + 'scrollbar', + array( + 'nodeID' => 'phabricator-standard-page', + 'isMainContent' => true, + )); + } $main_page = phutil_tag( 'div', diff --git a/webroot/rsrc/css/application/conpherence/message-pane.css b/webroot/rsrc/css/application/conpherence/message-pane.css index fa78b622e5..5226480031 100644 --- a/webroot/rsrc/css/application/conpherence/message-pane.css +++ b/webroot/rsrc/css/application/conpherence/message-pane.css @@ -37,7 +37,7 @@ background: #EBECEE; } -.conpherence-show-older-messages { +.conpherence-show-more-messages { display: block; background: #e0e3ec; margin: 10px; @@ -46,7 +46,7 @@ color: {$bluetext}; } -.conpherence-show-older-messages-loading { +.conpherence-show-more-messages-loading { font-style: italic; } diff --git a/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js b/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js index d0c3a020f4..b1694e19ca 100644 --- a/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js +++ b/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js @@ -254,9 +254,10 @@ JX.install('ConpherenceThreadManager', { this.syncWorkflow(workflow, params.stage); }, - loadThreadByID: function(thread_id) { + loadThreadByID: function(thread_id, force_reload) { if (this.isThreadLoaded() && - this.isThreadIDLoaded(thread_id)) { + this.isThreadIDLoaded(thread_id) && + !force_reload) { return; } @@ -277,6 +278,10 @@ JX.install('ConpherenceThreadManager', { JX.Stratcom.invoke('notification-panel-update', null, {}); this._didLoadThreadCallback(r); + + if (force_reload) { + JX.Stratcom.invoke('hashchange'); + } }); // should this be sync'd too? diff --git a/webroot/rsrc/js/application/conpherence/behavior-menu.js b/webroot/rsrc/js/application/conpherence/behavior-menu.js index cf59991c6e..26e184813e 100644 --- a/webroot/rsrc/js/application/conpherence/behavior-menu.js +++ b/webroot/rsrc/js/application/conpherence/behavior-menu.js @@ -119,6 +119,7 @@ JX.behavior('conpherence-menu', function(config) { var messages_root = JX.DOM.find(root, 'div', 'conpherence-message-pane'); var messages = JX.DOM.find(messages_root, 'div', 'conpherence-messages'); scrollbar = new JX.Scrollbar(messages); + scrollbar.setAsScrollFrame(); } init(); @@ -317,12 +318,26 @@ JX.behavior('conpherence-menu', function(config) { buildDeviceWidgetSelector : build_device_widget_selector }); } + var _firstScroll = true; function _scrollMessageWindow() { if (_firstScroll) { _firstScroll = false; - // let the standard #anchor tech take over + + // We want to let the standard #anchor tech take over after we make sure + // we don't have to present the user with a "load older message?" dialog if (window.location.hash) { + var hash = window.location.hash.replace(/^#/, ''); + try { + JX.$('anchor-' + hash); + } catch (ex) { + var uri = '/conpherence/' + + _thread.selected + '/' + hash + '/'; + threadManager.setLoadThreadURI(uri); + threadManager.loadThreadByID(_thread.selected, true); + _firstScroll = true; + return; + } return; } } @@ -374,7 +389,7 @@ JX.behavior('conpherence-menu', function(config) { var form = JX.DOM.find(root, 'form', 'conpherence-pontificate'); var data = e.getNodeData('conpherence-edit-metadata'); var header = JX.DOM.find(root, 'div', 'conpherence-header-pane'); - var messages = JX.DOM.find(root, 'div', 'conpherence-messages'); + var messages = scrollbar.getContentNode(); new JX.Workflow.newFromForm(form, data) .setHandler(JX.bind(this, function(r) { @@ -400,21 +415,21 @@ JX.behavior('conpherence-menu', function(config) { .start(); }); - var _loadingTransactionID = null; + var _oldLoadingTransactionID = null; JX.Stratcom.listen('click', 'show-older-messages', function(e) { e.kill(); var data = e.getNodeData('show-older-messages'); - if (data.oldest_transaction_id == _loadingTransactionID) { + if (data.oldest_transaction_id == _oldLoadingTransactionID) { return; } - _loadingTransactionID = data.oldest_transaction_id; + _oldLoadingTransactionID = data.oldest_transaction_id; + var node = e.getNode('show-older-messages'); JX.DOM.setContent(node, 'Loading...'); - JX.DOM.alterClass(node, 'conpherence-show-older-messages-loading', true); + JX.DOM.alterClass(node, 'conpherence-show-more-messages-loading', true); var conf_id = _thread.selected; - var root = JX.DOM.find(document, 'div', 'conpherence-layout'); - var messages_root = JX.DOM.find(root, 'div', 'conpherence-messages'); + var messages_root = scrollbar.getContentNode(); new JX.Workflow(config.baseURI + conf_id + '/', data) .setHandler(function(r) { JX.DOM.remove(node); @@ -425,6 +440,31 @@ JX.behavior('conpherence-menu', function(config) { }).start(); }); + var _newLoadingTransactionID = null; + JX.Stratcom.listen('click', 'show-newer-messages', function(e) { + e.kill(); + var data = e.getNodeData('show-newer-messages'); + if (data.newest_transaction_id == _newLoadingTransactionID) { + return; + } + _newLoadingTransactionID = data.newest_transaction_id; + + var node = e.getNode('show-newer-messages'); + JX.DOM.setContent(node, 'Loading...'); + JX.DOM.alterClass(node, 'conpherence-show-more-messages-loading', true); + + var conf_id = _thread.selected; + var messages_root = scrollbar.getContentNode(); + new JX.Workflow(config.baseURI + conf_id + '/', data) + .setHandler(function(r) { + JX.DOM.remove(node); + var messages = JX.$H(r.messages); + JX.DOM.appendContent( + messages_root, + JX.$H(messages)); + }).start(); + }); + /** * On devices, we just show a thread list, so we don't want to automatically * select or load any threads. On desktop, we automatically select the first