From 7ef2bb1b569bd0e6c989a78b701e726fcab3600b Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 23 Aug 2018 14:59:42 -0700 Subject: [PATCH 1/6] Support Mercurial "protocaps" wire command Summary: Ref T13187. See PHI834. Mercurial has somewhat-recently (changeset is from Jan 2018) introduced a new "protocaps" command, that appears in Mercurial 4.7 and possibly before then. We must explicitly enumerate all protocol commands because you can't decode the protocol without knowing how many arguments the command expects, so enumerate it. (Also fix an issue where the related error message had an extra apostrophe.) Test Plan: - Ran `hg clone ...` with client and server on Mercurial 4.7. - Before: fatal on unknown "protocaps" command. - Midway: better typography in error message. - After: clean clone. Reviewers: amckinley Maniphest Tasks: T13187 Differential Revision: https://secure.phabricator.com/D19596 --- .../diffusion/protocol/DiffusionMercurialWireProtocol.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php b/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php index 251935a6dc..8a9deb5d84 100644 --- a/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php +++ b/src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php @@ -23,12 +23,16 @@ final class DiffusionMercurialWireProtocol extends Phobject { 'listkeys' => array('namespace'), 'lookup' => array('key'), 'pushkey' => array('namespace', 'key', 'old', 'new'), + 'protocaps' => array('caps'), 'stream_out' => array(''), 'unbundle' => array('heads'), ); if (!isset($commands[$command])) { - throw new Exception(pht("Unknown Mercurial command '%s!", $command)); + throw new Exception( + pht( + 'Unknown Mercurial command "%s"!', + $command)); } return $commands[$command]; @@ -49,6 +53,7 @@ final class DiffusionMercurialWireProtocol extends Phobject { 'known' => true, 'listkeys' => true, 'lookup' => true, + 'protocaps' => true, 'stream_out' => true, ); From ca618a86794f5d3b7df737a6c096bfbad8501b3e Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 24 Aug 2018 08:04:46 -0700 Subject: [PATCH 2/6] Document that `phd.taskmasters` is a local setting, per daemon Summary: Ref T13187. See PHI807. The documentation currently does not make it very clear that this is a local setting, per `phd` process. Make it more clear. Test Plan: {F5827757} Reviewers: amckinley Maniphest Tasks: T13187 Differential Revision: https://secure.phabricator.com/D19597 --- .../config/option/PhabricatorPHDConfigOptions.php | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/applications/config/option/PhabricatorPHDConfigOptions.php b/src/applications/config/option/PhabricatorPHDConfigOptions.php index bfe7c148c4..37fae45dfb 100644 --- a/src/applications/config/option/PhabricatorPHDConfigOptions.php +++ b/src/applications/config/option/PhabricatorPHDConfigOptions.php @@ -34,9 +34,14 @@ final class PhabricatorPHDConfigOptions ->setSummary(pht('Maximum taskmaster daemon pool size.')) ->setDescription( pht( - 'Maximum number of taskmaster daemons to run at once. Raising '. - 'this can increase the maximum throughput of the task queue. The '. - 'pool will automatically scale down when unutilized.')), + "Maximum number of taskmaster daemons to run at once. Raising ". + "this can increase the maximum throughput of the task queue. The ". + "pool will automatically scale down when unutilized.". + "\n\n". + "If you are running a cluster, this limit applies separately ". + "to each instance of `phd`. For example, if this limit is set ". + "to `4` and you have three hosts running daemons, the effective ". + "global limit will be 12.")), $this->newOption('phd.verbose', 'bool', false) ->setLocked(true) ->setBoolOptions( From 5e4d9dfa9219cb50b9d3559437855855fb8eb854 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 24 Aug 2018 09:50:36 -0700 Subject: [PATCH 3/6] Condition "Changes Since Last Action" Differential link on "first broadcast", not "new object" Summary: Ref T13187. Ref T13176. With drafts, we actually want to suppress this link on "first broadcast" (the first time we send mail), not on "new object" (when the revision is created as a draft). Test Plan: Poked at this locally, will keep an eye on it in production. Reviewers: amckinley Maniphest Tasks: T13187, T13176 Differential Revision: https://secure.phabricator.com/D19598 --- .../differential/editor/DifferentialTransactionEditor.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index f4dd3f9fe3..9c24a50b4e 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -671,7 +671,7 @@ final class DifferentialTransactionEditor $this->addCustomFieldsToMailBody($body, $object, $xactions); - if (!$this->getIsNewObject()) { + if (!$this->isFirstBroadcast()) { $body->addLinkSection(pht('CHANGES SINCE LAST ACTION'), $new_uri); } From b6fa009cf00a6bbd894a49d519e6c1d03a999790 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 24 Aug 2018 10:00:37 -0700 Subject: [PATCH 4/6] Enrich "priority" transactions in Maniphest for "transaction.search" Summary: Ref T13187. See . We can reasonably enrich these transactions. Since priorities don't have unique authorative string identifiers, I've mostly mimicked the `maniphest.search` structure. Test Plan: Called `transaction.search` on tasks which were: created normally, created with a priority change, saw a priority change after creation. All the output looked useful and sensible. Reviewers: amckinley Maniphest Tasks: T13187 Differential Revision: https://secure.phabricator.com/D19599 --- .../ManiphestTaskPriorityTransaction.php | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/applications/maniphest/xaction/ManiphestTaskPriorityTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskPriorityTransaction.php index 83f38fc659..398808e006 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskPriorityTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskPriorityTransaction.php @@ -172,4 +172,33 @@ final class ManiphestTaskPriorityTransaction return $errors; } + public function getTransactionTypeForConduit($xaction) { + return 'priority'; + } + + public function getFieldValuesForConduit($xaction, $data) { + $old = $xaction->getOldValue(); + if ($old !== null) { + $old = (int)$old; + $old_name = ManiphestTaskPriority::getTaskPriorityName($old); + } else { + $old_name = null; + } + + $new = $xaction->getNewValue(); + $new = (int)$new; + $new_name = ManiphestTaskPriority::getTaskPriorityName($new); + + return array( + 'old' => array( + 'value' => $old, + 'name' => $old_name, + ), + 'new' => array( + 'value' => $new, + 'name' => $new_name, + ), + ); + } + } From 8a6d767843631e1858459cf7da018388d022870a Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 24 Aug 2018 10:07:47 -0700 Subject: [PATCH 5/6] Fix a minor text alignment issue for static text comment actions like "Accept Revision" Summary: Ref T13187. See PHI836. The "action" comment actions in Differential (Accept, Reject, etc) render a single line of descriptive text. This is currently slightly misaligned. Give it similar sizing information to the label element to the left, so it lines up properly. Test Plan: Note that "Request Review" and "This revision will be..." are now aligned: {F5828077} Reviewers: amckinley Maniphest Tasks: T13187 Differential Revision: https://secure.phabricator.com/D19600 --- resources/celerity/map.php | 6 +++--- webroot/rsrc/css/phui/phui-form-view.css | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index c60c2f7e85..66a568dabe 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => '15191c65', - 'core.pkg.css' => 'f515619b', + 'core.pkg.css' => 'fc4839c8', 'core.pkg.js' => '2058ec09', 'differential.pkg.css' => '06dc617c', 'differential.pkg.js' => 'c1cfa143', @@ -151,7 +151,7 @@ return array( 'rsrc/css/phui/phui-document.css' => '878c2f52', 'rsrc/css/phui/phui-feed-story.css' => '44a9c8e9', 'rsrc/css/phui/phui-fontkit.css' => '1320ed01', - 'rsrc/css/phui/phui-form-view.css' => 'ae9f8d16', + 'rsrc/css/phui/phui-form-view.css' => 'f808e5be', 'rsrc/css/phui/phui-form.css' => '7aaa04e3', 'rsrc/css/phui/phui-head-thing.css' => 'fd311e5f', 'rsrc/css/phui/phui-header-view.css' => 'edeb9252', @@ -819,7 +819,7 @@ return array( 'phui-font-icon-base-css' => '870a7360', 'phui-fontkit-css' => '1320ed01', 'phui-form-css' => '7aaa04e3', - 'phui-form-view-css' => 'ae9f8d16', + 'phui-form-view-css' => 'f808e5be', 'phui-head-thing-view-css' => 'fd311e5f', 'phui-header-view-css' => 'edeb9252', 'phui-hovercard' => '1bd28176', diff --git a/webroot/rsrc/css/phui/phui-form-view.css b/webroot/rsrc/css/phui/phui-form-view.css index 1b7400656a..9824196882 100644 --- a/webroot/rsrc/css/phui/phui-form-view.css +++ b/webroot/rsrc/css/phui/phui-form-view.css @@ -546,7 +546,8 @@ properly, and submit values. */ } .phui-form-static-action { - padding: 4px; + height: 28px; + line-height: 28px; color: {$bluetext}; } From b584834b193413806e91a03d0e94508f3bcc34e5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 24 Aug 2018 10:18:28 -0700 Subject: [PATCH 6/6] In Differential: when the file tree is enabled, default to the "History" tab instead of "Files" Summary: Ref T13187. See PHI811. If the file tree is enabled and visible, set the default tab to "History". - This is a bit magic. - It won't work entirely on mobile (we can't tell that you're on mobile on the server, so we'll pick the "History" tab even though the file tree isn't actually visible on your device). - There's no corresponding logic in Diffusion. Diffusion doesn't have the same tab layout, but this makes things somewhat inconsistent. So I don't love this, but we can try it and see if it's confusing or helpful on the balance. Test Plan: With filetree on and off, reloaded revisions. Saw appropriate tab selected by default. Reviewers: amckinley Maniphest Tasks: T13187 Differential Revision: https://secure.phabricator.com/D19601 --- .../DifferentialRevisionViewController.php | 46 +++++++++++-------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 33440e535f..b809a8ee80 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -463,7 +463,7 @@ final class DifferentialRevisionViewController } } - $tab_group = id(new PHUITabGroupView()); + $tab_group = new PHUITabGroupView(); if ($toc_view) { $tab_group->addTab( @@ -473,17 +473,30 @@ final class DifferentialRevisionViewController ->appendChild($toc_view)); } - $tab_group - ->addTab( - id(new PHUITabView()) - ->setName(pht('History')) - ->setKey('history') - ->appendChild($history)) - ->addTab( - id(new PHUITabView()) - ->setName(pht('Commits')) - ->setKey('commits') - ->appendChild($local_table)); + $tab_group->addTab( + id(new PHUITabView()) + ->setName(pht('History')) + ->setKey('history') + ->appendChild($history)); + + $filetree_on = $viewer->compareUserSetting( + PhabricatorShowFiletreeSetting::SETTINGKEY, + PhabricatorShowFiletreeSetting::VALUE_ENABLE_FILETREE); + + $collapsed_key = PhabricatorFiletreeVisibleSetting::SETTINGKEY; + $filetree_collapsed = (bool)$viewer->getUserSetting($collapsed_key); + + // See PHI811. If the viewer has the file tree on, the files tab with the + // table of contents is redundant, so default to the "History" tab instead. + if ($filetree_on && !$filetree_collapsed) { + $tab_group->selectTab('history'); + } + + $tab_group->addTab( + id(new PHUITabView()) + ->setName(pht('Commits')) + ->setKey('commits') + ->appendChild($local_table)); $stack_graph = id(new DifferentialRevisionGraph()) ->setViewer($viewer) @@ -601,22 +614,15 @@ final class DifferentialRevisionViewController $crumbs->addTextCrumb($monogram); $crumbs->setBorder(true); - $filetree_on = $viewer->compareUserSetting( - PhabricatorShowFiletreeSetting::SETTINGKEY, - PhabricatorShowFiletreeSetting::VALUE_ENABLE_FILETREE); - $nav = null; if ($filetree_on && !$this->isVeryLargeDiff()) { - $collapsed_key = PhabricatorFiletreeVisibleSetting::SETTINGKEY; - $collapsed_value = $viewer->getUserSetting($collapsed_key); - $width_key = PhabricatorFiletreeWidthSetting::SETTINGKEY; $width_value = $viewer->getUserSetting($width_key); $nav = id(new DifferentialChangesetFileTreeSideNavBuilder()) ->setTitle($monogram) ->setBaseURI(new PhutilURI($revision->getURI())) - ->setCollapsed((bool)$collapsed_value) + ->setCollapsed($filetree_collapsed) ->setWidth((int)$width_value) ->build($changesets); }