From 6c049d06ce4d9d1cf4701c4a66fbc53e09954d05 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Wed, 13 May 2015 11:06:54 -0700 Subject: [PATCH] Conpherence - change message rendering logic to eradicate possibility of duplicates Summary: Fixes T6713. Before this diff, we would update the DOM when various requests came back, but the logic to erase race conditions proved too tricky for me to get right. Instead, change the algorithm up and keep a set of transaction ids around per thread. When its time to update the transactions, sort the list of ids and just render the whole darn set again. To make this work, this ends up adding transacton ids to fake transactons like "show older" and date markers. This is able to work by using a float sort and giving these transactions ids that are .5 from being an integer and in the right place numerically. Test Plan: for durable column, clicked show older and it worked. sent a message and it worked. for main view, clicked show older and it worked. sent a message and it worked. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T6713 Differential Revision: https://secure.phabricator.com/D12819 --- resources/celerity/map.php | 84 +++++++-------- .../ConpherenceTransactionRenderer.php | 19 ++++ .../controller/ConpherenceViewController.php | 4 +- .../view/ConpherenceTransactionView.php | 12 ++- .../conpherence/ConpherenceThreadManager.js | 101 +++++++++++++++--- .../conpherence/behavior-durable-column.js | 2 - .../application/conpherence/behavior-menu.js | 8 +- 7 files changed, 162 insertions(+), 68 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 0d1ec02ee0..3afab6cdb9 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -8,7 +8,7 @@ return array( 'names' => array( 'core.pkg.css' => 'ed3d6355', - 'core.pkg.js' => '616511ac', + 'core.pkg.js' => 'ac41c400', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => 'bb338e4b', 'differential.pkg.js' => '895b8d62', @@ -348,10 +348,10 @@ return array( 'rsrc/js/application/auth/behavior-persona-login.js' => '9414ff18', 'rsrc/js/application/calendar/event-all-day.js' => 'ca5fa62a', 'rsrc/js/application/config/behavior-reorder-fields.js' => '14a827de', - 'rsrc/js/application/conpherence/ConpherenceThreadManager.js' => 'de397217', + 'rsrc/js/application/conpherence/ConpherenceThreadManager.js' => 'b7342ddb', 'rsrc/js/application/conpherence/behavior-drag-and-drop-photo.js' => 'cf86d16a', - 'rsrc/js/application/conpherence/behavior-durable-column.js' => '61252a27', - 'rsrc/js/application/conpherence/behavior-menu.js' => 'eb61cb03', + 'rsrc/js/application/conpherence/behavior-durable-column.js' => '16c695bf', + 'rsrc/js/application/conpherence/behavior-menu.js' => '4351c4a0', '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', @@ -516,7 +516,7 @@ return array( 'conpherence-menu-css' => 'f389e048', 'conpherence-message-pane-css' => '0e75feef', 'conpherence-notification-css' => 'd208f806', - 'conpherence-thread-manager' => 'de397217', + 'conpherence-thread-manager' => 'b7342ddb', 'conpherence-transaction-css' => '42a457f6', 'conpherence-update-css' => '1099a660', 'conpherence-widget-pane-css' => '2af42ebe', @@ -557,7 +557,7 @@ return array( 'javelin-behavior-choose-control' => '6153c708', 'javelin-behavior-config-reorder-fields' => '14a827de', 'javelin-behavior-conpherence-drag-and-drop-photo' => 'cf86d16a', - 'javelin-behavior-conpherence-menu' => 'eb61cb03', + 'javelin-behavior-conpherence-menu' => '4351c4a0', 'javelin-behavior-conpherence-pontificate' => '21ba5861', 'javelin-behavior-conpherence-widget-pane' => '93568464', 'javelin-behavior-countdown-timer' => 'e4cc26b3', @@ -584,7 +584,7 @@ return array( 'javelin-behavior-diffusion-locate-file' => '6d3e1947', 'javelin-behavior-diffusion-pull-lastmodified' => '2b228192', 'javelin-behavior-doorkeeper-tag' => 'e5822781', - 'javelin-behavior-durable-column' => '61252a27', + 'javelin-behavior-durable-column' => '16c695bf', 'javelin-behavior-error-log' => '6882e80a', 'javelin-behavior-event-all-day' => 'ca5fa62a', 'javelin-behavior-fancy-datepicker' => '5c0f680f', @@ -930,6 +930,16 @@ return array( 'javelin-request', 'javelin-util', ), + '16c695bf' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-behavior-device', + 'javelin-scrollbar', + 'javelin-quicksand', + 'phabricator-keyboard-shortcut', + 'conpherence-thread-manager', + ), '1ad0a787' => array( 'javelin-install', 'javelin-reactor', @@ -1073,6 +1083,20 @@ return array( 'javelin-dom', 'javelin-request', ), + '4351c4a0' => 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', + ), '44168bad' => array( 'javelin-behavior', 'javelin-dom', @@ -1256,16 +1280,6 @@ return array( 'javelin-stratcom', 'javelin-dom', ), - '61252a27' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-behavior-device', - 'javelin-scrollbar', - 'javelin-quicksand', - 'phabricator-keyboard-shortcut', - 'conpherence-thread-manager', - ), '6153c708' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1705,6 +1719,17 @@ return array( 'javelin-dom', 'javelin-util', ), + 'b7342ddb' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-aphlict', + 'javelin-workflow', + 'javelin-router', + 'javelin-behavior-device', + 'javelin-vector', + ), 'ba4fa35c' => array( 'javelin-behavior', 'javelin-dom', @@ -1832,17 +1857,6 @@ return array( 'javelin-typeahead-ondemand-source', 'javelin-dom', ), - 'de397217' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-aphlict', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - ), 'df5e11d2' => array( 'javelin-install', ), @@ -1920,20 +1934,6 @@ return array( 'phabricator-phtize', 'javelin-dom', ), - 'eb61cb03' => 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', - ), 'efe49472' => array( 'javelin-install', 'javelin-util', diff --git a/src/applications/conpherence/ConpherenceTransactionRenderer.php b/src/applications/conpherence/ConpherenceTransactionRenderer.php index d6be83fb65..1b14f5f0d6 100644 --- a/src/applications/conpherence/ConpherenceTransactionRenderer.php +++ b/src/applications/conpherence/ConpherenceTransactionRenderer.php @@ -90,6 +90,7 @@ final class ConpherenceTransactionRenderer { if ($previous_day != $current_day) { $date_marker_transaction->setDateCreated( $transaction->getDateCreated()); + $date_marker_transaction->setID($previous_transaction->getID()); $rendered_transactions[] = $date_marker_transaction_view->render(); } } @@ -144,6 +145,15 @@ final class ConpherenceTransactionRenderer { ), ), pht('Show Older Messages')); + $oldscrollbutton = javelin_tag( + 'div', + array( + 'sigil' => 'conpherence-transaction-view', + 'meta' => array( + 'id' => $oldest_transaction_id - 0.5, + ), + ), + $oldscrollbutton); } $newscrollbutton = ''; @@ -160,6 +170,15 @@ final class ConpherenceTransactionRenderer { ), ), pht('Show Newer Messages')); + $newscrollbutton = javelin_tag( + 'div', + array( + 'sigil' => 'conpherence-transaction-view', + 'meta' => array( + 'id' => $newest_transaction_id + 0.5, + ), + ), + $newscrollbutton); } return hsprintf( diff --git a/src/applications/conpherence/controller/ConpherenceViewController.php b/src/applications/conpherence/controller/ConpherenceViewController.php index 39138a40b4..6293c1bbdd 100644 --- a/src/applications/conpherence/controller/ConpherenceViewController.php +++ b/src/applications/conpherence/controller/ConpherenceViewController.php @@ -79,7 +79,7 @@ final class ConpherenceViewController extends if ($before_transaction_id || $after_transaction_id) { $header = null; $form = null; - $content = array('messages' => $messages); + $content = array('transactions' => $messages); } else { $policy_objects = id(new PhabricatorPolicyQuery()) ->setViewer($user) @@ -89,7 +89,7 @@ final class ConpherenceViewController extends $form = $this->renderFormContent(); $content = array( 'header' => $header, - 'messages' => $messages, + 'transactions' => $messages, 'form' => $form, ); } diff --git a/src/applications/conpherence/view/ConpherenceTransactionView.php b/src/applications/conpherence/view/ConpherenceTransactionView.php index 3ed5fa14eb..2dba770e54 100644 --- a/src/applications/conpherence/view/ConpherenceTransactionView.php +++ b/src/applications/conpherence/view/ConpherenceTransactionView.php @@ -103,10 +103,14 @@ final class ConpherenceTransactionView extends AphrontView { $transaction = $this->getConpherenceTransaction(); switch ($transaction->getTransactionType()) { case ConpherenceTransactionType::TYPE_DATE_MARKER: - return phutil_tag( + return javelin_tag( 'div', array( 'class' => 'conpherence-transaction-view date-marker', + 'sigil' => 'conpherence-transaction-view', + 'meta' => array( + 'id' => $transaction->getID() + 0.5, + ), ), array( phutil_tag( @@ -134,11 +138,15 @@ final class ConpherenceTransactionView extends AphrontView { 'conpherence-transaction-header grouped', array($actions, $info)); - return phutil_tag( + return javelin_tag( 'div', array( 'class' => 'conpherence-transaction-view '.$classes, 'id' => $transaction_id, + 'sigil' => 'conpherence-transaction-view', + 'meta' => array( + 'id' => $transaction->getID(), + ), ), array( $image, diff --git a/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js b/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js index 8a534c9c01..c1c47c5c78 100644 --- a/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js +++ b/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js @@ -27,6 +27,8 @@ JX.install('ConpherenceThreadManager', { _loadedThreadID: null, _loadedThreadPHID: null, _latestTransactionID: null, + _transactionIDMap: null, + _transactionCache: null, _canEditLoadedThread: null, _updating: null, _minimalDisplay: false, @@ -83,6 +85,59 @@ JX.install('ConpherenceThreadManager', { return this; }, + _updateTransactionIDMap: function(transactions) { + var loaded_id = this.getLoadedThreadID(); + if (!this._transactionIDMap[loaded_id]) { + this._transactionIDMap[this._loadedThreadID] = {}; + } + var loaded_transaction_ids = this._transactionIDMap[loaded_id]; + var transaction; + for (var ii = 0; ii < transactions.length; ii++) { + transaction = transactions[ii]; + loaded_transaction_ids[JX.Stratcom.getData(transaction).id] = 1; + } + this._transactionIDMap[this._loadedThreadID] = loaded_transaction_ids; + return this; + }, + + _updateTransactionCache: function(transactions) { + var transaction; + for (var ii = 0; ii < transactions.length; ii++) { + transaction = transactions[ii]; + this._transactionCache[JX.Stratcom.getData(transaction).id] = + transaction; + } + return this; + }, + + _getLoadedTransactions: function() { + var loaded_id = this.getLoadedThreadID(); + var loaded_tx_ids = JX.keys(this._transactionIDMap[loaded_id]); + loaded_tx_ids.sort(function (a, b) { + var x = parseFloat(a); + var y = parseFloat(b); + if (x > y) { + return 1; + } + if (x < y) { + return -1; + } + return 0; + }); + var transactions = []; + for (var ii = 0; ii < loaded_tx_ids.length; ii++) { + transactions.push(this._transactionCache[loaded_tx_ids[ii]]); + } + return transactions; + }, + + _deleteTransactionCaches: function(id) { + delete this._transactionCache[id]; + delete this._transactionIDMap[this._loadedThreadID][id]; + + return this; + }, + setCanEditLoadedThread: function(bool) { this._canEditLoadedThread = bool; return this; @@ -151,6 +206,10 @@ JX.install('ConpherenceThreadManager', { }, start: function() { + + this._transactionIDMap = {}; + this._transactionCache = {}; + JX.Stratcom.listen( 'aphlict-server-message', null, @@ -206,11 +265,9 @@ JX.install('ConpherenceThreadManager', { new JX.Workflow(this._getMoreMessagesURI(), data) .setHandler(JX.bind(this, function(r) { + this._deleteTransactionCaches(JX.Stratcom.getData(node).id); JX.DOM.remove(node); - var messages = JX.$H(r.messages); - JX.DOM.prependContent( - this._messagesRootCallback(), - messages); + this._updateTransactions(r); })).start(); })); JX.Stratcom.listen( @@ -228,11 +285,9 @@ JX.install('ConpherenceThreadManager', { new JX.Workflow(this._getMoreMessagesURI(), data) .setHandler(JX.bind(this, function(r) { + this._deleteTransactionCaches(JX.Stratcom.getData(node).id); JX.DOM.remove(node); - var messages = JX.$H(r.messages); - JX.DOM.appendContent( - this._messagesRootCallback(), - JX.$H(messages)); + this._updateTransactions(r); })).start(); })); }, @@ -254,7 +309,9 @@ JX.install('ConpherenceThreadManager', { return true; }, - _markUpdated: function(r) { + _updateDOM: function(r) { + this._updateTransactions(r); + this._updating.knownID = r.latest_transaction_id; this._latestTransactionID = r.latest_transaction_id; JX.Stratcom.invoke( @@ -263,6 +320,16 @@ JX.install('ConpherenceThreadManager', { r.aphlictDropdownData); }, + _updateTransactions: function(r) { + var new_transactions = JX.$H(r.transactions).getFragment().childNodes; + this._updateTransactionIDMap(new_transactions); + this._updateTransactionCache(new_transactions); + + var transactions = this._getLoadedTransactions(); + + JX.DOM.setContent(this._messagesRootCallback(), transactions); + }, + _updateThread: function() { var params = this._getParams({ action: 'load', @@ -272,8 +339,7 @@ JX.install('ConpherenceThreadManager', { .setData(params) .setHandler(JX.bind(this, function(r) { if (this._shouldUpdateDOM(r)) { - this._markUpdated(r); - + this._updateDOM(r); this._didUpdateThreadCallback(r); } })); @@ -306,8 +372,7 @@ JX.install('ConpherenceThreadManager', { .setData(params) .setHandler(JX.bind(this, function(r) { if (this._shouldUpdateDOM(r)) { - this._markUpdated(r); - + this._updateDOM(r); this._didUpdateWorkflowCallback(r); } })); @@ -357,6 +422,13 @@ JX.install('ConpherenceThreadManager', { r.aphlictDropdownData); this._didLoadThreadCallback(r); + var messages_root = this._messagesRootCallback(); + var messages = JX.DOM.scry( + messages_root, + 'div', + 'conpherence-transaction-view'); + this._updateTransactionIDMap(messages); + this._updateTransactionCache(messages); if (force_reload) { JX.Stratcom.invoke('hashchange'); @@ -383,8 +455,7 @@ JX.install('ConpherenceThreadManager', { var workflow = JX.Workflow.newFromForm(form, params, keep_enabled) .setHandler(JX.bind(this, function(r) { if (this._shouldUpdateDOM(r)) { - this._markUpdated(r); - + this._updateDOM(r); this._didSendMessageCallback(r); } else if (r.non_update) { this._didSendMessageCallback(r, true); diff --git a/webroot/rsrc/js/application/conpherence/behavior-durable-column.js b/webroot/rsrc/js/application/conpherence/behavior-durable-column.js index 78ed506da9..4a9eac1eaf 100644 --- a/webroot/rsrc/js/application/conpherence/behavior-durable-column.js +++ b/webroot/rsrc/js/application/conpherence/behavior-durable-column.js @@ -159,7 +159,6 @@ JX.behavior('durable-column', function(config, statics) { return; } var messages = _getColumnMessagesNode(); - JX.DOM.appendContent(messages, JX.$H(r.transactions)); scrollbar.scrollTo(messages.scrollHeight); }); @@ -168,7 +167,6 @@ JX.behavior('durable-column', function(config, statics) { }); threadManager.setDidUpdateWorkflowCallback(function(r) { var messages = _getColumnMessagesNode(); - JX.DOM.appendContent(messages, JX.$H(r.transactions)); scrollbar.scrollTo(messages.scrollHeight); JX.DOM.setContent(_getColumnTitleNode(), r.conpherence_title); }); diff --git a/webroot/rsrc/js/application/conpherence/behavior-menu.js b/webroot/rsrc/js/application/conpherence/behavior-menu.js index e8621cc990..2233b3ff28 100644 --- a/webroot/rsrc/js/application/conpherence/behavior-menu.js +++ b/webroot/rsrc/js/application/conpherence/behavior-menu.js @@ -36,7 +36,7 @@ JX.behavior('conpherence-menu', function(config) { }); threadManager.setDidLoadThreadCallback(function(r) { var header = JX.$H(r.header); - var messages = JX.$H(r.messages); + var messages = JX.$H(r.transactions); var form = JX.$H(r.form); var root = JX.DOM.find(document, 'div', 'conpherence-layout'); var header_root = JX.DOM.find(root, 'div', 'conpherence-header-pane'); @@ -51,7 +51,6 @@ JX.behavior('conpherence-menu', function(config) { }); threadManager.setDidUpdateThreadCallback(function(r) { - JX.DOM.appendContent(scrollbar.getContentNode(), JX.$H(r.transactions)); _scrollMessageWindow(); }); @@ -73,14 +72,13 @@ JX.behavior('conpherence-menu', function(config) { } catch (ex) { // Ignore; maybe no files widget } - JX.DOM.appendContent(scrollbar.getContentNode(), JX.$H(r.transactions)); - _scrollMessageWindow(); - if (fileWidget) { JX.DOM.setContent( fileWidget, JX.$H(r.file_widget)); } + + _scrollMessageWindow(); textarea.value = ''; } markThreadLoading(false);