From 7a96de10f07a51d8e81a56a281fe10c5eaaf8f5d Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 14 Feb 2014 10:23:07 -0800 Subject: [PATCH] Hide seen transactions by default in all modern applications Summary: Ref T2222. This restores the "N older comments are hidden." shield to all ApplicationTransactions applications. Roughly the rule this uses is that transactions older than your most recent comment are hidden, under the assumption that you've already read and dealt with them, since you replied afterward. Then we show your last comment to remind/contextualize you, and anything afterward. We also don't hide transactions if we'd only be hiding a handfull, and we never hide the few most recent transactions. This might need some #design help. Test Plan: The tricky part here is the anchor rule, which deals with the case where you follow a link to `T123#4`, but that would normally be hidden. We simulate a click on "show all" if you hit an anchor which is hidden. Here's what it looks like in Maniphest: {F112891} Reviewers: btrahan, chad Reviewed By: btrahan CC: chad, aran Maniphest Tasks: T2222 Differential Revision: https://secure.phabricator.com/D8229 --- resources/celerity/map.php | 33 ++++---- resources/celerity/packages.php | 1 - .../PhabricatorApplicationTransactionView.php | 46 +++++++++- .../PhabricatorBaseEnglishTranslation.php | 5 ++ src/view/phui/PHUITimelineEventView.php | 14 ++++ src/view/phui/PHUITimelineView.php | 68 ++++++++++++++- .../differential/revision-comment-list.css | 12 --- webroot/rsrc/css/phui/phui-timeline-view.css | 9 ++ .../behavior-show-all-comments.js | 83 ++++++++++--------- 9 files changed, 196 insertions(+), 75 deletions(-) delete mode 100644 webroot/rsrc/css/application/differential/revision-comment-list.css diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 21420c02bb..33ccea4d8c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'core.pkg.css' => '607946ba', 'core.pkg.js' => 'c7854cc5', 'darkconsole.pkg.js' => 'ca8671ce', - 'differential.pkg.css' => '5a65a762', + 'differential.pkg.css' => '6aef439e', 'differential.pkg.js' => '322ea941', 'diffusion.pkg.css' => '3783278d', 'diffusion.pkg.js' => '7b51e80a', @@ -60,7 +60,6 @@ return array( 'rsrc/css/application/differential/core.css' => '8135cb0c', 'rsrc/css/application/differential/local-commits-view.css' => '19649019', 'rsrc/css/application/differential/results-table.css' => '239924f9', - 'rsrc/css/application/differential/revision-comment-list.css' => 'bc291c47', 'rsrc/css/application/differential/revision-comment.css' => '48186045', 'rsrc/css/application/differential/revision-history.css' => 'f37aee8f', 'rsrc/css/application/differential/revision-list.css' => 'f3c47d33', @@ -144,7 +143,7 @@ return array( 'rsrc/css/phui/phui-status.css' => '2f562399', 'rsrc/css/phui/phui-tag-view.css' => '295d81c4', 'rsrc/css/phui/phui-text.css' => '23e9b4b7', - 'rsrc/css/phui/phui-timeline-view.css' => '6c5e2da9', + 'rsrc/css/phui/phui-timeline-view.css' => 'd3ccba00', 'rsrc/css/phui/phui-workboard-view.css' => 'bf70dd2e', 'rsrc/css/phui/phui-workpanel-view.css' => '6f8527f6', 'rsrc/css/sprite-actions.css' => '4557baf8', @@ -359,7 +358,7 @@ return array( 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '93f43142', 'rsrc/js/application/differential/behavior-keyboard-nav.js' => 'da3e88f9', 'rsrc/js/application/differential/behavior-populate.js' => 'ce0c217a', - 'rsrc/js/application/differential/behavior-show-all-comments.js' => '784b8218', + 'rsrc/js/application/differential/behavior-show-all-comments.js' => '7c273581', 'rsrc/js/application/differential/behavior-show-field-details.js' => '441f2137', 'rsrc/js/application/differential/behavior-show-more.js' => 'dd7e8ef5', 'rsrc/js/application/differential/behavior-toggle-files.js' => 'ca3f91eb', @@ -508,7 +507,6 @@ return array( 'differential-results-table-css' => '239924f9', 'differential-revision-add-comment-css' => 'c478bcaa', 'differential-revision-comment-css' => '48186045', - 'differential-revision-comment-list-css' => 'bc291c47', 'differential-revision-history-css' => 'f37aee8f', 'differential-revision-list-css' => 'f3c47d33', 'differential-table-of-contents-css' => '19566f76', @@ -549,7 +547,6 @@ return array( 'javelin-behavior-differential-feedback-preview' => '127f2018', 'javelin-behavior-differential-keyboard-navigation' => 'da3e88f9', 'javelin-behavior-differential-populate' => 'ce0c217a', - 'javelin-behavior-differential-show-all-comments' => '784b8218', 'javelin-behavior-differential-show-field-details' => '441f2137', 'javelin-behavior-differential-show-more' => 'dd7e8ef5', 'javelin-behavior-differential-toggle-files' => 'ca3f91eb', @@ -598,6 +595,7 @@ return array( 'javelin-behavior-phabricator-remarkup-assist' => 'c021950a', 'javelin-behavior-phabricator-reveal-content' => '8f24abfc', 'javelin-behavior-phabricator-search-typeahead' => 'f6b56f7a', + 'javelin-behavior-phabricator-show-all-transactions' => '7c273581', 'javelin-behavior-phabricator-tooltips' => 'e5dd1c6d', 'javelin-behavior-phabricator-transaction-comment-form' => '9084a36f', 'javelin-behavior-phabricator-transaction-list' => 'bfb45968', @@ -756,7 +754,7 @@ return array( 'phui-status-list-view-css' => '2f562399', 'phui-tag-view-css' => '295d81c4', 'phui-text-css' => '23e9b4b7', - 'phui-timeline-view-css' => '6c5e2da9', + 'phui-timeline-view-css' => 'd3ccba00', 'phui-workboard-view-css' => 'bf70dd2e', 'phui-workpanel-view-css' => '6f8527f6', 'policy-css' => '957ea14c', @@ -1235,12 +1233,6 @@ return array( 0 => 'javelin-install', 1 => 'javelin-util', ), - '784b8218' => - array( - 0 => 'javelin-behavior', - 1 => 'javelin-stratcom', - 2 => 'javelin-dom', - ), '79473b62' => array( 0 => 'javelin-install', @@ -1256,6 +1248,12 @@ return array( 1 => 'javelin-dom', 2 => 'javelin-reactor-dom', ), + '7c273581' => + array( + 0 => 'javelin-behavior', + 1 => 'javelin-stratcom', + 2 => 'javelin-dom', + ), '7e41274a' => array( 0 => 'javelin-install', @@ -2114,11 +2112,10 @@ return array( 5 => 'differential-table-of-contents-css', 6 => 'differential-revision-comment-css', 7 => 'differential-revision-add-comment-css', - 8 => 'differential-revision-comment-list-css', - 9 => 'phabricator-object-selector-css', - 10 => 'phabricator-content-source-view-css', - 11 => 'differential-local-commits-view-css', - 12 => 'inline-comment-summary-css', + 8 => 'phabricator-object-selector-css', + 9 => 'phabricator-content-source-view-css', + 10 => 'differential-local-commits-view-css', + 11 => 'inline-comment-summary-css', ), 'differential.pkg.js' => array( diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php index 08abd1a186..da471882ad 100644 --- a/resources/celerity/packages.php +++ b/resources/celerity/packages.php @@ -124,7 +124,6 @@ return array( 'differential-table-of-contents-css', 'differential-revision-comment-css', 'differential-revision-add-comment-css', - 'differential-revision-comment-list-css', 'phabricator-object-selector-css', 'phabricator-content-source-view-css', 'differential-local-commits-view-css', diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php index d0692c6052..2d2a47afdb 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php @@ -51,7 +51,7 @@ class PhabricatorApplicationTransactionView extends AphrontView { return $this; } - public function buildEvents() { + public function buildEvents($with_hiding = false) { $user = $this->getUser(); $anchor = $this->anchorOffset; @@ -62,11 +62,50 @@ class PhabricatorApplicationTransactionView extends AphrontView { $xactions = $this->groupRelatedTransactions($xactions); $groups = $this->groupDisplayTransactions($xactions); + // If the viewer has interacted with this object, we hide things from + // before their most recent interaction by default. This tends to make + // very long threads much more manageable, because you don't have to + // scroll through a lot of history and can focus on just new stuff. + + $show_group = null; + + if ($with_hiding) { + // Find the most recent comment by the viewer. + $group_keys = array_keys($groups); + $group_keys = array_reverse($group_keys); + + // If we would only hide a small number of transactions, don't hide + // anything. Just don't examine the last few keys. Also, we always + // want to show the most recent pieces of activity, so don't examine + // the first few keys either. + $group_keys = array_slice($group_keys, 2, -2); + + $type_comment = PhabricatorTransactions::TYPE_COMMENT; + foreach ($group_keys as $group_key) { + $group = $groups[$group_key]; + foreach ($group as $xaction) { + if ($xaction->getAuthorPHID() == $user->getPHID() && + $xaction->getTransactionType() == $type_comment) { + // This is the most recent group where the user commented. + $show_group = $group_key; + break 2; + } + } + } + } + $events = array(); - foreach ($groups as $group) { + $hide_by_default = ($show_group !== null); + + foreach ($groups as $group_key => $group) { + if ($hide_by_default && ($show_group === $group_key)) { + $hide_by_default = false; + } + $group_event = null; foreach ($group as $xaction) { $event = $this->renderEvent($xaction, $group, $anchor); + $event->setHideByDefault($hide_by_default); $anchor++; if (!$group_event) { $group_event = $event; @@ -75,6 +114,7 @@ class PhabricatorApplicationTransactionView extends AphrontView { } } $events[] = $group_event; + } return $events; @@ -86,7 +126,7 @@ class PhabricatorApplicationTransactionView extends AphrontView { } $view = new PHUITimelineView(); - $events = $this->buildEvents(); + $events = $this->buildEvents($with_hiding = true); foreach ($events as $event) { $view->addEvent($event); } diff --git a/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php index 09a2cce336..c6773d09c4 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php +++ b/src/infrastructure/internationalization/translation/PhabricatorBaseEnglishTranslation.php @@ -826,6 +826,11 @@ abstract class PhabricatorBaseEnglishTranslation 'users will need to sign again. Proceed carefully.', ), + '%s older changes(s) are hidden.' => array( + '%d older change is hidden.', + '%d older changes are hidden.', + ), + ); } diff --git a/src/view/phui/PHUITimelineEventView.php b/src/view/phui/PHUITimelineEventView.php index 5e86d5f9d4..6de44fcf8a 100644 --- a/src/view/phui/PHUITimelineEventView.php +++ b/src/view/phui/PHUITimelineEventView.php @@ -15,6 +15,16 @@ final class PHUITimelineEventView extends AphrontView { private $transactionPHID; private $isPreview; private $eventGroup = array(); + private $hideByDefault; + + public function setHideByDefault($hide_by_default) { + $this->hideByDefault = $hide_by_default; + return $this; + } + + public function getHideByDefault() { + return $this->hideByDefault; + } public function setTransactionPHID($transaction_phid) { $this->transactionPHID = $transaction_phid; @@ -80,6 +90,10 @@ final class PHUITimelineEventView extends AphrontView { return $this; } + public function getAnchor() { + return $this->anchor; + } + public function setTitle($title) { $this->title = $title; return $this; diff --git a/src/view/phui/PHUITimelineView.php b/src/view/phui/PHUITimelineView.php index b690a19ae6..4858afce30 100644 --- a/src/view/phui/PHUITimelineView.php +++ b/src/view/phui/PHUITimelineView.php @@ -20,12 +20,74 @@ final class PHUITimelineView extends AphrontView { $spacer = self::renderSpacer(); - $events = array(); + $hide = array(); + $show = array(); + foreach ($this->events as $event) { + if ($event->getHideByDefault()) { + $hide[] = $event; + } else { + $show[] = $event; + } + } + + $events = array(); + if ($hide) { + $hidden = phutil_implode_html($spacer, $hide); + $count = count($hide); + + $show_id = celerity_generate_unique_node_id(); + $hide_id = celerity_generate_unique_node_id(); + $link_id = celerity_generate_unique_node_id(); + + Javelin::initBehavior( + 'phabricator-show-all-transactions', + array( + 'anchors' => array_filter(mpull($hide, 'getAnchor')), + 'linkID' => $link_id, + 'hideID' => $hide_id, + 'showID' => $show_id, + )); + + $events[] = phutil_tag( + 'div', + array( + 'id' => $hide_id, + 'class' => 'phui-timeline-older-transactions-are-hidden', + ), + array( + pht('%s older changes(s) are hidden.', new PhutilNumber($count)), + ' ', + javelin_tag( + 'a', + array( + 'href' => '#', + 'mustcapture' => true, + 'id' => $link_id, + ), + pht('Show all changes.')), + )); + + $events[] = phutil_tag( + 'div', + array( + 'id' => $show_id, + 'style' => 'display: none', + ), + $hidden); + } + + if ($hide && $show) { $events[] = $spacer; - $events[] = $event; } - $events[] = $spacer; + + if ($show) { + $events[] = phutil_implode_html($spacer, $show); + } + + if ($events) { + $events = array($spacer, $events, $spacer); + } return phutil_tag( 'div', diff --git a/webroot/rsrc/css/application/differential/revision-comment-list.css b/webroot/rsrc/css/application/differential/revision-comment-list.css deleted file mode 100644 index 6571c126a3..0000000000 --- a/webroot/rsrc/css/application/differential/revision-comment-list.css +++ /dev/null @@ -1,12 +0,0 @@ -/** - * @provides differential-revision-comment-list-css - */ - -.differential-older-comments-are-hidden { - background: {$lightyellow}; - border: 1px solid {$yellow}; - text-align: center; - padding: 12px; - color: {$darkgreytext}; - margin-top: 16px; -} diff --git a/webroot/rsrc/css/phui/phui-timeline-view.css b/webroot/rsrc/css/phui/phui-timeline-view.css index 65afd372a1..db5f9a9b44 100644 --- a/webroot/rsrc/css/phui/phui-timeline-view.css +++ b/webroot/rsrc/css/phui/phui-timeline-view.css @@ -260,3 +260,12 @@ border-color: #efefef; border-width: 1px 0; } + +.phui-timeline-older-transactions-are-hidden { + background: {$lightyellow}; + border: 1px solid {$yellow}; + text-align: center; + padding: 12px; + color: {$darkgreytext}; + margin-top: 16px; +} diff --git a/webroot/rsrc/js/application/differential/behavior-show-all-comments.js b/webroot/rsrc/js/application/differential/behavior-show-all-comments.js index 6bd9f825a1..9482c569a3 100644 --- a/webroot/rsrc/js/application/differential/behavior-show-all-comments.js +++ b/webroot/rsrc/js/application/differential/behavior-show-all-comments.js @@ -1,59 +1,66 @@ /** - * @provides javelin-behavior-differential-show-all-comments + * @provides javelin-behavior-phabricator-show-all-transactions * @requires javelin-behavior * javelin-stratcom * javelin-dom */ -JX.behavior('differential-show-all-comments', function(config) { +/** + * Automatically show older transactions if the user follows an anchor to a + * transaction which is hidden by the "N older changes are hidden." shield. + */ +JX.behavior('phabricator-show-all-transactions', function(config) { - var shown = false; - function reveal(node) { - if (shown) { - return false; - } - shown = true; - node = node || JX.DOM.find( - document.body, - 'div', - 'differential-all-comments-container'); - if (node) { - JX.DOM.setContent(node, JX.$H(config.markup)); + var revealed = false; + + function get_hash() { + return window.location.hash.replace(/^#/, ''); + } + + function hash_is_hidden() { + var hash = get_hash(); + for (var ii = 0; ii < config.anchors.length; ii++) { + if (config.anchors[ii] == hash) { + return true; + } } - return true; + return false; } - // Reveal the hidden comments if the user clicks "Show All Comments", or if - // there's an anchor in the URL, since we don't want to link to "#comment-3" - // and have it collapsed. + function reveal() { + if (revealed) { + return false; + } + + JX.DOM.hide(JX.$(config.hideID)); + JX.DOM.show(JX.$(config.showID)); + revealed = true; - function at_comment_hash() { - return window.location.hash && window.location.hash.match(/comment/); + return true; } - if (at_comment_hash()) { - reveal(); - } else { - JX.Stratcom.listen( - 'hashchange', - null, - function(e) { - if (at_comment_hash() && reveal()) { - try { - var target = JX.$(window.location.hash.replace(/^#/, '')); - window.scrollTo(0, target.offsetTop); - } catch (ex) { - } + function check_hash() { + if (hash_is_hidden()) { + if (reveal()) { + try { + var target = JX.$(get_hash()); + JX.DOM.scrollTo(target); + } catch (ignored) { + // We did our best. } - }); + } + } } - JX.Stratcom.listen( + JX.DOM.listen( + JX.$(config.linkID), 'click', - 'differential-show-all-comments', - function(e) { - reveal(e.getNode('differential-all-comments-container')); + null, + function (e) { e.kill(); + reveal(); }); + JX.Stratcom.listen('hashchange', null, check_hash); + check_hash(); }); -- 2.51.2