From 60de1506fe3b78c7e904af670226c987ab0e9713 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 22 Apr 2020 13:13:15 -0700 Subject: [PATCH] Make "hidden" changesets sticky, and show hidden state in the filetree Summary: Ref T13455. Make "hidden" a changeset property similar to other changeset properties. We don't need to render this on the server, so we make a request (to update the setting) and just discard the response. Test Plan: {F7375468} Maniphest Tasks: T13455 Differential Revision: https://secure.phabricator.com/D21158 --- resources/celerity/map.php | 44 +++++++------- .../DifferentialChangesetViewController.php | 4 ++ .../parser/DifferentialChangesetParser.php | 1 + .../storage/DifferentialViewState.php | 22 +++++-- .../controller/DiffusionDiffController.php | 4 ++ .../PhabricatorChangesetViewState.php | 30 ++++++++++ .../PhabricatorChangesetViewStateEngine.php | 60 ++++++++++++++++++- .../css/application/diff/diff-tree-view.css | 13 ++++ .../rsrc/js/application/diff/DiffChangeset.js | 44 ++++++++++++-- .../rsrc/js/application/diff/DiffPathView.js | 44 +++++++++++++- 10 files changed, 229 insertions(+), 37 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 70f48e222e..8969ef414c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,7 +12,7 @@ return array( 'core.pkg.css' => 'dd5f04a3', 'core.pkg.js' => '544bc792', 'differential.pkg.css' => 'cb99cd21', - 'differential.pkg.js' => '6c315e3f', + 'differential.pkg.js' => '54613dd5', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', 'maniphest.pkg.css' => '35995d6d', @@ -59,7 +59,7 @@ return array( 'rsrc/css/application/countdown/timer.css' => 'bff8012f', 'rsrc/css/application/daemon/bulk-job.css' => '73af99f5', 'rsrc/css/application/dashboard/dashboard.css' => '5a205b9d', - 'rsrc/css/application/diff/diff-tree-view.css' => '26fb4a0d', + 'rsrc/css/application/diff/diff-tree-view.css' => 'e2d3e222', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', 'rsrc/css/application/differential/changeset-view.css' => '489b6995', @@ -378,10 +378,10 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => 'a2ab19be', 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8', - 'rsrc/js/application/diff/DiffChangeset.js' => 'a1a5dc46', + 'rsrc/js/application/diff/DiffChangeset.js' => 'dd1a6f34', 'rsrc/js/application/diff/DiffChangesetList.js' => '57035863', 'rsrc/js/application/diff/DiffInline.js' => '16e97ebc', - 'rsrc/js/application/diff/DiffPathView.js' => 'ceb66010', + 'rsrc/js/application/diff/DiffPathView.js' => '8207abf9', 'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b', 'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17', 'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd', @@ -558,7 +558,7 @@ return array( 'conpherence-thread-manager' => 'aec8e38c', 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => '9d068042', - 'diff-tree-view-css' => '26fb4a0d', + 'diff-tree-view-css' => 'e2d3e222', 'differential-changeset-view-css' => '489b6995', 'differential-core-view-css' => '7300a73e', 'differential-revision-add-comment-css' => '7e5900d9', @@ -775,10 +775,10 @@ return array( 'phabricator-darklog' => '3b869402', 'phabricator-darkmessage' => '26cd4b73', 'phabricator-dashboard-css' => '5a205b9d', - 'phabricator-diff-changeset' => 'a1a5dc46', + 'phabricator-diff-changeset' => 'dd1a6f34', 'phabricator-diff-changeset-list' => '57035863', 'phabricator-diff-inline' => '16e97ebc', - 'phabricator-diff-path-view' => 'ceb66010', + 'phabricator-diff-path-view' => '8207abf9', 'phabricator-diff-tree-view' => '5d83623b', 'phabricator-drag-and-drop-file-upload' => '4370900d', 'phabricator-draggable-list' => '0169e425', @@ -1645,6 +1645,9 @@ return array( 'javelin-dom', 'javelin-vector', ), + '8207abf9' => array( + 'javelin-dom', + ), 83754533 => array( 'javelin-install', 'javelin-util', @@ -1829,18 +1832,6 @@ return array( 'javelin-dom', 'javelin-workflow', ), - 'a1a5dc46' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - 'phabricator-diff-inline', - 'phabricator-diff-path-view', - ), 'a241536a' => array( 'javelin-install', ), @@ -2079,9 +2070,6 @@ return array( 'phuix-icon-view', 'phabricator-busy', ), - 'ceb66010' => array( - 'javelin-dom', - ), 'cef53b3e' => array( 'javelin-install', 'javelin-dom', @@ -2120,6 +2108,18 @@ return array( 'javelin-uri', 'phabricator-notification', ), + 'dd1a6f34' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-workflow', + 'javelin-router', + 'javelin-behavior-device', + 'javelin-vector', + 'phabricator-diff-inline', + 'phabricator-diff-path-view', + ), 'e150bd50' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index 80283e9eea..1dac4c137d 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -195,6 +195,10 @@ final class DifferentialChangesetViewController extends DifferentialController { $viewstate = $viewstate_engine->newViewStateFromRequest($request); + if ($viewstate->getDiscardResponse()) { + return new AphrontAjaxResponse(); + } + $parser = id(new DifferentialChangesetParser()) ->setViewer($viewer) ->setViewState($viewstate) diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index e9ae71d63e..870fd9af6b 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1895,6 +1895,7 @@ final class DifferentialChangesetParser extends Phobject { 'highlight' => $viewstate->getHighlightLanguage(), 'characterEncoding' => $viewstate->getCharacterEncoding(), 'documentEngine' => $viewstate->getDocumentEngineKey(), + 'isHidden' => $viewstate->getHidden(), ); return id(new PhabricatorChangesetResponse()) diff --git a/src/applications/differential/storage/DifferentialViewState.php b/src/applications/differential/storage/DifferentialViewState.php index adf23b6abd..763edd8460 100644 --- a/src/applications/differential/storage/DifferentialViewState.php +++ b/src/applications/differential/storage/DifferentialViewState.php @@ -60,12 +60,9 @@ final class DifferentialViewState $key, $default = null) { - $path_hash = $this->getChangesetPathHash($changeset); - - $entries = idxv($this->viewState, array('changesets', $path_hash, $key)); - if (!is_array($entries)) { - $entries = array(); - } + $entries = $this->getChangesetPropertyEntries( + $changeset, + $key); $entries = isort($entries, 'epoch'); @@ -77,6 +74,19 @@ final class DifferentialViewState return idx($entry, 'value', $default); } + public function getChangesetPropertyEntries( + DifferentialChangeset $changeset, + $key) { + $path_hash = $this->getChangesetPathHash($changeset); + + $entries = idxv($this->viewState, array('changesets', $path_hash, $key)); + if (!is_array($entries)) { + $entries = array(); + } + + return $entries; + } + public function getHasModifications() { return $this->hasModifications; } diff --git a/src/applications/diffusion/controller/DiffusionDiffController.php b/src/applications/diffusion/controller/DiffusionDiffController.php index d85c292cb7..17d6b85af1 100644 --- a/src/applications/diffusion/controller/DiffusionDiffController.php +++ b/src/applications/diffusion/controller/DiffusionDiffController.php @@ -69,6 +69,10 @@ final class DiffusionDiffController extends DiffusionController { $viewstate = $viewstate_engine->newViewStateFromRequest($request); + if ($viewstate->getDiscardResponse()) { + return new AphrontAjaxResponse(); + } + $parser = id(new DifferentialChangesetParser()) ->setViewer($viewer) ->setChangeset($changeset) diff --git a/src/infrastructure/diff/viewstate/PhabricatorChangesetViewState.php b/src/infrastructure/diff/viewstate/PhabricatorChangesetViewState.php index 4d29a8fd59..a0324cef9c 100644 --- a/src/infrastructure/diff/viewstate/PhabricatorChangesetViewState.php +++ b/src/infrastructure/diff/viewstate/PhabricatorChangesetViewState.php @@ -8,6 +8,9 @@ final class PhabricatorChangesetViewState private $documentEngineKey; private $rendererKey; private $defaultDeviceRendererKey; + private $hidden; + private $modifiedSinceHide; + private $discardResponse; public function setHighlightLanguage($highlight_language) { $this->highlightLanguage = $highlight_language; @@ -54,4 +57,31 @@ final class PhabricatorChangesetViewState return $this->defaultDeviceRendererKey; } + public function setHidden($hidden) { + $this->hidden = $hidden; + return $this; + } + + public function getHidden() { + return $this->hidden; + } + + public function setModifiedSinceHide($modified_since_hide) { + $this->modifiedSinceHide = $modified_since_hide; + return $this; + } + + public function getModifiedSinceHide() { + return $this->modifiedSinceHide; + } + + public function setDiscardResponse($discard_response) { + $this->discardResponse = $discard_response; + return $this; + } + + public function getDiscardResponse() { + return $this->discardResponse; + } + } diff --git a/src/infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php b/src/infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php index 96e544f560..a506ec38dc 100644 --- a/src/infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php +++ b/src/infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php @@ -60,6 +60,11 @@ final class PhabricatorChangesetViewStateEngine $this->setChangesetProperty('renderer', $renderer); } + $hidden = $request->getStr('hidden'); + if ($hidden !== null) { + $this->setChangesetProperty('hidden', (int)$hidden); + } + $this->saveViewStateStorage(); $state = new PhabricatorChangesetViewState(); @@ -76,14 +81,21 @@ final class PhabricatorChangesetViewStateEngine $renderer = $this->getChangesetProperty('renderer'); $state->setRendererKey($renderer); + $this->updateHiddenState($state); + // This is the client-selected default renderer based on viewport // dimensions. $device_key = $request->getStr('device'); - if ($device_key !== null && strlen($device_key)) { + if ($device_key !== null) { $state->setDefaultDeviceRendererKey($device_key); } + $discard_response = $request->getStr('discard'); + if ($discard_response !== null) { + $state->setDiscardResponse(true); + } + return $state; } @@ -174,4 +186,50 @@ final class PhabricatorChangesetViewStateEngine unset($unguarded); } + private function updateHiddenState(PhabricatorChangesetViewState $state) { + $is_hidden = false; + $was_modified = false; + + $storage = $this->getStorage(); + $changeset = $this->getChangeset(); + + $entries = $storage->getChangesetPropertyEntries($changeset, 'hidden'); + $entries = isort($entries, 'epoch'); + + if ($entries) { + $other_phid = last_key($entries); + $other_spec = last($entries); + + $this_version = (int)$changeset->getDiffID(); + $other_version = (int)idx($other_spec, 'diffID'); + $other_value = (bool)idx($other_spec, 'value', false); + + if ($other_value === false) { + $is_hidden = false; + } else if ($other_version >= $this_version) { + $is_hidden = $other_value; + } else { + $viewer = $this->getViewer(); + + $other_changeset = id(new DifferentialChangesetQuery()) + ->setViewer($viewer) + ->withPHIDs(array($other_phid)) + ->executeOne(); + + $is_modified = false; + if ($other_changeset) { + if (!$changeset->hasSameEffectAs($other_changeset)) { + $is_modified = true; + } + } + + $is_hidden = false; + $was_modified = true; + } + } + + $state->setHidden($is_hidden); + $state->setModifiedSinceHide($was_modified); + } + } diff --git a/webroot/rsrc/css/application/diff/diff-tree-view.css b/webroot/rsrc/css/application/diff/diff-tree-view.css index cc6ac934b5..33174d88d7 100644 --- a/webroot/rsrc/css/application/diff/diff-tree-view.css +++ b/webroot/rsrc/css/application/diff/diff-tree-view.css @@ -59,6 +59,19 @@ opacity: 0.5; } +.diff-tree-path-hidden { + opacity: 0.25; +} + +.diff-tree-path-icon-hidden, +.diff-tree-path-hidden .diff-tree-path-icon-kind { + display: none; +} + +.diff-tree-path-hidden .diff-tree-path-icon-hidden { + display: block; +} + .diff-tree-path-owned { border-left-color: {$orange}; box-shadow: inset 2px 0 {$lightorange}; diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js index a6772a034a..6afefcf7d9 100644 --- a/webroot/rsrc/js/application/diff/DiffChangeset.js +++ b/webroot/rsrc/js/application/diff/DiffChangeset.js @@ -39,6 +39,7 @@ JX.install('DiffChangeset', { this._pathIconColor = data.pathIconColor; this._isLowImportance = data.isLowImportance; this._isOwned = data.isOwned; + this._isLoading = true; this._inlines = []; @@ -81,6 +82,7 @@ JX.install('DiffChangeset', { _pathIconColor: null, _isLowImportance: null, _isOwned: null, + _isHidden: null, getEditorURI: function() { return this._editorURI; @@ -202,14 +204,13 @@ JX.install('DiffChangeset', { this._loaded = true; this._sequence++; - var params = this._getViewParameters(state); - var pht = this.getChangesetList().getTranslations(); - - var workflow = new JX.Workflow(this._renderURI, params) + var workflow = this._newReloadWorkflow(state) .setHandler(JX.bind(this, this._onresponse, this._sequence)); this._startContentWorkflow(workflow); + var pht = this.getChangesetList().getTranslations(); + JX.DOM.setContent( this._getContentFrame(), JX.$N( @@ -220,6 +221,11 @@ JX.install('DiffChangeset', { return this; }, + _newReloadWorkflow: function(state) { + var params = this._getViewParameters(state); + return new JX.Workflow(this._renderURI, params); + }, + /** * Load missing context in a changeset. * @@ -637,6 +643,15 @@ JX.install('DiffChangeset', { this._highlight = state.highlight; this._characterEncoding = state.characterEncoding; this._documentEngine = state.documentEngine; + this._isHidden = state.isHidden; + + var is_hidden = !this.isVisible(); + if (this._isHidden != is_hidden) { + this.setVisible(!this._isHidden); + } + + this._isLoading = false; + this.getPathView().setIsLoading(this._isLoading); }, _getContentFrame: function() { @@ -844,7 +859,21 @@ JX.install('DiffChangeset', { }, toggleVisibility: function() { - this._visible = !this._visible; + this.setVisible(!this._visible); + + var attrs = { + hidden: this.isVisible() ? 0 : 1, + discard: 1 + }; + + var workflow = this._newReloadWorkflow(attrs) + .setHandler(JX.bag); + + this._startContentWorkflow(workflow); + }, + + setVisible: function(visible) { + this._visible = visible; var diff = JX.DOM.find(this._node, 'table', 'differential-diff'); var undo = this._getUndoNode(); @@ -858,6 +887,8 @@ JX.install('DiffChangeset', { } JX.Stratcom.invoke('resize'); + + this.getPathView().setIsHidden(!this._visible); }, isVisible: function() { @@ -906,7 +937,8 @@ JX.install('DiffChangeset', { .setChangeset(this) .setPath(this._pathParts) .setIsLowImportance(this._isLowImportance) - .setIsOwned(this._isOwned); + .setIsOwned(this._isOwned) + .setIsLoading(this._isLoading); view.getIcon() .setIcon(this._pathIconIcon) diff --git a/webroot/rsrc/js/application/diff/DiffPathView.js b/webroot/rsrc/js/application/diff/DiffPathView.js index 92441254a9..58c8866c21 100644 --- a/webroot/rsrc/js/application/diff/DiffPathView.js +++ b/webroot/rsrc/js/application/diff/DiffPathView.js @@ -23,8 +23,10 @@ JX.install('DiffPathView', { _inlineNode: null, _isDirectory: false, _displayPath: null, - _isOwned: false, _isLowImportance: false, + _isOwned: false, + _isHidden: false, + _isLoading: false, getNode: function() { if (!this._node) { @@ -142,6 +144,24 @@ JX.install('DiffPathView', { return this; }, + setIsHidden: function(hidden) { + this._isHidden = hidden; + + var node = this.getNode(); + JX.DOM.alterClass(node, 'diff-tree-path-hidden', this._isHidden); + + return this; + }, + + setIsLoading: function(loading) { + this._isLoading = loading; + + var node = this.getNode(); + JX.DOM.alterClass(node, 'diff-tree-path-loading', this._isLoading); + + return this; + }, + _onclick: function(e) { if (!e.isNormalClick()) { return; @@ -163,6 +183,7 @@ JX.install('DiffPathView', { var content = [ this.getInlineNode(), + this._getHiddenIconNode(), this._getIconNode(), this._getPathNode(), ]; @@ -186,13 +207,32 @@ JX.install('DiffPathView', { _getIconNode: function() { if (!this._iconNode) { var attrs = { - className: 'diff-tree-path-icon', + className: 'diff-tree-path-icon diff-tree-path-icon-kind', }; this._iconNode = JX.$N('div', attrs, this.getIcon().getNode()); } return this._iconNode; }, + _getHiddenIconNode: function() { + if (!this._hiddenIconNode) { + var attrs = { + className: 'diff-tree-path-icon diff-tree-path-icon-hidden', + }; + this._hiddenIconNode = + JX.$N('div', attrs, this._getHiddenIcon().getNode()); + } + return this._hiddenIconNode; + }, + + _getHiddenIcon: function() { + if (!this._hiddenIcon) { + this._hiddenIcon = new JX.PHUIXIconView() + .setIcon('fa-times-circle-o'); + } + return this._hiddenIcon; + }, + getInlineNode: function() { if (!this._inlineNode) { var attrs = {