Permalink
Browse files

Use CustomField, not AuxiliaryField, to power RevisionView

Summary: Ref T2222. This will probably have some rough edges for a bit (e.g., weird cases I didn't remember or think of), but there's no change to the underlying data and we can easily revert if things get too messy.

Test Plan: Looked at a variety of revisions and saw sensible output.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8361
  • Loading branch information...
1 parent 0b4a6b8 commit f6a13fd1c7b1567d672d41bba16e1f1dbf6f1ac1 @epriestley epriestley committed Feb 27, 2014
@@ -787,8 +787,6 @@
// -- Differential ---------------------------------------------------------- //
- 'differential.revision-custom-detail-renderer' => null,
-
// List of file regexps where whitespace is meaningful and should not
// use 'ignore-all' by default
'differential.whitespace-matters' => array(
@@ -180,6 +180,8 @@ public static function getAncientConfig() {
'auth.sessions.web' => $session_reason,
'tokenizer.ondemand' => pht(
'Phabricator now manages typeahead strategies automatically.'),
+ 'differential.revision-custom-detail-renderer' => pht(
+ 'Obsolete; use standard rendering events instead.'),
);
return $ancient_config;
@@ -14,12 +14,6 @@ public function getDescription() {
public function getOptions() {
return array(
$this->newOption(
- 'differential.revision-custom-detail-renderer',
- 'class',
- null)
- ->setBaseClass('DifferentialRevisionDetailRenderer')
- ->setDescription(pht("Custom revision detail renderer.")),
- $this->newOption(
'differential.whitespace-matters',
'list<regex>',
array(
@@ -1,46 +0,0 @@
-<?php
-
-abstract class DifferentialRevisionDetailRenderer {
- private $user;
- private $diff;
- private $vsDiff;
-
- final public function setUser(PhabricatorUser $user) {
- $this->user = $user;
- return $this;
- }
-
- final protected function getUser() {
- return $this->user;
- }
-
- final public function setDiff(DifferentialDiff $diff) {
- $this->diff = $diff;
- return $this;
- }
-
- final protected function getDiff() {
- return $this->diff;
- }
-
- final public function setVSDiff(DifferentialDiff $diff) {
- $this->vsDiff = $diff;
- return $this;
- }
-
- final protected function getVSDiff() {
- return $this->vsDiff;
- }
-
- /**
- * This function must return an array of action links that will be
- * added to the end of action links on the differential revision
- * page. Each element in the array must be an array which must
- * contain 'name' and 'href' fields. 'name' will be the name of the
- * link and 'href' will be the address where the link points
- * to. 'class' is optional and can be used for specifying a CSS
- * class.
- */
- abstract public function generateActionLinks(DifferentialRevision $revision,
- DifferentialDiff $diff);
-}
@@ -41,6 +41,8 @@ public function processRequest() {
"This revision has no diffs. Something has gone quite wrong.");
}
+ $revision->attachActiveDiff(last($diffs));
+
$diff_vs = $request->getInt('vs');
$target_id = $request->getInt('id');
@@ -82,8 +84,6 @@ public function processRequest() {
$target_manual->getID());
$props = mpull($props, 'getData', 'getName');
- $aux_fields = $this->loadAuxiliaryFields($revision);
-
$comments = $revision->loadComments();
$all_changesets = $changesets;
@@ -113,25 +113,8 @@ public function processRequest() {
}
}
- $aux_phids = array();
- foreach ($aux_fields as $key => $aux_field) {
- $aux_field->setDiff($target);
- $aux_field->setManualDiff($target_manual);
- $aux_field->setDiffProperties($props);
- $aux_phids[$key] = $aux_field->getRequiredHandlePHIDsForRevisionView();
- }
- $object_phids = array_merge($object_phids, array_mergev($aux_phids));
- $object_phids = array_unique($object_phids);
-
$handles = $this->loadViewerHandles($object_phids);
- 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;
@@ -184,32 +167,22 @@ public function processRequest() {
$visible_changesets = $changesets;
}
+ $field_list = PhabricatorCustomField::getObjectFields(
+ $revision,
+ PhabricatorCustomField::ROLE_VIEW);
+
+ $field_list->setViewer($user);
+ $field_list->readFieldsFromStorage($revision);
+
$revision_detail = id(new DifferentialRevisionDetailView())
->setUser($user)
->setRevision($revision)
->setDiff(end($diffs))
- ->setAuxiliaryFields($aux_fields)
+ ->setCustomFields($field_list)
->setURI($request->getRequestURI());
$actions = $this->getRevisionActions($revision);
- $custom_renderer_class = PhabricatorEnv::getEnvConfig(
- 'differential.revision-custom-detail-renderer');
- if ($custom_renderer_class) {
-
- // TODO: build a better version of the action links and deprecate the
- // whole DifferentialRevisionDetailRenderer class.
- $custom_renderer = newv($custom_renderer_class, array());
- $custom_renderer->setUser($user);
- $custom_renderer->setDiff($target);
- if ($diff_vs) {
- $custom_renderer->setVSDiff($diffs[$diff_vs]);
- }
- $actions = array_merge(
- $actions,
- $custom_renderer->generateActionLinks($revision, $target_manual));
- }
-
$whitespace = $request->getStr(
'whitespace',
DifferentialChangesetParser::WHITESPACE_IGNORE_ALL);
@@ -340,7 +313,9 @@ public function processRequest() {
$comment_form = new DifferentialAddCommentView();
$comment_form->setRevision($revision);
- $comment_form->setAuxFields($aux_fields);
+
+ // TODO: Restore the ability for fields to add accept warnings.
+
$comment_form->setActions($this->getRevisionCommentActions($revision));
$action_uri = '/differential/comment/save/';
@@ -450,46 +425,42 @@ private function getRevisionActions(DifferentialRevision $revision) {
$revision,
PhabricatorPolicyCapability::CAN_EDIT);
- $links = array();
+ $actions = array();
- $links[] = array(
- 'icon' => 'edit',
- 'href' => "/differential/revision/edit/{$revision_id}/",
- 'name' => pht('Edit Revision'),
- 'disabled' => !$can_edit,
- 'sigil' => $can_edit ? null : 'workflow',
- );
+ $actions[] = id(new PhabricatorActionView())
+ ->setIcon('edit')
+ ->setHref("/differential/revision/edit/{$revision_id}/")
+ ->setName(pht('Edit Revision'))
+ ->setDisabled(!$can_edit)
+ ->setWorkflow(!$can_edit);
$this->requireResource('phabricator-object-selector-css');
$this->requireResource('javelin-behavior-phabricator-object-selector');
- $links[] = array(
- 'icon' => 'link',
- 'name' => pht('Edit Dependencies'),
- 'href' => "/search/attach/{$revision_phid}/DREV/dependencies/",
- 'sigil' => 'workflow',
- 'disabled' => !$can_edit,
- );
+ $actions[] = id(new PhabricatorActionView())
+ ->setIcon('link')
+ ->setName(pht('Edit Dependencies'))
+ ->setHref("/search/attach/{$revision_phid}/DREV/dependencies/")
+ ->setWorkflow(true)
+ ->setDisabled(!$can_edit);
$maniphest = 'PhabricatorApplicationManiphest';
if (PhabricatorApplication::isClassInstalled($maniphest)) {
- $links[] = array(
- 'icon' => 'attach',
- 'name' => pht('Edit Maniphest Tasks'),
- 'href' => "/search/attach/{$revision_phid}/TASK/",
- 'sigil' => 'workflow',
- 'disabled' => !$can_edit,
- );
+ $actions[] = id(new PhabricatorActionView())
+ ->setIcon('attach')
+ ->setName(pht('Edit Maniphest Tasks'))
+ ->setHref("/search/attach/{$revision_phid}/TASK/")
+ ->setWorkflow(true)
+ ->setDisabled(!$can_edit);
}
$request_uri = $this->getRequest()->getRequestURI();
- $links[] = array(
- 'icon' => 'download',
- 'name' => pht('Download Raw Diff'),
- 'href' => $request_uri->alter('download', 'true')
- );
+ $actions[] = id(new PhabricatorActionView())
+ ->setIcon('download')
+ ->setName(pht('Download Raw Diff'))
+ ->setHref($request_uri->alter('download', 'true'));
- return $links;
+ return $actions;
}
private function getRevisionCommentActions(DifferentialRevision $revision) {
@@ -689,25 +660,6 @@ private function loadChangesetsAndVsMap(
return array($changesets, $vs_map, $vs_changesets, $refs);
}
- private function loadAuxiliaryFields(DifferentialRevision $revision) {
-
- $aux_fields = DifferentialFieldSelector::newSelector()
- ->getFieldSpecifications();
- foreach ($aux_fields as $key => $aux_field) {
- if (!$aux_field->shouldAppearOnRevisionView()) {
- unset($aux_fields[$key]);
- } else {
- $aux_field->setUser($this->getRequest()->getUser());
- }
- }
-
- $aux_fields = DifferentialAuxiliaryField::loadFromStorage(
- $revision,
- $aux_fields);
-
- return $aux_fields;
- }
-
private function buildSymbolIndexes(
PhabricatorRepositoryArcanistProject $arc_project,
array $visible_changesets) {
@@ -15,6 +15,10 @@ public function getFieldDescription() {
return pht('Stores a reference to what this fixes.');
}
+ public function shouldDisableByDefault() {
+ return true;
+ }
+
public function shouldAppearInPropertyView() {
return true;
}
@@ -15,6 +15,10 @@ public function getFieldDescription() {
return pht('Shows the local host where the diff came from.');
}
+ public function shouldDisableByDefault() {
+ return true;
+ }
+
public function shouldAppearInPropertyView() {
return true;
}
@@ -15,6 +15,10 @@ public function getFieldDescription() {
return pht('Shows the local path where the diff came from.');
}
+ public function shouldDisableByDefault() {
+ return true;
+ }
+
public function shouldAppearInPropertyView() {
return true;
}
@@ -15,6 +15,10 @@ public function getFieldDescription() {
return pht('Instructions for reverting/undoing this change.');
}
+ public function shouldDisableByDefault() {
+ return true;
+ }
+
public function shouldAppearInPropertyView() {
return true;
}
@@ -469,8 +469,7 @@ public function isAutomaticallySubscribed($phid) {
}
public function shouldShowSubscribersProperty() {
- // TODO: Differential does its own thing for now.
- return false;
+ return true;
}
public function shouldAllowSubscription($phid) {
@@ -483,19 +482,42 @@ public function shouldAllowSubscription($phid) {
public function getCustomFieldSpecificationForRole($role) {
$fields = array(
+ new DifferentialAuthorField(),
+
new DifferentialTitleField(),
new DifferentialSummaryField(),
new DifferentialTestPlanField(),
new DifferentialReviewersField(),
+ new DifferentialProjectReviewersField(),
new DifferentialSubscribersField(),
new DifferentialRepositoryField(),
new DifferentialViewPolicyField(),
new DifferentialEditPolicyField(),
+
+ new DifferentialDependsOnField(),
+ new DifferentialDependenciesField(),
+ new DifferentialManiphestTasksField(),
+ new DifferentialCommitsField(),
+
+ new DifferentialJIRAIssuesField(),
+ new DifferentialAsanaRepresentationField(),
+
+ new DifferentialBlameRevisionField(),
+ new DifferentialPathField(),
+ new DifferentialHostField(),
+ new DifferentialRevertPlanField(),
+
+ new DifferentialApplyPatchField(),
);
- return array_fill_keys(
- mpull($fields, 'getFieldKey'),
- array('disabled' => false));
+ $result = array();
+ foreach ($fields as $field) {
+ $result[$field->getFieldKey()] = array(
+ 'disabled' => $field->shouldDisableByDefault(),
+ );
+ }
+
+ return $result;
}
public function getCustomFieldBaseClass() {
Oops, something went wrong.

0 comments on commit f6a13fd

Please sign in to comment.