From 5038b26018e9bd6e6fb2a2fbb5d0c1ac8d74a3fa Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 14 Aug 2011 12:33:54 -0700 Subject: [PATCH] Move Differential's read-only fields to the extensible field schema Summary: Move additional fields (which rely on loading handles) to the extensible field classes and out of hardcoding in the controller. Depends on D807. Test Plan: Viewed, edited, and hit conduit for revisions. Reviewed By: jungejason Reviewers: jungejason, tuomaspelkonen, aran CC: aran, jungejason, epriestley Differential Revision: 808 --- src/__phutil_library_map__.php | 10 +++ .../DifferentialRevisionViewController.php | 57 +++---------- .../controller/revisionview/__init__.php | 1 - .../DifferentialDefaultFieldSelector.php | 5 ++ .../field/selector/default/__init__.php | 5 ++ ...ntialArcanistProjectFieldSpecification.php | 54 ++++++++++++ .../arcanistproject/__init__.php | 14 ++++ .../DifferentialAuthorFieldSpecification.php | 46 ++++++++++ .../field/specification/author/__init__.php | 12 +++ .../base/DifferentialFieldSpecification.php | 84 +++++++++++++++++++ .../DifferentialCommitsFieldSpecification.php | 53 ++++++++++++ .../field/specification/commits/__init__.php | 12 +++ ...erentialDependenciesFieldSpecification.php | 54 ++++++++++++ .../specification/dependencies/__init__.php | 13 +++ ...entialManiphestTasksFieldSpecification.php | 54 ++++++++++++ .../specification/maniphesttasks/__init__.php | 14 ++++ 16 files changed, 443 insertions(+), 45 deletions(-) create mode 100644 src/applications/differential/field/specification/arcanistproject/DifferentialArcanistProjectFieldSpecification.php create mode 100644 src/applications/differential/field/specification/arcanistproject/__init__.php create mode 100644 src/applications/differential/field/specification/author/DifferentialAuthorFieldSpecification.php create mode 100644 src/applications/differential/field/specification/author/__init__.php create mode 100644 src/applications/differential/field/specification/commits/DifferentialCommitsFieldSpecification.php create mode 100644 src/applications/differential/field/specification/commits/__init__.php create mode 100644 src/applications/differential/field/specification/dependencies/DifferentialDependenciesFieldSpecification.php create mode 100644 src/applications/differential/field/specification/dependencies/__init__.php create mode 100644 src/applications/differential/field/specification/maniphesttasks/DifferentialManiphestTasksFieldSpecification.php create mode 100644 src/applications/differential/field/specification/maniphesttasks/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 688bbe2e3d..61100f848f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -134,6 +134,8 @@ phutil_register_library_map(array( 'DifferentialAction' => 'applications/differential/constants/action', 'DifferentialAddCommentView' => 'applications/differential/view/addcomment', 'DifferentialApplyPatchFieldSpecification' => 'applications/differential/field/specification/applypatch', + 'DifferentialArcanistProjectFieldSpecification' => 'applications/differential/field/specification/arcanistproject', + 'DifferentialAuthorFieldSpecification' => 'applications/differential/field/specification/author', 'DifferentialAuxiliaryField' => 'applications/differential/storage/auxiliaryfield', 'DifferentialBlameRevisionFieldSpecification' => 'applications/differential/field/specification/blamerev', 'DifferentialCCWelcomeMail' => 'applications/differential/mail/ccwelcome', @@ -153,9 +155,11 @@ phutil_register_library_map(array( 'DifferentialCommitMessageField' => 'applications/differential/data/commitmessage', 'DifferentialCommitMessageModifier' => 'applications/differential/data/commitmessage', 'DifferentialCommitMessageParserException' => 'applications/differential/parser/commitmessage/exception', + 'DifferentialCommitsFieldSpecification' => 'applications/differential/field/specification/commits', 'DifferentialController' => 'applications/differential/controller/base', 'DifferentialDAO' => 'applications/differential/storage/base', 'DifferentialDefaultFieldSelector' => 'applications/differential/field/selector/default', + 'DifferentialDependenciesFieldSpecification' => 'applications/differential/field/specification/dependencies', 'DifferentialDiff' => 'applications/differential/storage/diff', 'DifferentialDiffContentMail' => 'applications/differential/mail/diffcontent', 'DifferentialDiffCreateController' => 'applications/differential/controller/diffcreate', @@ -178,6 +182,7 @@ phutil_register_library_map(array( 'DifferentialLinesFieldSpecification' => 'applications/differential/field/specification/lines', 'DifferentialLintStatus' => 'applications/differential/constants/lintstatus', 'DifferentialMail' => 'applications/differential/mail/base', + 'DifferentialManiphestTasksFieldSpecification' => 'applications/differential/field/specification/maniphesttasks', 'DifferentialNewDiffMail' => 'applications/differential/mail/newdiff', 'DifferentialPathFieldSpecification' => 'applications/differential/field/specification/path', 'DifferentialPrimaryPaneView' => 'applications/differential/view/primarypane', @@ -782,6 +787,8 @@ phutil_register_library_map(array( 'DarkConsoleXHProfPlugin' => 'DarkConsolePlugin', 'DifferentialAddCommentView' => 'AphrontView', 'DifferentialApplyPatchFieldSpecification' => 'DifferentialFieldSpecification', + 'DifferentialArcanistProjectFieldSpecification' => 'DifferentialFieldSpecification', + 'DifferentialAuthorFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialAuxiliaryField' => 'DifferentialDAO', 'DifferentialBlameRevisionFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialCCWelcomeMail' => 'DifferentialReviewRequestMail', @@ -793,9 +800,11 @@ phutil_register_library_map(array( 'DifferentialCommentMail' => 'DifferentialMail', 'DifferentialCommentPreviewController' => 'DifferentialController', 'DifferentialCommentSaveController' => 'DifferentialController', + 'DifferentialCommitsFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialController' => 'PhabricatorController', 'DifferentialDAO' => 'PhabricatorLiskDAO', 'DifferentialDefaultFieldSelector' => 'DifferentialFieldSelector', + 'DifferentialDependenciesFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialDiff' => 'DifferentialDAO', 'DifferentialDiffContentMail' => 'DifferentialMail', 'DifferentialDiffCreateController' => 'DifferentialController', @@ -811,6 +820,7 @@ phutil_register_library_map(array( 'DifferentialInlineCommentPreviewController' => 'DifferentialController', 'DifferentialInlineCommentView' => 'AphrontView', 'DifferentialLinesFieldSpecification' => 'DifferentialFieldSpecification', + 'DifferentialManiphestTasksFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialNewDiffMail' => 'DifferentialReviewRequestMail', 'DifferentialPathFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialPrimaryPaneView' => 'AphrontView', diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index 823c46b53a..accf29754a 100644 --- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php @@ -104,20 +104,29 @@ class DifferentialRevisionViewController extends DifferentialController { } } - if ($target->getArcanistProjectPHID()) { - $object_phids[] = $target->getArcanistProjectPHID(); - } - foreach ($revision->getAttached() as $type => $phids) { foreach ($phids as $phid => $info) { $object_phids[] = $phid; } } + + $aux_phids = array(); + foreach ($aux_fields as $key => $aux_field) { + $aux_phids[$key] = $aux_field->getRequiredHandlePHIDsForRevisionView(); + } + $object_phids = array_merge($object_phids, array_mergev($aux_phids)); $object_phids = array_unique($object_phids); $handles = id(new PhabricatorObjectHandleData($object_phids)) ->loadHandles(); + foreach ($aux_fields as $key => $aux_field) { + // Make sure each field only has access to handles it specifically + // requested, not all handles. Otherwise you can get a field which works + // only in the presence of other fields. + $aux_field->setHandles(array_select_keys($handles, $aux_phids[$key])); + } + $request_uri = $request->getRequestURI(); $limit = 100; @@ -326,9 +335,6 @@ class DifferentialRevisionViewController extends DifferentialController { $status = DifferentialRevisionStatus::getNameForRevisionStatus($status); $properties['Revision Status'] = ''.$status.''.$next_step; - $author = $handles[$revision->getAuthorPHID()]; - $properties['Author'] = $author->renderLink(); - $properties['Reviewers'] = $this->renderHandleLinkList( array_select_keys( $handles, @@ -435,43 +441,6 @@ class DifferentialRevisionViewController extends DifferentialController { $properties['Unit'] = $ustar.' '.$umsg.$utail; - $drevs = $revision->getAttachedPHIDs( - PhabricatorPHIDConstants::PHID_TYPE_DREV); - if ($drevs) { - $links = array(); - foreach ($drevs as $drev_phid) { - $links[] = $handles[$drev_phid]->renderLink(); - } - $properties['Depends On'] = implode('
', $links); - } - - if (PhabricatorEnv::getEnvConfig('maniphest.enabled')) { - $tasks = $revision->getAttachedPHIDs( - PhabricatorPHIDConstants::PHID_TYPE_TASK); - if ($tasks) { - $links = array(); - foreach ($tasks as $task_phid) { - $links[] = $handles[$task_phid]->renderLink(); - } - $properties['Maniphest Tasks'] = implode('
', $links); - } - } - - $commit_phids = $revision->getCommitPHIDs(); - if ($commit_phids) { - $links = array(); - foreach ($commit_phids as $commit_phid) { - $links[] = $handles[$commit_phid]->renderLink(); - } - $properties['Commits'] = implode('
', $links); - } - - $arcanist_phid = $diff->getArcanistProjectPHID(); - if ($arcanist_phid) { - $properties['Arcanist Project'] = phutil_escape_html( - $handles[$arcanist_phid]->getName()); - } - return $properties; } diff --git a/src/applications/differential/controller/revisionview/__init__.php b/src/applications/differential/controller/revisionview/__init__.php index 28fddb9729..042008c08e 100644 --- a/src/applications/differential/controller/revisionview/__init__.php +++ b/src/applications/differential/controller/revisionview/__init__.php @@ -29,7 +29,6 @@ phutil_require_module('phabricator', 'applications/differential/view/revisioncom phutil_require_module('phabricator', 'applications/differential/view/revisiondetail'); phutil_require_module('phabricator', 'applications/differential/view/revisionupdatehistory'); phutil_require_module('phabricator', 'applications/draft/storage/draft'); -phutil_require_module('phabricator', 'applications/phid/constants'); phutil_require_module('phabricator', 'applications/phid/handle/data'); phutil_require_module('phabricator', 'applications/repository/constants/repositorytype'); phutil_require_module('phabricator', 'infrastructure/celerity/api'); diff --git a/src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php b/src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php index 62778ff174..5c31d0fb02 100644 --- a/src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php +++ b/src/applications/differential/field/selector/default/DifferentialDefaultFieldSelector.php @@ -21,9 +21,14 @@ final class DifferentialDefaultFieldSelector public function getFieldSpecifications() { return array( + new DifferentialAuthorFieldSpecification(), + new DifferentialDependenciesFieldSpecification(), + new DifferentialManiphestTasksFieldSpecification(), + new DifferentialCommitsFieldSpecification(), new DifferentialHostFieldSpecification(), new DifferentialPathFieldSpecification(), new DifferentialLinesFieldSpecification(), + new DifferentialArcanistProjectFieldSpecification(), new DifferentialApplyPatchFieldSpecification(), new DifferentialExportPatchFieldSpecification(), ); diff --git a/src/applications/differential/field/selector/default/__init__.php b/src/applications/differential/field/selector/default/__init__.php index 2c43e80899..b8cd112fb7 100644 --- a/src/applications/differential/field/selector/default/__init__.php +++ b/src/applications/differential/field/selector/default/__init__.php @@ -8,9 +8,14 @@ phutil_require_module('phabricator', 'applications/differential/field/selector/base'); phutil_require_module('phabricator', 'applications/differential/field/specification/applypatch'); +phutil_require_module('phabricator', 'applications/differential/field/specification/arcanistproject'); +phutil_require_module('phabricator', 'applications/differential/field/specification/author'); +phutil_require_module('phabricator', 'applications/differential/field/specification/commits'); +phutil_require_module('phabricator', 'applications/differential/field/specification/dependencies'); phutil_require_module('phabricator', 'applications/differential/field/specification/exportpatch'); phutil_require_module('phabricator', 'applications/differential/field/specification/host'); phutil_require_module('phabricator', 'applications/differential/field/specification/lines'); +phutil_require_module('phabricator', 'applications/differential/field/specification/maniphesttasks'); phutil_require_module('phabricator', 'applications/differential/field/specification/path'); diff --git a/src/applications/differential/field/specification/arcanistproject/DifferentialArcanistProjectFieldSpecification.php b/src/applications/differential/field/specification/arcanistproject/DifferentialArcanistProjectFieldSpecification.php new file mode 100644 index 0000000000..d8098b3839 --- /dev/null +++ b/src/applications/differential/field/specification/arcanistproject/DifferentialArcanistProjectFieldSpecification.php @@ -0,0 +1,54 @@ +getArcanistProjectPHID(); + if (!$arcanist_phid) { + return array(); + } + + return array($arcanist_phid); + } + + public function renderLabelForRevisionView() { + return 'Arcanist Project:'; + } + + public function renderValueForRevisionView() { + $arcanist_phid = $this->getArcanistProjectPHID(); + if (!$arcanist_phid) { + return null; + } + + $handle = $this->getHandle($arcanist_phid); + return phutil_escape_html($handle->getName()); + } + + private function getArcanistProjectPHID() { + $diff = $this->getDiff(); + return $diff->getArcanistProjectPHID(); + } + +} diff --git a/src/applications/differential/field/specification/arcanistproject/__init__.php b/src/applications/differential/field/specification/arcanistproject/__init__.php new file mode 100644 index 0000000000..47f73216c4 --- /dev/null +++ b/src/applications/differential/field/specification/arcanistproject/__init__.php @@ -0,0 +1,14 @@ +getAuthorPHID()); + } + + public function renderLabelForRevisionView() { + return 'Author:'; + } + + public function renderValueForRevisionView() { + $author_phid = $this->getAuthorPHID(); + $handle = $this->getHandle($author_phid); + + return $handle->renderLink(); + } + + private function getAuthorPHID() { + $revision = $this->getRevision(); + return $revision->getAuthorPHID(); + } + +} diff --git a/src/applications/differential/field/specification/author/__init__.php b/src/applications/differential/field/specification/author/__init__.php new file mode 100644 index 0000000000..fbef1bd2b7 --- /dev/null +++ b/src/applications/differential/field/specification/author/__init__.php @@ -0,0 +1,12 @@ +getRequiredHandlePHIDs(); + } + + /** + * Specify which @{class:PhabricatorObjectHandles} need to be loaded for your + * field to render correctly on the edit interface. + * + * This is a more specific version of @{method:getRequiredHandlePHIDs} which + * can be overridden to improve field performance by loading only data you + * need. + * + * @return list List of PHIDs to load handles for. + * @task handles + */ + public function getRequiredHandlePHIDsForEdit() { + return $this->getRequiredHandlePHIDs(); + } + + /* -( Contextual Data )---------------------------------------------------- */ + /** * @task context */ @@ -247,6 +304,14 @@ abstract class DifferentialFieldSpecification { return $this; } + /** + * @task context + */ + final public function setHandles(array $handles) { + $this->handles = $handles; + return $this; + } + /** * @task context */ @@ -267,4 +332,23 @@ abstract class DifferentialFieldSpecification { return $this->diff; } + /** + * Get the handle for an object PHID. You must overload + * @{method:getRequiredHandlePHIDs} (or a more specific version thereof) + * and include the PHID you want in the list for it to be available here. + * + * @return PhabricatorObjectHandle Handle to the object. + * @task context + */ + final protected function getHandle($phid) { + if (empty($this->handles[$phid])) { + $class = get_class($this); + throw new Exception( + "A differential field (of class '{$class}') is attempting to retrieve ". + "a handle ('{$phid}') which it did not request. Return all handle ". + "PHIDs you need from getRequiredHandlePHIDs()."); + } + return $this->handles[$phid]; + } + } diff --git a/src/applications/differential/field/specification/commits/DifferentialCommitsFieldSpecification.php b/src/applications/differential/field/specification/commits/DifferentialCommitsFieldSpecification.php new file mode 100644 index 0000000000..245bf6049c --- /dev/null +++ b/src/applications/differential/field/specification/commits/DifferentialCommitsFieldSpecification.php @@ -0,0 +1,53 @@ +getCommitPHIDs(); + } + + public function renderLabelForRevisionView() { + return 'Commits:'; + } + + public function renderValueForRevisionView() { + $commit_phids = $this->getCommitPHIDs(); + if (!$commit_phids) { + return null; + } + + $links = array(); + foreach ($commit_phids as $commit_phid) { + $links[] = $this->getHandle($commit_phid)->renderLink(); + } + + return implode('
', $links); + } + + private function getCommitPHIDs() { + $revision = $this->getRevision(); + return $revision->getCommitPHIDs(); + } + +} diff --git a/src/applications/differential/field/specification/commits/__init__.php b/src/applications/differential/field/specification/commits/__init__.php new file mode 100644 index 0000000000..ff5f56210b --- /dev/null +++ b/src/applications/differential/field/specification/commits/__init__.php @@ -0,0 +1,12 @@ +getDependentRevisionPHIDs(); + } + + public function renderLabelForRevisionView() { + return 'Depends On:'; + } + + public function renderValueForRevisionView() { + $revision_phids = $this->getDependentRevisionPHIDs(); + if (!$revision_phids) { + return null; + } + + $links = array(); + foreach ($revision_phids as $revision_phids) { + $links[] = $this->getHandle($revision_phids)->renderLink(); + } + + return implode('
', $links); + } + + private function getDependentRevisionPHIDs() { + $revision = $this->getRevision(); + return $revision->getAttachedPHIDs( + PhabricatorPHIDConstants::PHID_TYPE_DREV); + } + +} diff --git a/src/applications/differential/field/specification/dependencies/__init__.php b/src/applications/differential/field/specification/dependencies/__init__.php new file mode 100644 index 0000000000..6a6a204086 --- /dev/null +++ b/src/applications/differential/field/specification/dependencies/__init__.php @@ -0,0 +1,13 @@ +getManiphestTaskPHIDs(); + } + + public function renderLabelForRevisionView() { + return 'Maniphest Tasks:'; + } + + public function renderValueForRevisionView() { + $task_phids = $this->getManiphestTaskPHIDs(); + if (!$task_phids) { + return null; + } + + $links = array(); + foreach ($task_phids as $task_phid) { + $links[] = $this->getHandle($task_phid)->renderLink(); + } + + return implode('
', $links); + } + + private function getManiphestTaskPHIDs() { + $revision = $this->getRevision(); + return $revision->getAttachedPHIDs( + PhabricatorPHIDConstants::PHID_TYPE_TASK); + } + +} diff --git a/src/applications/differential/field/specification/maniphesttasks/__init__.php b/src/applications/differential/field/specification/maniphesttasks/__init__.php new file mode 100644 index 0000000000..54c0ba2d66 --- /dev/null +++ b/src/applications/differential/field/specification/maniphesttasks/__init__.php @@ -0,0 +1,14 @@ +