diff --git a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php index 2c8f4e0bd9..7d0eae99c2 100644 --- a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php +++ b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php @@ -34,9 +34,7 @@ final class PhabricatorDifferentialConfigOptions new DifferentialReviewedByField(), new DifferentialSubscribersField(), new DifferentialRepositoryField(), - new DifferentialLintField(), new DifferentialProjectsField(), - new DifferentialUnitField(), new DifferentialViewPolicyField(), new DifferentialEditPolicyField(), @@ -54,6 +52,8 @@ final class PhabricatorDifferentialConfigOptions new DifferentialBlameRevisionField(), new DifferentialPathField(), new DifferentialHostField(), + new DifferentialLintField(), + new DifferentialUnitField(), new DifferentialRevertPlanField(), new DifferentialApplyPatchField(), diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index eb0e38aec5..b8e040c50a 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -206,27 +206,6 @@ final class DifferentialRevisionViewController extends DifferentialController { $visible_changesets = $changesets; } - - // TODO: This should be in a DiffQuery or similar. - $need_props = array(); - foreach ($field_list->getFields() as $field) { - foreach ($field->getRequiredDiffPropertiesForRevisionView() as $prop) { - $need_props[$prop] = $prop; - } - } - - if ($need_props) { - $prop_diff = $revision->getActiveDiff(); - $load_props = id(new DifferentialDiffProperty())->loadAllWhere( - 'diffID = %d AND name IN (%Ls)', - $prop_diff->getID(), - $need_props); - $load_props = mpull($load_props, 'getData', 'getName'); - foreach ($need_props as $need) { - $prop_diff->attachProperty($need, idx($load_props, $need)); - } - } - $commit_hashes = mpull($diffs, 'getSourceControlBaseRevision'); $local_commits = idx($props, 'local:commits', array()); foreach ($local_commits as $local_commit) { @@ -286,6 +265,11 @@ final class DifferentialRevisionViewController extends DifferentialController { $revision_detail_box->setInfoView($revision_warnings); } + $diff_detail_box = $this->buildDiffDetailView( + array_select_keys($diffs, array($diff_vs, $target->getID())), + $revision, + $field_list); + $comment_view = $this->buildTransactions( $revision, $diff_vs ? $diffs[$diff_vs] : $target, @@ -486,6 +470,7 @@ final class DifferentialRevisionViewController extends DifferentialController { $content = array( $revision_detail_box, + $diff_detail_box, $page_pane, ); @@ -972,4 +957,85 @@ final class DifferentialRevisionViewController extends DifferentialController { return $warnings; } + private function buildDiffDetailView( + array $diffs, + DifferentialRevision $revision, + PhabricatorCustomFieldList $field_list) { + $viewer = $this->getViewer(); + + $fields = array(); + foreach ($field_list->getFields() as $field) { + if ($field->shouldAppearInDiffPropertyView()) { + $fields[] = $field; + } + } + + if (!$fields) { + return null; + } + + // Make sure we're only going to render unique diffs. + $diffs = mpull($diffs, null, 'getID'); + $labels = array(pht('Left'), pht('Right')); + + $property_lists = array(); + foreach ($diffs as $diff) { + if (count($diffs) == 2) { + $label = array_shift($labels); + $label = pht('%s (Diff %d)', $label, $diff->getID()); + } else { + $label = pht('Diff %d', $diff->getID()); + } + + $property_lists[] = array( + $label, + $this->buildDiffPropertyList($diff, $revision, $fields), + ); + } + + $box = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Diff Detail')) + ->setUser($viewer); + + $last_tab = null; + foreach ($property_lists as $key => $property_list) { + list($tab_name, $list_view) = $property_list; + + $tab = id(new PHUIListItemView()) + ->setKey($key) + ->setName($tab_name); + + $box->addPropertyList($list_view, $tab); + $last_tab = $tab; + } + + if ($last_tab) { + $last_tab->setSelected(true); + } + + return $box; + } + + private function buildDiffPropertyList( + DifferentialDiff $diff, + DifferentialRevision $revision, + array $fields) { + $viewer = $this->getViewer(); + + $view = id(new PHUIPropertyListView()) + ->setUser($viewer) + ->setObject($diff); + + foreach ($fields as $field) { + $label = $field->renderDiffPropertyViewLabel($diff); + $value = $field->renderDiffPropertyViewValue($diff); + if ($value !== null) { + $view->addProperty($label, $value); + } + } + + return $view; + } + + } diff --git a/src/applications/differential/customfield/DifferentialBranchField.php b/src/applications/differential/customfield/DifferentialBranchField.php index 35ed809f12..fc50943f1d 100644 --- a/src/applications/differential/customfield/DifferentialBranchField.php +++ b/src/applications/differential/customfield/DifferentialBranchField.php @@ -19,12 +19,20 @@ final class DifferentialBranchField return true; } - public function renderPropertyViewLabel() { + public function renderPropertyViewValue(array $handles) { + return null; + } + + public function shouldAppearInDiffPropertyView() { + return true; + } + + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { return $this->getFieldName(); } - public function renderPropertyViewValue(array $handles) { - return $this->getBranchDescription($this->getObject()->getActiveDiff()); + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + return $this->getBranchDescription($diff); } private function getBranchDescription(DifferentialDiff $diff) { diff --git a/src/applications/differential/customfield/DifferentialCustomField.php b/src/applications/differential/customfield/DifferentialCustomField.php index 088e7beeac..b4ef47c274 100644 --- a/src/applications/differential/customfield/DifferentialCustomField.php +++ b/src/applications/differential/customfield/DifferentialCustomField.php @@ -2,6 +2,7 @@ /** * @task commitmessage Integration with Commit Messages + * @task diff Integration with Diff Properties */ abstract class DifferentialCustomField extends PhabricatorCustomField { @@ -31,13 +32,6 @@ abstract class DifferentialCustomField return parent::shouldEnableForRole($role); } - public function getRequiredDiffPropertiesForRevisionView() { - if ($this->getProxy()) { - return $this->getProxy()->getRequiredDiffPropertiesForRevisionView(); - } - return array(); - } - protected function parseObjectList( $value, array $types, @@ -82,6 +76,7 @@ abstract class DifferentialCustomField return array(); } + /* -( Integration with Commit Messages )----------------------------------- */ @@ -217,4 +212,40 @@ abstract class DifferentialCustomField return; } + +/* -( Integration with Diff Properties )----------------------------------- */ + + + /** + * @task diff + */ + public function shouldAppearInDiffPropertyView() { + if ($this->getProxy()) { + return $this->getProxy()->shouldAppearInDiffPropertyView(); + } + return false; + } + + + /** + * @task diff + */ + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { + if ($this->proxy) { + return $this->proxy->renderDiffPropertyViewLabel($diff); + } + return $this->getFieldName(); + } + + + /** + * @task diff + */ + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + if ($this->proxy) { + return $this->proxy->renderDiffPropertyViewValue($diff); + } + throw new PhabricatorCustomFieldImplementationIncompleteException($this); + } + } diff --git a/src/applications/differential/customfield/DifferentialHostField.php b/src/applications/differential/customfield/DifferentialHostField.php index 2c5c65b8c1..af71dcf987 100644 --- a/src/applications/differential/customfield/DifferentialHostField.php +++ b/src/applications/differential/customfield/DifferentialHostField.php @@ -23,12 +23,20 @@ final class DifferentialHostField return true; } - public function renderPropertyViewLabel() { + public function renderPropertyViewValue(array $handles) { + return null; + } + + public function shouldAppearInDiffPropertyView() { + return true; + } + + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { return $this->getFieldName(); } - public function renderPropertyViewValue(array $handles) { - $host = $this->getObject()->getActiveDiff()->getSourceMachine(); + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + $host = $diff->getSourceMachine(); if (!$host) { return null; } diff --git a/src/applications/differential/customfield/DifferentialLintField.php b/src/applications/differential/customfield/DifferentialLintField.php index c43034dd06..94df1b7e07 100644 --- a/src/applications/differential/customfield/DifferentialLintField.php +++ b/src/applications/differential/customfield/DifferentialLintField.php @@ -19,20 +19,37 @@ final class DifferentialLintField return true; } - public function renderPropertyViewLabel() { + public function renderPropertyViewValue(array $handles) { + return null; + } + + public function shouldAppearInDiffPropertyView() { + return true; + } + + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { return $this->getFieldName(); } - public function getRequiredDiffPropertiesForRevisionView() { - return array( + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + // TODO: This load is slightly inefficient, but most of this is moving + // to Harbormaster and this simplifies the transition. Eat 1-2 extra + // queries for now. + $keys = array( 'arc:lint', 'arc:lint-excuse', 'arc:lint-postponed', ); - } - public function renderPropertyViewValue(array $handles) { - $diff = $this->getObject()->getActiveDiff(); + $properties = id(new DifferentialDiffProperty())->loadAllWhere( + 'diffID = %d AND name IN (%Ls)', + $diff->getID(), + $keys); + $properties = mpull($properties, 'getData', 'getName'); + + foreach ($keys as $key) { + $diff->attachProperty($key, idx($properties, $key)); + } $path_changesets = mpull($diff->loadChangesets(), 'getID', 'getFilename'); diff --git a/src/applications/differential/customfield/DifferentialPathField.php b/src/applications/differential/customfield/DifferentialPathField.php index 1eb0ffbf50..39ca8abd50 100644 --- a/src/applications/differential/customfield/DifferentialPathField.php +++ b/src/applications/differential/customfield/DifferentialPathField.php @@ -23,12 +23,20 @@ final class DifferentialPathField return true; } - public function renderPropertyViewLabel() { + public function renderPropertyViewValue(array $handles) { + return null; + } + + public function shouldAppearInDiffPropertyView() { + return true; + } + + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { return $this->getFieldName(); } - public function renderPropertyViewValue(array $handles) { - $path = $this->getObject()->getActiveDiff()->getSourcePath(); + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + $path = $diff->getSourcePath(); if (!$path) { return null; } diff --git a/src/applications/differential/customfield/DifferentialRepositoryField.php b/src/applications/differential/customfield/DifferentialRepositoryField.php index fc6c88194a..7f39d25823 100644 --- a/src/applications/differential/customfield/DifferentialRepositoryField.php +++ b/src/applications/differential/customfield/DifferentialRepositoryField.php @@ -125,20 +125,24 @@ final class DifferentialRepositoryField return true; } - public function renderPropertyViewLabel() { + public function renderPropertyViewValue(array $handles) { + return null; + } + + public function shouldAppearInDiffPropertyView() { + return true; + } + + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { return $this->getFieldName(); } - public function getRequiredHandlePHIDsForPropertyView() { - $repository_phid = $this->getObject()->getRepositoryPHID(); - if ($repository_phid) { - return array($repository_phid); + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + if (!$diff->getRepositoryPHID()) { + return null; } - return array(); - } - public function renderPropertyViewValue(array $handles) { - return $this->renderHandleList($handles); + return $this->getViewer()->renderHandle($diff->getRepositoryPHID()); } public function shouldAppearInTransactionMail() { diff --git a/src/applications/differential/customfield/DifferentialUnitField.php b/src/applications/differential/customfield/DifferentialUnitField.php index 2cc86d2660..56ea06d3ae 100644 --- a/src/applications/differential/customfield/DifferentialUnitField.php +++ b/src/applications/differential/customfield/DifferentialUnitField.php @@ -19,19 +19,34 @@ final class DifferentialUnitField return true; } - public function renderPropertyViewLabel() { + public function renderPropertyViewValue(array $handles) { + return null; + } + + public function shouldAppearInDiffPropertyView() { + return true; + } + + public function renderDiffPropertyViewLabel(DifferentialDiff $diff) { return $this->getFieldName(); } - public function getRequiredDiffPropertiesForRevisionView() { - return array( + public function renderDiffPropertyViewValue(DifferentialDiff $diff) { + // TODO: See DifferentialLintField. + $keys = array( 'arc:unit', 'arc:unit-excuse', ); - } - public function renderPropertyViewValue(array $handles) { - $diff = $this->getObject()->getActiveDiff(); + $properties = id(new DifferentialDiffProperty())->loadAllWhere( + 'diffID = %d AND name IN (%Ls)', + $diff->getID(), + $keys); + $properties = mpull($properties, 'getData', 'getName'); + + foreach ($keys as $key) { + $diff->attachProperty($key, idx($properties, $key)); + } $ustar = DifferentialRevisionUpdateHistoryView::renderDiffUnitStar($diff); $umsg = DifferentialRevisionUpdateHistoryView::getDiffUnitMessage($diff); diff --git a/src/applications/harbormaster/event/HarbormasterUIEventListener.php b/src/applications/harbormaster/event/HarbormasterUIEventListener.php index 7df5c111bd..e965e67fb6 100644 --- a/src/applications/harbormaster/event/HarbormasterUIEventListener.php +++ b/src/applications/harbormaster/event/HarbormasterUIEventListener.php @@ -31,6 +31,13 @@ final class HarbormasterUIEventListener return; } + if ($object instanceof DifferentialRevision) { + // TODO: This is a bit hacky and we could probably find a cleaner fix + // eventually, but we show build status on each diff, immediately below + // this property list, so it's redundant to show it on the revision view. + return; + } + if (!($object instanceof HarbormasterBuildableInterface)) { return; } diff --git a/src/applications/policy/controller/PhabricatorPolicyExplainController.php b/src/applications/policy/controller/PhabricatorPolicyExplainController.php index 09fd69242a..4224319d9c 100644 --- a/src/applications/policy/controller/PhabricatorPolicyExplainController.php +++ b/src/applications/policy/controller/PhabricatorPolicyExplainController.php @@ -69,7 +69,27 @@ final class PhabricatorPolicyExplainController $capability_name = $capobj->getCapabilityName(); } + $space_info = null; + if ($object instanceof PhabricatorSpacesInterface) { + if (PhabricatorSpacesNamespaceQuery::getViewerSpacesExist($viewer)) { + $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID( + $object); + + $handles = $viewer->loadHandles(array($space_phid)); + + $space_info = array( + pht( + 'This object is in %s, and can only be seen by users with '. + 'access to that space.', + $handles[$space_phid]->renderLink()), + phutil_tag('br'), + phutil_tag('br'), + ); + } + } + $content = array( + $space_info, pht('Users with the "%s" capability:', $capability_name), $auto_info, );