1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-24 14:30:56 +01:00

Show all packages with a claim on a file in the table of contents view

Summary:
Ref T9218. See discussion there for rationale; I think this is the right behavior to pursue.

The screenshot below is pretty ugly. I think it's a lot worse than most real-world cases will be, since you have to sort of opt-in to having crazy levels of overlapping packages, and it's perfectly normal/reasonable for files owned by one package. Owners is powerful enough to let you specify sub-packages with exclusive ownership.

That said, this may be more typical than I hope. I don't think we can reduce the complexity here much for free, but it would might be reasonable to add some view options (e.g.: group by package?, show only packages I own?, show packages as icons with a tooltip?) if it's an issue.

Test Plan: {F734956}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9218

Differential Revision: https://secure.phabricator.com/D13940
This commit is contained in:
epriestley 2015-08-20 04:46:17 -07:00
parent c87d463093
commit 9790f93a83
5 changed files with 29 additions and 44 deletions

View file

@ -80,12 +80,10 @@ abstract class DifferentialController extends PhabricatorController {
->setCoverageID($coverage_id);
if ($have_owners) {
$package = $control_query->getControllingPackageForPath(
$packages = $control_query->getControllingPackagesForPath(
$repository_phid,
$changeset->getFilename());
if ($package) {
$item->setPackage($package);
}
$item->setPackages($packages);
}
$toc_view->addItem($item);

View file

@ -1123,12 +1123,10 @@ final class DiffusionCommitController extends DiffusionController {
));
if ($have_owners) {
$package = $control_query->getControllingPackageForPath(
$packages = $control_query->getControllingPackagesForPath(
$repository_phid,
$changeset->getFilename());
if ($package) {
$item->setPackage($package);
}
$item->setPackages($packages);
}
$toc_view->addItem($item);

View file

@ -342,28 +342,14 @@ final class PhabricatorOwnersPackageQuery
/* -( Path Control )------------------------------------------------------- */
/**
* Get the package which controls a path, if one exists.
*
* @return PhabricatorOwnersPackage|null Package, if one exists.
*/
public function getControllingPackageForPath($repository_phid, $path) {
$packages = $this->getControllingPackagesForPath($repository_phid, $path);
if (!$packages) {
return null;
}
return head($packages);
}
/**
* Get a list of all packages which control a path or its parent directories,
* ordered from weakest to strongest.
*
* The first package has the most specific claim on the path; the last
* package has the most general claim.
* package has the most general claim. Multiple packages may have claims of
* equal strength, so this ordering is primarily one of usability and
* convenience.
*
* @return list<PhabricatorOwnersPackage> List of controlling packages.
*/

View file

@ -8,7 +8,7 @@ final class PHUIDiffTableOfContentsItemView extends AphrontView {
private $coverage;
private $coverageID;
private $context;
private $package;
private $packages;
public function setChangeset(DifferentialChangeset $changeset) {
$this->changeset = $changeset;
@ -64,13 +64,14 @@ final class PHUIDiffTableOfContentsItemView extends AphrontView {
return $this->context;
}
public function setPackage(PhabricatorOwnersPackage $package) {
$this->package = $package;
public function setPackages(array $packages) {
assert_instances_of($packages, 'PhabricatorOwnersPackage');
$this->packages = mpull($packages, null, 'getPHID');
return $this;
}
public function getPackage() {
return $this->package;
public function getPackages() {
return $this->packages;
}
public function render() {
@ -97,7 +98,7 @@ final class PHUIDiffTableOfContentsItemView extends AphrontView {
$cells[] = $this->renderCoverage();
$cells[] = $this->renderModifiedCoverage();
$cells[] = $this->renderPackage();
$cells[] = $this->renderPackages();
return $cells;
}
@ -284,14 +285,16 @@ final class PHUIDiffTableOfContentsItemView extends AphrontView {
$meta);
}
private function renderPackage() {
$package = $this->getPackage();
if (!$package) {
private function renderPackages() {
$packages = $this->getPackages();
if (!$packages) {
return null;
}
return $this->getUser()->renderHandle($package->getPHID());
$viewer = $this->getUser();
$package_phids = mpull($packages, 'getPHID');
return $viewer->renderHandleList($package_phids);
}
private function renderRename($self, $other, $arrow) {

View file

@ -43,9 +43,9 @@ final class PHUIDiffTableOfContentsListView extends AphrontView {
$have_authority = false;
$package = $item->getPackage();
if ($package) {
if (isset($authority[$package->getPHID()])) {
$packages = $item->getPackages();
if ($packages) {
if (array_intersect_key($packages, $authority)) {
$have_authority = true;
}
}
@ -61,7 +61,7 @@ final class PHUIDiffTableOfContentsListView extends AphrontView {
// just hide them.
$any_coverage = false;
$any_context = false;
$any_package = false;
$any_packages = false;
foreach ($items as $item) {
if ($item->getContext() !== null) {
$any_context = true;
@ -71,8 +71,8 @@ final class PHUIDiffTableOfContentsListView extends AphrontView {
$any_coverage = true;
}
if ($item->getPackage() !== null) {
$any_package = true;
if ($item->getPackages() !== null) {
$any_packages = true;
}
}
@ -103,7 +103,7 @@ final class PHUIDiffTableOfContentsListView extends AphrontView {
pht('Path'),
pht('Coverage (All)'),
pht('Coverage (Touched)'),
pht('Package'),
pht('Packages'),
))
->setColumnClasses(
array(
@ -125,7 +125,7 @@ final class PHUIDiffTableOfContentsListView extends AphrontView {
true,
$any_coverage,
$any_coverage,
$any_package,
$any_packages,
))
->setDeviceVisibility(
array(