From c5e16f9bd9960310001976b77d0a183778a34724 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 15 Feb 2019 06:42:53 -0800 Subject: [PATCH 01/30] Give HarbormasterBuildUnitMessage a real Query class Summary: Ref T13088. Prepares for putting test names in a separate table to release the 255-character limit. Test Plan: Viewed revisions, buildables, builds, test lists, specific tests. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D20179 --- src/__phutil_library_map__.php | 7 +- .../DifferentialChangesetViewController.php | 10 +-- .../controller/DifferentialController.php | 7 +- .../differential/storage/DifferentialDiff.php | 7 +- .../HarbormasterBuildableViewController.php | 7 +- .../HarbormasterUnitMessageListController.php | 7 +- .../HarbormasterUnitMessageViewController.php | 5 +- .../HarbormasterBuildUnitMessageQuery.php | 64 +++++++++++++++++++ .../build/HarbormasterBuildUnitMessage.php | 24 ++++++- 9 files changed, 119 insertions(+), 19 deletions(-) create mode 100644 src/applications/harbormaster/query/HarbormasterBuildUnitMessageQuery.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 3c9a329d99..f82c92d02b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1365,6 +1365,7 @@ phutil_register_library_map(array( 'HarbormasterBuildTransactionEditor' => 'applications/harbormaster/editor/HarbormasterBuildTransactionEditor.php', 'HarbormasterBuildTransactionQuery' => 'applications/harbormaster/query/HarbormasterBuildTransactionQuery.php', 'HarbormasterBuildUnitMessage' => 'applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php', + 'HarbormasterBuildUnitMessageQuery' => 'applications/harbormaster/query/HarbormasterBuildUnitMessageQuery.php', 'HarbormasterBuildViewController' => 'applications/harbormaster/controller/HarbormasterBuildViewController.php', 'HarbormasterBuildWorker' => 'applications/harbormaster/worker/HarbormasterBuildWorker.php', 'HarbormasterBuildable' => 'applications/harbormaster/storage/HarbormasterBuildable.php', @@ -6983,7 +6984,11 @@ phutil_register_library_map(array( 'HarbormasterBuildTransaction' => 'PhabricatorApplicationTransaction', 'HarbormasterBuildTransactionEditor' => 'PhabricatorApplicationTransactionEditor', 'HarbormasterBuildTransactionQuery' => 'PhabricatorApplicationTransactionQuery', - 'HarbormasterBuildUnitMessage' => 'HarbormasterDAO', + 'HarbormasterBuildUnitMessage' => array( + 'HarbormasterDAO', + 'PhabricatorPolicyInterface', + ), + 'HarbormasterBuildUnitMessageQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'HarbormasterBuildViewController' => 'HarbormasterController', 'HarbormasterBuildWorker' => 'HarbormasterWorker', 'HarbormasterBuildable' => array( diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index c41f951394..35793a2108 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -420,15 +420,17 @@ final class DifferentialChangesetViewController extends DifferentialController { } private function loadCoverage(DifferentialChangeset $changeset) { + $viewer = $this->getViewer(); + $target_phids = $changeset->getDiff()->getBuildTargetPHIDs(); if (!$target_phids) { return null; } - $unit = id(new HarbormasterBuildUnitMessage())->loadAllWhere( - 'buildTargetPHID IN (%Ls)', - $target_phids); - + $unit = id(new HarbormasterBuildUnitMessageQuery()) + ->setViewer($viewer) + ->withBuildTargetPHIDs($target_phids) + ->execute(); if (!$unit) { return null; } diff --git a/src/applications/differential/controller/DifferentialController.php b/src/applications/differential/controller/DifferentialController.php index 8fe5b5caca..334d46c3cb 100644 --- a/src/applications/differential/controller/DifferentialController.php +++ b/src/applications/differential/controller/DifferentialController.php @@ -192,9 +192,10 @@ abstract class DifferentialController extends PhabricatorController { $all_target_phids = array_mergev($target_map); if ($all_target_phids) { - $unit_messages = id(new HarbormasterBuildUnitMessage())->loadAllWhere( - 'buildTargetPHID IN (%Ls)', - $all_target_phids); + $unit_messages = id(new HarbormasterBuildUnitMessageQuery()) + ->setViewer($viewer) + ->withBuildTargetPHIDs($all_target_phids) + ->execute(); $unit_messages = mgroup($unit_messages, 'getBuildTargetPHID'); } else { $unit_messages = array(); diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php index e4c33dc766..a39610c54c 100644 --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -387,9 +387,10 @@ final class DifferentialDiff return array(); } - $unit = id(new HarbormasterBuildUnitMessage())->loadAllWhere( - 'buildTargetPHID IN (%Ls)', - $target_phids); + $unit = id(new HarbormasterBuildUnitMessageQuery()) + ->setViewer($viewer) + ->withBuildTargetPHIDs($target_phids) + ->execute(); $map = array(); foreach ($unit as $message) { diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php index 1e79ad2b46..40f6587116 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php @@ -312,9 +312,10 @@ final class HarbormasterBuildableViewController 'buildTargetPHID IN (%Ls)', $target_phids); - $unit_data = id(new HarbormasterBuildUnitMessage())->loadAllWhere( - 'buildTargetPHID IN (%Ls)', - $target_phids); + $unit_data = id(new HarbormasterBuildUnitMessageQuery()) + ->setViewer($viewer) + ->withBuildTargetPHIDs($target_phids) + ->execute(); if ($lint_data) { $lint_table = id(new HarbormasterLintPropertyView()) diff --git a/src/applications/harbormaster/controller/HarbormasterUnitMessageListController.php b/src/applications/harbormaster/controller/HarbormasterUnitMessageListController.php index d548ceac98..a87d17c4fa 100644 --- a/src/applications/harbormaster/controller/HarbormasterUnitMessageListController.php +++ b/src/applications/harbormaster/controller/HarbormasterUnitMessageListController.php @@ -31,9 +31,10 @@ final class HarbormasterUnitMessageListController $unit_data = array(); if ($target_phids) { - $unit_data = id(new HarbormasterBuildUnitMessage())->loadAllWhere( - 'buildTargetPHID IN (%Ls)', - $target_phids); + $unit_data = id(new HarbormasterBuildUnitMessageQuery()) + ->setViewer($viewer) + ->withBuildTargetPHIDs($target_phids) + ->execute(); } else { $unit_data = array(); } diff --git a/src/applications/harbormaster/controller/HarbormasterUnitMessageViewController.php b/src/applications/harbormaster/controller/HarbormasterUnitMessageViewController.php index 5cb33c0c9a..7111db654f 100644 --- a/src/applications/harbormaster/controller/HarbormasterUnitMessageViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterUnitMessageViewController.php @@ -12,7 +12,10 @@ final class HarbormasterUnitMessageViewController $message_id = $request->getURIData('id'); - $message = id(new HarbormasterBuildUnitMessage())->load($message_id); + $message = id(new HarbormasterBuildUnitMessageQuery()) + ->setViewer($viewer) + ->withIDs(array($message_id)) + ->executeOne(); if (!$message) { return new Aphront404Response(); } diff --git a/src/applications/harbormaster/query/HarbormasterBuildUnitMessageQuery.php b/src/applications/harbormaster/query/HarbormasterBuildUnitMessageQuery.php new file mode 100644 index 0000000000..fbd33a5d95 --- /dev/null +++ b/src/applications/harbormaster/query/HarbormasterBuildUnitMessageQuery.php @@ -0,0 +1,64 @@ +ids = $ids; + return $this; + } + + public function withPHIDs(array $phids) { + $this->phids = $phids; + return $this; + } + + public function withBuildTargetPHIDs(array $target_phids) { + $this->targetPHIDs = $target_phids; + return $this; + } + + public function newResultObject() { + return new HarbormasterBuildUnitMessage(); + } + + protected function loadPage() { + return $this->loadStandardPage($this->newResultObject()); + } + + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); + + if ($this->ids !== null) { + $where[] = qsprintf( + $conn, + 'id IN (%Ld)', + $this->ids); + } + + if ($this->phids !== null) { + $where[] = qsprintf( + $conn, + 'phid in (%Ls)', + $this->phids); + } + + if ($this->targetPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'buildTargetPHID in (%Ls)', + $this->targetPHIDs); + } + + return $where; + } + + public function getQueryApplicationClass() { + return 'PhabricatorHarbormasterApplication'; + } + +} diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php b/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php index b2f566c3eb..1f5e5e1440 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php @@ -1,7 +1,8 @@ Date: Fri, 15 Feb 2019 04:39:49 -0800 Subject: [PATCH 02/30] Correct schema irregularities (including weird keys) with worker task tables Summary: Ref T13253. Fixes T6615. See that task for discussion. - Remove three keys which serve no real purpose: `dataID` doesn't do anything for us, and the two `leaseOwner` keys are unused. - Rename `leaseOwner_2` to `key_owner`. - Fix an issue where `dataID` was nullable in the active table and non-nullable in the archive table. In practice, //all// workers have data, so all workers have a `dataID`: if they didn't, we'd already fatal when trying to move tasks to the archive table. Just clean this up for consistency, and remove the ancient codepath which imagined tasks with no data. Test Plan: - Ran `bin/storage upgrade`, inspected tables. - Ran `bin/phd debug taskmaster`, worked through a bunch of tasks with no problems. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13253, T6615 Differential Revision: https://secure.phabricator.com/D20175 --- .../20190215.daemons.01.dropdataid.php | 21 +++++++++++++++++++ .../20190215.daemons.02.nulldataid.sql | 2 ++ .../storage/PhabricatorWorkerActiveTask.php | 18 ++-------------- .../storage/PhabricatorWorkerArchiveTask.php | 3 --- 4 files changed, 25 insertions(+), 19 deletions(-) create mode 100644 resources/sql/autopatches/20190215.daemons.01.dropdataid.php create mode 100644 resources/sql/autopatches/20190215.daemons.02.nulldataid.sql diff --git a/resources/sql/autopatches/20190215.daemons.01.dropdataid.php b/resources/sql/autopatches/20190215.daemons.01.dropdataid.php new file mode 100644 index 0000000000..05cc4adfee --- /dev/null +++ b/resources/sql/autopatches/20190215.daemons.01.dropdataid.php @@ -0,0 +1,21 @@ +establishConnection('w'); + +try { + queryfx( + $conn, + 'ALTER TABLE %R DROP KEY %T', + $table, + 'dataID'); +} catch (AphrontQueryException $ex) { + // Ignore. +} diff --git a/resources/sql/autopatches/20190215.daemons.02.nulldataid.sql b/resources/sql/autopatches/20190215.daemons.02.nulldataid.sql new file mode 100644 index 0000000000..19be602efe --- /dev/null +++ b/resources/sql/autopatches/20190215.daemons.02.nulldataid.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_worker.worker_activetask + CHANGE dataID dataID INT UNSIGNED NOT NULL; diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php index ed1eeaea63..b6bd462df7 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php @@ -14,35 +14,21 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask { self::CONFIG_IDS => self::IDS_COUNTER, self::CONFIG_TIMESTAMPS => false, self::CONFIG_KEY_SCHEMA => array( - 'dataID' => array( - 'columns' => array('dataID'), - 'unique' => true, - ), 'taskClass' => array( 'columns' => array('taskClass'), ), 'leaseExpires' => array( 'columns' => array('leaseExpires'), ), - 'leaseOwner' => array( - 'columns' => array('leaseOwner(16)'), - ), 'key_failuretime' => array( 'columns' => array('failureTime'), ), - 'leaseOwner_2' => array( + 'key_owner' => array( 'columns' => array('leaseOwner', 'priority', 'id'), ), ) + $parent[self::CONFIG_KEY_SCHEMA], ); - $config[self::CONFIG_COLUMN_SCHEMA] = array( - // T6203/NULLABILITY - // This isn't nullable in the archive table, so at a minimum these - // should be the same. - 'dataID' => 'uint32?', - ) + $parent[self::CONFIG_COLUMN_SCHEMA]; - return $config + $parent; } @@ -74,7 +60,7 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask { $this->failureCount = 0; } - if ($is_new && ($this->getData() !== null)) { + if ($is_new) { $data = new PhabricatorWorkerTaskData(); $data->setData($this->getData()); $data->save(); diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php index fe1164e532..0062d07a84 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php @@ -28,9 +28,6 @@ final class PhabricatorWorkerArchiveTask extends PhabricatorWorkerTask { 'dateCreated' => array( 'columns' => array('dateCreated'), ), - 'leaseOwner' => array( - 'columns' => array('leaseOwner', 'priority', 'id'), - ), 'key_modified' => array( 'columns' => array('dateModified'), ), From 0b2d25778d8f71a860c4622d2e92e76f1bd47351 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 13 Feb 2019 09:13:46 -0800 Subject: [PATCH 03/30] Add basic, rough support for changing field behavior based on object subtype Summary: Ref T13248. This will probably need quite a bit of refinement, but we can reasonably allow subtype definitions to adjust custom field behavior. Some places where we use fields are global, and always need to show all the fields. For example, on `/maniphest/`, where you can search across all tasks, you need to be able to search across all fields that are present on any task. Likewise, if you "export" a bunch of tasks into a spreadsheet, we need to have columns for every field. However, when you're clearly in the scope of a particular task (like viewing or editing `T123`), there's no reason we can't hide fields based on the task subtype. To start with, allow subtypes to override "disabled" and "name" for custom fields. Test Plan: - Defined several custom fields and several subtypes. - Disabled/renamed some fields for some subtypes. - Viewed/edited tasks of different subtypes, got desired field behavior. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13248 Differential Revision: https://secure.phabricator.com/D20161 --- .../PhabricatorManiphestConfigOptions.php | 23 +++++ .../editengine/PhabricatorEditEngine.php | 19 +++- .../PhabricatorEditEngineSubtype.php | 34 ++++++++ .../field/PhabricatorCustomField.php | 87 +++++++++++++++++++ .../PhabricatorStandardCustomField.php | 14 +++ 5 files changed, 175 insertions(+), 2 deletions(-) diff --git a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php index 8f2830908b..05f98a2f62 100644 --- a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php +++ b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php @@ -342,6 +342,7 @@ dictionary with these keys: - `icon` //Optional string.// Icon for the subtype. - `children` //Optional map.// Configure options shown to the user when they "Create Subtask". See below. + - `fields` //Optional map.// Configure field behaviors. See below. Each subtype must have a unique key, and you must define a subtype with the key "%s", which is used as a default subtype. @@ -397,6 +398,28 @@ be used when presenting options to the user. If only one option would be presented, the user will be taken directly to the appropriate form instead of being prompted to choose a form. + +The `fields` key can configure the behavior of custom fields on specific +task subtypes. For example: + +``` +{ + ... + "fields": { + "custom.some-field": { + "disabled": true + } + } + ... +} +``` + +Each field supports these options: + + - `disabled` //Optional bool.// Allows you to disable fields on certain + subtypes. + - `name` //Optional string.// Custom name of this field for the subtype. + EOTEXT , $subtype_default_key)); diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 353d7ee385..82af45dca8 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -165,14 +165,29 @@ abstract class PhabricatorEditEngine $extensions = array(); } + // See T13248. Create a template object to provide to extensions. We + // adjust the template to have the intended subtype, so that extensions + // may change behavior based on the form subtype. + + $template_object = clone $object; + if ($this->getIsCreate()) { + if ($this->supportsSubtypes()) { + $config = $this->getEditEngineConfiguration(); + $subtype = $config->getSubtype(); + $template_object->setSubtype($subtype); + } + } + foreach ($extensions as $extension) { $extension->setViewer($viewer); - if (!$extension->supportsObject($this, $object)) { + if (!$extension->supportsObject($this, $template_object)) { continue; } - $extension_fields = $extension->buildCustomEditFields($this, $object); + $extension_fields = $extension->buildCustomEditFields( + $this, + $template_object); // TODO: Validate this in more detail with a more tailored error. assert_instances_of($extension_fields, 'PhabricatorEditField'); diff --git a/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php b/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php index 9e754a3ca8..6e1d1de115 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngineSubtype.php @@ -13,6 +13,7 @@ final class PhabricatorEditEngineSubtype private $color; private $childSubtypes = array(); private $childIdentifiers = array(); + private $fieldConfiguration = array(); public function setKey($key) { $this->key = $key; @@ -94,6 +95,17 @@ final class PhabricatorEditEngineSubtype return $view; } + public function setSubtypeFieldConfiguration( + $subtype_key, + array $configuration) { + $this->fieldConfiguration[$subtype_key] = $configuration; + return $this; + } + + public function getSubtypeFieldConfiguration($subtype_key) { + return idx($this->fieldConfiguration, $subtype_key); + } + public static function validateSubtypeKey($subtype) { if (strlen($subtype) > 64) { throw new Exception( @@ -139,6 +151,7 @@ final class PhabricatorEditEngineSubtype 'color' => 'optional string', 'icon' => 'optional string', 'children' => 'optional map', + 'fields' => 'optional map', )); $key = $value['key']; @@ -183,6 +196,18 @@ final class PhabricatorEditEngineSubtype 'or the other, but not both.')); } } + + $fields = idx($value, 'fields'); + if ($fields) { + foreach ($fields as $field_key => $configuration) { + PhutilTypeSpec::checkMap( + $configuration, + array( + 'disabled' => 'optional bool', + 'name' => 'optional string', + )); + } + } } if (!isset($map[self::SUBTYPE_DEFAULT])) { @@ -233,6 +258,15 @@ final class PhabricatorEditEngineSubtype $subtype->setChildFormIdentifiers($child_forms); } + $field_configurations = idx($entry, 'fields'); + if ($field_configurations) { + foreach ($field_configurations as $field_key => $field_configuration) { + $subtype->setSubtypeFieldConfiguration( + $field_key, + $field_configuration); + } + } + $map[$key] = $subtype; } diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php index d7df3c5b78..c6c70a9614 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -74,9 +74,22 @@ abstract class PhabricatorCustomField extends Phobject { $spec, $object); + $fields = self::adjustCustomFieldsForObjectSubtype( + $object, + $role, + $fields); + foreach ($fields as $key => $field) { + // NOTE: We perform this filtering in "buildFieldList()", but may need + // to filter again after subtype adjustment. + if (!$field->isFieldEnabled()) { + unset($fields[$key]); + continue; + } + if (!$field->shouldEnableForRole($role)) { unset($fields[$key]); + continue; } } @@ -1622,4 +1635,78 @@ abstract class PhabricatorCustomField extends Phobject { return null; } + private static function adjustCustomFieldsForObjectSubtype( + PhabricatorCustomFieldInterface $object, + $role, + array $fields) { + assert_instances_of($fields, __CLASS__); + + // We only apply subtype adjustment for some roles. For example, when + // writing Herald rules or building a Search interface, we always want to + // show all the fields in their default state, so we do not apply any + // adjustments. + $subtype_roles = array( + self::ROLE_EDITENGINE, + self::ROLE_VIEW, + ); + + $subtype_roles = array_fuse($subtype_roles); + if (!isset($subtype_roles[$role])) { + return $fields; + } + + // If the object doesn't support subtypes, we can't possibly make + // any adjustments based on subtype. + if (!($object instanceof PhabricatorEditEngineSubtypeInterface)) { + return $fields; + } + + $subtype_map = $object->newEditEngineSubtypeMap(); + $subtype_key = $object->getEditEngineSubtype(); + $subtype_object = $subtype_map->getSubtype($subtype_key); + + $map = array(); + foreach ($fields as $field) { + $modern_key = $field->getModernFieldKey(); + if (!strlen($modern_key)) { + continue; + } + + $map[$modern_key] = $field; + } + + foreach ($map as $field_key => $field) { + // For now, only support overriding standard custom fields. In the + // future there's no technical or product reason we couldn't let you + // override (some properites of) other fields like "Title", but they + // don't usually support appropriate "setX()" methods today. + if (!($field instanceof PhabricatorStandardCustomField)) { + // For fields that are proxies on top of StandardCustomField, which + // is how most application custom fields work today, we can reconfigure + // the proxied field instead. + $field = $field->getProxy(); + if (!$field || !($field instanceof PhabricatorStandardCustomField)) { + continue; + } + } + + $subtype_config = $subtype_object->getSubtypeFieldConfiguration( + $field_key); + + if (!$subtype_config) { + continue; + } + + if (isset($subtype_config['disabled'])) { + $field->setIsEnabled(!$subtype_config['disabled']); + } + + if (isset($subtype_config['name'])) { + $field->setFieldName($subtype_config['name']); + } + } + + return $fields; + } + } diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php index 5bd6256b73..4c0bce861b 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomField.php @@ -18,6 +18,7 @@ abstract class PhabricatorStandardCustomField private $isCopyable; private $hasStorageValue; private $isBuiltin; + private $isEnabled = true; abstract public function getFieldType(); @@ -175,6 +176,19 @@ abstract class PhabricatorStandardCustomField return $this->rawKey; } + public function setIsEnabled($is_enabled) { + $this->isEnabled = $is_enabled; + return $this; + } + + public function getIsEnabled() { + return $this->isEnabled; + } + + public function isFieldEnabled() { + return $this->getIsEnabled(); + } + /* -( PhabricatorCustomField )--------------------------------------------- */ From 3058cae4b82e104686717f01e16790d38b5360f4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 8 Feb 2019 06:07:24 -0800 Subject: [PATCH 04/30] Allow task statuses to specify that either "comments" or "edits" are "locked" Summary: Ref T13249. See PHI1059. This allows "locked" in `maniphest.statuses` to specify that either "comments" are locked (current behavior, advisory, overridable by users with edit permission, e.g. for calming discussion on a contentious issue or putting a guard rail on things); or "edits" are locked (hard lock, only task owner can edit things). Roughly, "comments" is a soft/advisory lock. "edits" is a hard/strict lock. (I think both types of locks have reasonable use cases, which is why I'm not just making locks stronger across the board.) When "edits" are locked: - The edit policy looks like "no one" to normal callers. - In one special case, we sneak the real value through a back channel using PolicyCodex in the specific narrow case that you're editing the object. Otherwise, the policy selector control incorrectly switches to "No One". - We also have to do a little more validation around applying a mixture of status + owner transactions that could leave the task uneditable. For now, I'm allowing you to reassign a hard-locked task to someone else. If you get this wrong, we can end up in a state where no one can edit the task. If this is an issue, we could respond in various ways: prevent these edits; prevent assigning to disabled users; provide a `bin/task reassign`; uh maybe have a quorum convene? Test Plan: - Defined "Soft Locked" and "Hard Locked" statues. - "Hard Locked" a task, hit errors (trying to unassign myself, trying to hard lock an unassigned task). - Saw nice new policy guidance icon in header. {F6210362} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13249 Differential Revision: https://secure.phabricator.com/D20165 --- resources/celerity/map.php | 6 +- src/__phutil_library_map__.php | 3 + .../PhabricatorManiphestConfigOptions.php | 5 +- .../constants/ManiphestTaskStatus.php | 38 ++++++++- .../editor/ManiphestTransactionEditor.php | 85 +++++++++++++++++++ .../policy/ManiphestTaskPolicyCodex.php | 70 +++++++++++++++ .../maniphest/storage/ManiphestTask.php | 31 +++++-- .../policy/codex/PhabricatorPolicyCodex.php | 4 + .../PhabricatorPolicyEditEngineExtension.php | 22 ++++- webroot/rsrc/css/phui/phui-header-view.css | 10 +++ 10 files changed, 260 insertions(+), 14 deletions(-) create mode 100644 src/applications/maniphest/policy/ManiphestTaskPolicyCodex.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 2d402dbaa3..19f8924555 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '85a1da99', + 'core.pkg.css' => 'f2319e1f', 'core.pkg.js' => '5c737607', 'differential.pkg.css' => 'b8df73d4', 'differential.pkg.js' => '67c9ea4c', @@ -154,7 +154,7 @@ return array( 'rsrc/css/phui/phui-form-view.css' => '01b796c0', 'rsrc/css/phui/phui-form.css' => '159e2d9c', 'rsrc/css/phui/phui-head-thing.css' => 'd7f293df', - 'rsrc/css/phui/phui-header-view.css' => '93cea4ec', + 'rsrc/css/phui/phui-header-view.css' => '285c9139', 'rsrc/css/phui/phui-hovercard.css' => '6ca90fa0', 'rsrc/css/phui/phui-icon-set-selector.css' => '7aa5f3ec', 'rsrc/css/phui/phui-icon.css' => '4cbc684a', @@ -821,7 +821,7 @@ return array( 'phui-form-css' => '159e2d9c', 'phui-form-view-css' => '01b796c0', 'phui-head-thing-view-css' => 'd7f293df', - 'phui-header-view-css' => '93cea4ec', + 'phui-header-view-css' => '285c9139', 'phui-hovercard' => '074f0783', 'phui-hovercard-view-css' => '6ca90fa0', 'phui-icon-set-selector-css' => '7aa5f3ec', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f82c92d02b..37be79ec59 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1743,6 +1743,7 @@ phutil_register_library_map(array( 'ManiphestTaskParentTransaction' => 'applications/maniphest/xaction/ManiphestTaskParentTransaction.php', 'ManiphestTaskPoints' => 'applications/maniphest/constants/ManiphestTaskPoints.php', 'ManiphestTaskPointsTransaction' => 'applications/maniphest/xaction/ManiphestTaskPointsTransaction.php', + 'ManiphestTaskPolicyCodex' => 'applications/maniphest/policy/ManiphestTaskPolicyCodex.php', 'ManiphestTaskPriority' => 'applications/maniphest/constants/ManiphestTaskPriority.php', 'ManiphestTaskPriorityDatasource' => 'applications/maniphest/typeahead/ManiphestTaskPriorityDatasource.php', 'ManiphestTaskPriorityHeraldAction' => 'applications/maniphest/herald/ManiphestTaskPriorityHeraldAction.php', @@ -7384,6 +7385,7 @@ phutil_register_library_map(array( 'PhabricatorEditEngineSubtypeInterface', 'PhabricatorEditEngineLockableInterface', 'PhabricatorEditEngineMFAInterface', + 'PhabricatorPolicyCodexInterface', ), 'ManiphestTaskAssignHeraldAction' => 'HeraldAction', 'ManiphestTaskAssignOtherHeraldAction' => 'ManiphestTaskAssignHeraldAction', @@ -7435,6 +7437,7 @@ phutil_register_library_map(array( 'ManiphestTaskParentTransaction' => 'ManiphestTaskTransactionType', 'ManiphestTaskPoints' => 'Phobject', 'ManiphestTaskPointsTransaction' => 'ManiphestTaskTransactionType', + 'ManiphestTaskPolicyCodex' => 'PhabricatorPolicyCodex', 'ManiphestTaskPriority' => 'ManiphestConstants', 'ManiphestTaskPriorityDatasource' => 'PhabricatorTypeaheadDatasource', 'ManiphestTaskPriorityHeraldAction' => 'HeraldAction', diff --git a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php index 05f98a2f62..f1916cffec 100644 --- a/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php +++ b/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php @@ -210,8 +210,9 @@ The keys you can provide in a specification are: - `claim` //Optional bool.// By default, closing an unassigned task claims it. You can set this to `false` to disable this behavior for a particular status. - - `locked` //Optional bool.// Lock tasks in this status, preventing users - from commenting. + - `locked` //Optional string.// Lock tasks in this status. Specify "comments" + to lock comments (users who can edit the task may override this lock). + Specify "edits" to prevent anyone except the task owner from making edits. - `mfa` //Optional bool.// Require all edits to this task to be signed with multi-factor authentication. diff --git a/src/applications/maniphest/constants/ManiphestTaskStatus.php b/src/applications/maniphest/constants/ManiphestTaskStatus.php index 4d58816e2a..c040befeed 100644 --- a/src/applications/maniphest/constants/ManiphestTaskStatus.php +++ b/src/applications/maniphest/constants/ManiphestTaskStatus.php @@ -16,6 +16,9 @@ final class ManiphestTaskStatus extends ManiphestConstants { const SPECIAL_CLOSED = 'closed'; const SPECIAL_DUPLICATE = 'duplicate'; + const LOCKED_COMMENTS = 'comments'; + const LOCKED_EDITS = 'edits'; + private static function getStatusConfig() { return PhabricatorEnv::getEnvConfig('maniphest.statuses'); } @@ -156,8 +159,13 @@ final class ManiphestTaskStatus extends ManiphestConstants { return !self::isOpenStatus($status); } - public static function isLockedStatus($status) { - return self::getStatusAttribute($status, 'locked', false); + public static function areCommentsLockedInStatus($status) { + return (bool)self::getStatusAttribute($status, 'locked', false); + } + + public static function areEditsLockedInStatus($status) { + $locked = self::getStatusAttribute($status, 'locked'); + return ($locked === self::LOCKED_EDITS); } public static function isMFAStatus($status) { @@ -285,11 +293,35 @@ final class ManiphestTaskStatus extends ManiphestConstants { 'keywords' => 'optional list', 'disabled' => 'optional bool', 'claim' => 'optional bool', - 'locked' => 'optional bool', + 'locked' => 'optional bool|string', 'mfa' => 'optional bool', )); } + // Supported values are "comments" or "edits". For backward compatibility, + // "true" is an alias of "comments". + + foreach ($config as $key => $value) { + $locked = idx($value, 'locked', false); + if ($locked === true || $locked === false) { + continue; + } + + if ($locked === self::LOCKED_EDITS || + $locked === self::LOCKED_COMMENTS) { + continue; + } + + throw new Exception( + pht( + 'Task status ("%s") has unrecognized value for "locked" '. + 'configuration ("%s"). Supported values are: "%s", "%s".', + $key, + $locked, + self::LOCKED_COMMENTS, + self::LOCKED_EDITS)); + } + $special_map = array(); foreach ($config as $key => $value) { $special = idx($value, 'special'); diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 0722e0e27a..1748e5e84e 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -552,6 +552,10 @@ final class ManiphestTransactionEditor $errors = array_merge($errors, $this->moreValidationErrors); } + foreach ($this->getLockValidationErrors($object, $xactions) as $error) { + $errors[] = $error; + } + return $errors; } @@ -1011,5 +1015,86 @@ final class ManiphestTransactionEditor } + private function getLockValidationErrors($object, array $xactions) { + $errors = array(); + + $old_owner = $object->getOwnerPHID(); + $old_status = $object->getStatus(); + + $new_owner = $old_owner; + $new_status = $old_status; + + $owner_xaction = null; + $status_xaction = null; + + foreach ($xactions as $xaction) { + switch ($xaction->getTransactionType()) { + case ManiphestTaskOwnerTransaction::TRANSACTIONTYPE: + $new_owner = $xaction->getNewValue(); + $owner_xaction = $xaction; + break; + case ManiphestTaskStatusTransaction::TRANSACTIONTYPE: + $new_status = $xaction->getNewValue(); + $status_xaction = $xaction; + break; + } + } + + $actor_phid = $this->getActingAsPHID(); + + $was_locked = ManiphestTaskStatus::areEditsLockedInStatus( + $old_status); + $now_locked = ManiphestTaskStatus::areEditsLockedInStatus( + $new_status); + + if (!$now_locked) { + // If we're not ending in an edit-locked status, everything is good. + } else if ($new_owner !== null) { + // If we ending the edit with some valid owner, this is allowed for + // now. We might need to revisit this. + } else { + // The edits end with the task locked and unowned. No one will be able + // to edit it, so we forbid this. We try to be specific about what the + // user did wrong. + + $owner_changed = ($old_owner && !$new_owner); + $status_changed = ($was_locked !== $now_locked); + $message = null; + + if ($status_changed && $owner_changed) { + $message = pht( + 'You can not lock this task and unassign it at the same time '. + 'because no one will be able to edit it anymore. Lock the task '. + 'or remove the owner, but not both.'); + $problem_xaction = $status_xaction; + } else if ($status_changed) { + $message = pht( + 'You can not lock this task because it does not have an owner. '. + 'No one would be able to edit the task. Assign the task to an '. + 'owner before locking it.'); + $problem_xaction = $status_xaction; + } else if ($owner_changed) { + $message = pht( + 'You can not remove the owner of this task because it is locked '. + 'and no one would be able to edit the task. Reassign the task or '. + 'unlock it before removing the owner.'); + $problem_xaction = $owner_xaction; + } else { + // If the task was already broken, we don't have a transaction to + // complain about so just let it through. In theory, this is + // impossible since policy rules should kick in before we get here. + } + + if ($message) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $problem_xaction->getTransactionType(), + pht('Lock Error'), + $message, + $problem_xaction); + } + } + + return $errors; + } } diff --git a/src/applications/maniphest/policy/ManiphestTaskPolicyCodex.php b/src/applications/maniphest/policy/ManiphestTaskPolicyCodex.php new file mode 100644 index 0000000000..638d9bfa60 --- /dev/null +++ b/src/applications/maniphest/policy/ManiphestTaskPolicyCodex.php @@ -0,0 +1,70 @@ +getObject(); + + if ($object->areEditsLocked()) { + return pht('Edits Locked'); + } + + return null; + } + + public function getPolicyIcon() { + $object = $this->getObject(); + + if ($object->areEditsLocked()) { + return 'fa-lock'; + } + + return null; + } + + public function getPolicyTagClasses() { + $object = $this->getObject(); + $classes = array(); + + if ($object->areEditsLocked()) { + $classes[] = 'policy-adjusted-locked'; + } + + return $classes; + } + + public function getPolicySpecialRuleDescriptions() { + $object = $this->getObject(); + + $rules = array(); + + $rules[] = $this->newRule() + ->setCapabilities( + array( + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->setIsActive($object->areEditsLocked()) + ->setDescription( + pht( + 'Tasks with edits locked may only be edited by their owner.')); + + return $rules; + } + + public function getPolicyForEdit($capability) { + + // When a task has its edits locked, the effective edit policy is locked + // to "No One". However, the task owner may still bypass the lock and edit + // the task. When they do, we want the control in the UI to have the + // correct value. Return the real value stored on the object. + + switch ($capability) { + case PhabricatorPolicyCapability::CAN_EDIT: + return $this->getObject()->getEditPolicy(); + } + + return parent::getPolicyForEdit($capability); + } + +} diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index ada537fa4d..0193830b39 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -20,7 +20,8 @@ final class ManiphestTask extends ManiphestDAO DoorkeeperBridgedObjectInterface, PhabricatorEditEngineSubtypeInterface, PhabricatorEditEngineLockableInterface, - PhabricatorEditEngineMFAInterface { + PhabricatorEditEngineMFAInterface, + PhabricatorPolicyCodexInterface { const MARKUP_FIELD_DESCRIPTION = 'markup:desc'; @@ -217,8 +218,16 @@ final class ManiphestTask extends ManiphestDAO return ManiphestTaskStatus::isClosedStatus($this->getStatus()); } - public function isLocked() { - return ManiphestTaskStatus::isLockedStatus($this->getStatus()); + public function areCommentsLocked() { + if ($this->areEditsLocked()) { + return true; + } + + return ManiphestTaskStatus::areCommentsLockedInStatus($this->getStatus()); + } + + public function areEditsLocked() { + return ManiphestTaskStatus::areEditsLockedInStatus($this->getStatus()); } public function setProperty($key, $value) { @@ -371,13 +380,17 @@ final class ManiphestTask extends ManiphestDAO case PhabricatorPolicyCapability::CAN_VIEW: return $this->getViewPolicy(); case PhabricatorPolicyCapability::CAN_INTERACT: - if ($this->isLocked()) { + if ($this->areCommentsLocked()) { return PhabricatorPolicies::POLICY_NOONE; } else { return $this->getViewPolicy(); } case PhabricatorPolicyCapability::CAN_EDIT: - return $this->getEditPolicy(); + if ($this->areEditsLocked()) { + return PhabricatorPolicies::POLICY_NOONE; + } else { + return $this->getEditPolicy(); + } } } @@ -628,4 +641,12 @@ final class ManiphestTask extends ManiphestDAO return new ManiphestTaskMFAEngine(); } + +/* -( PhabricatorPolicyCodexInterface )------------------------------------ */ + + + public function newPolicyCodex() { + return new ManiphestTaskPolicyCodex(); + } + } diff --git a/src/applications/policy/codex/PhabricatorPolicyCodex.php b/src/applications/policy/codex/PhabricatorPolicyCodex.php index 060a798e0a..48e6d2f557 100644 --- a/src/applications/policy/codex/PhabricatorPolicyCodex.php +++ b/src/applications/policy/codex/PhabricatorPolicyCodex.php @@ -29,6 +29,10 @@ abstract class PhabricatorPolicyCodex return array(); } + public function getPolicyForEdit($capability) { + return $this->getObject()->getPolicy($capability); + } + public function getDefaultPolicy() { return PhabricatorPolicyQuery::getDefaultPolicyForObject( $this->viewer, diff --git a/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php b/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php index 568b7bc399..14a4768f21 100644 --- a/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php +++ b/src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php @@ -68,6 +68,14 @@ final class PhabricatorPolicyEditEngineExtension ), ); + if ($object instanceof PhabricatorPolicyCodexInterface) { + $codex = PhabricatorPolicyCodex::newFromObject( + $object, + $viewer); + } else { + $codex = null; + } + $fields = array(); foreach ($map as $type => $spec) { if (empty($types[$type])) { @@ -82,6 +90,18 @@ final class PhabricatorPolicyEditEngineExtension $conduit_description = $spec['description.conduit']; $edit = $spec['edit']; + // Objects may present a policy value to the edit workflow that is + // different from their nominal policy value: for example, when tasks + // are locked, they appear as "Editable By: No One" to other applications + // but we still want to edit the actual policy stored in the database + // when we show the user a form with a policy control in it. + + if ($codex) { + $policy_value = $codex->getPolicyForEdit($capability); + } else { + $policy_value = $object->getPolicy($capability); + } + $policy_field = id(new PhabricatorPolicyEditField()) ->setKey($key) ->setLabel($label) @@ -94,7 +114,7 @@ final class PhabricatorPolicyEditEngineExtension ->setDescription($description) ->setConduitDescription($conduit_description) ->setConduitTypeDescription(pht('New policy PHID or constant.')) - ->setValue($object->getPolicy($capability)); + ->setValue($policy_value); $fields[] = $policy_field; if ($object instanceof PhabricatorSpacesInterface) { diff --git a/webroot/rsrc/css/phui/phui-header-view.css b/webroot/rsrc/css/phui/phui-header-view.css index 6cafb3e330..6a096af76d 100644 --- a/webroot/rsrc/css/phui/phui-header-view.css +++ b/webroot/rsrc/css/phui/phui-header-view.css @@ -249,6 +249,16 @@ body .phui-header-shell.phui-bleed-header color: {$sh-indigotext}; } +.policy-header-callout.policy-adjusted-locked { + background: {$sh-pinkbackground}; +} + +.policy-header-callout.policy-adjusted-locked .policy-link, +.policy-header-callout.policy-adjusted-locked .phui-icon-view { + color: {$sh-pinktext}; +} + + .policy-header-callout .policy-space-container { font-weight: bold; color: {$sh-redtext}; From dbcf41dbea007592cf12bb35fb3c02c9cdf05343 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 16 Feb 2019 07:26:19 -0800 Subject: [PATCH 05/30] Fix a couple more "URI->alter()" callsites in paging code Summary: `grep` had a hard time finding these. Test Plan: Will just hotfix this since I'm still reasonably in the deploy window, this currently fatals: Reviewers: amckinley Differential Revision: https://secure.phabricator.com/D20186 --- src/view/phui/PHUIPagerView.php | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/view/phui/PHUIPagerView.php b/src/view/phui/PHUIPagerView.php index b78efcda96..2bb3a8276e 100644 --- a/src/view/phui/PHUIPagerView.php +++ b/src/view/phui/PHUIPagerView.php @@ -187,9 +187,15 @@ final class PHUIPagerView extends AphrontView { foreach ($pager_index as $key => $index) { if ($index !== null) { $display_index = $this->getDisplayIndex($index); - $pager_links[$key] = (string)$base_uri->alter( - $parameter, - $display_index); + + $uri = id(clone $base_uri); + if ($display_index === null) { + $uri->removeQueryParam($parameter); + } else { + $uri->replaceQueryParam($parameter, $display_index); + } + + $pager_links[$key] = phutil_string_cast($uri); } } Javelin::initBehavior('phabricator-keyboard-pager', $pager_links); @@ -200,10 +206,17 @@ final class PHUIPagerView extends AphrontView { foreach ($links as $link) { list($index, $label, $class) = $link; $display_index = $this->getDisplayIndex($index); - $link = $base_uri->alter($parameter, $display_index); + + $uri = id(clone $base_uri); + if ($display_index === null) { + $uri->removeQueryParam($parameter); + } else { + $uri->replaceQueryParam($parameter, $display_index); + } + $rendered_links[] = id(new PHUIButtonView()) ->setTag('a') - ->setHref($link) + ->setHref($uri) ->setColor(PHUIButtonView::GREY) ->addClass('mml') ->addClass($class) From adab70240390b100fbd64e95d7522bb02079a9a8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 17 Feb 2019 17:39:34 -0800 Subject: [PATCH 06/30] Fix a PhutilURI issue in Multimeter Fished this out of the `secure` error logs. --- .../multimeter/controller/MultimeterSampleController.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/applications/multimeter/controller/MultimeterSampleController.php b/src/applications/multimeter/controller/MultimeterSampleController.php index 73c9ba530e..190a839f63 100644 --- a/src/applications/multimeter/controller/MultimeterSampleController.php +++ b/src/applications/multimeter/controller/MultimeterSampleController.php @@ -300,9 +300,10 @@ final class MultimeterSampleController extends MultimeterController { $group = implode('.', $group); if (!strlen($group)) { - $group = null; + $uri->removeQueryParam('group'); + } else { + $uri->replaceQueryParam('group', $group); } - $uri->replaceQueryParam('group', $group); if ($wipe) { foreach ($this->getColumnMap() as $key => $column) { From 8cf6c68c9520a8e65856799cfee4b27bf278f693 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 19 Feb 2019 09:01:10 -0800 Subject: [PATCH 07/30] Fix a PhutilURI issue in workboards Ref PHI1082. --- .../PhabricatorProjectBoardViewController.php | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php index ee239035da..f2965892d7 100644 --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -1182,19 +1182,27 @@ final class PhabricatorProjectBoardViewController $project = $this->getProject(); if ($base === null) { - $base = $this->getRequest()->getRequestURI(); + $base = $this->getRequest()->getPath(); } $base = new PhutilURI($base); if ($force || ($this->sortKey != $this->getDefaultSort($project))) { - $base->replaceQueryParam('order', $this->sortKey); + if ($this->sortKey !== null) { + $base->replaceQueryParam('order', $this->sortKey); + } else { + $base->removeQueryParam('order'); + } } else { $base->removeQueryParam('order'); } if ($force || ($this->queryKey != $this->getDefaultFilter($project))) { - $base->replaceQueryParam('filter', $this->queryKey); + if ($this->queryKey !== null) { + $base->replaceQueryParam('filter', $this->queryKey); + } else { + $base->removeQueryParam('filter'); + } } else { $base->removeQueryParam('filter'); } From e44b40ca4d83e04bd05070b93f8a42a15da25cb0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 14 Feb 2019 13:19:20 -0800 Subject: [PATCH 08/30] Make "Subscribe/Unsubscribe" require only "CAN_VIEW", not "CAN_INTERACT" Summary: Ref T13249. See PHI1059. Currently, Subscribe/Unsubscribe require CAN_INTERACT via the web UI and no permissions (i.e., effectively CAN_VIEW) via the API. Weaken the requirements from the web UI so that you do not need "CAN_INTERACT". This is a product change to the effect that it's okay to subscribe/unsubscribe from anything you can see, even hard-locked tasks. This generally seems reasonable. Increase the requirements for the actual transaction, which mostly applies to API changes: - To remove subscribers other than yourself, require CAN_EDIT. - To add subscribers other than yourself, require CAN_EDIT or CAN_INTERACT. You may have CAN_EDIT but not CAN_INTERACT on "soft locked" tasks. It's okay to click "Edit" on these, click "Yes, override lock", then remove subscribers other than yourself. This technically plugs some weird, mostly theoretical holes in the API where "attackers" could sometimes make more subscription changes than they should have been able to. Now that we send you email when you're unsubscribed this could only really be used to be mildly mischievous, but no harm in making the policy enforcement more correct. Test Plan: Against normal, soft-locked, and hard-locked tasks: subscribed, unsubscribed, added and removed subscribers, overrode locks, edited via API. Everything worked like it should and I couldn't find any combination of lock state, policy state, and edit pathway that did anything suspicious. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13249 Differential Revision: https://secure.phabricator.com/D20174 --- ...PhabricatorSubscriptionsEditController.php | 9 ---- ...habricatorSubscriptionsUIEventListener.php | 8 +--- ...habricatorApplicationTransactionEditor.php | 45 +++++++++++++++++-- 3 files changed, 44 insertions(+), 18 deletions(-) diff --git a/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php b/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php index 941c0c5811..747e4e98f8 100644 --- a/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php +++ b/src/applications/subscriptions/controller/PhabricatorSubscriptionsEditController.php @@ -47,15 +47,6 @@ final class PhabricatorSubscriptionsEditController $handle->getURI()); } - if (!PhabricatorPolicyFilter::canInteract($viewer, $object)) { - $lock = PhabricatorEditEngineLock::newForObject($viewer, $object); - - $dialog = $this->newDialog() - ->addCancelButton($handle->getURI()); - - return $lock->willBlockUserInteractionWithDialog($dialog); - } - if ($object instanceof PhabricatorApplicationTransactionInterface) { if ($is_add) { $xaction_value = array( diff --git a/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php b/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php index caf860117e..2077160b7c 100644 --- a/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php +++ b/src/applications/subscriptions/events/PhabricatorSubscriptionsUIEventListener.php @@ -73,24 +73,20 @@ final class PhabricatorSubscriptionsUIEventListener ->setName(pht('Automatically Subscribed')) ->setIcon('fa-check-circle lightgreytext'); } else { - $can_interact = PhabricatorPolicyFilter::canInteract($user, $object); - if ($is_subscribed) { $sub_action = id(new PhabricatorActionView()) ->setWorkflow(true) ->setRenderAsForm(true) ->setHref('/subscriptions/delete/'.$object->getPHID().'/') ->setName(pht('Unsubscribe')) - ->setIcon('fa-minus-circle') - ->setDisabled(!$can_interact); + ->setIcon('fa-minus-circle'); } else { $sub_action = id(new PhabricatorActionView()) ->setWorkflow(true) ->setRenderAsForm(true) ->setHref('/subscriptions/add/'.$object->getPHID().'/') ->setName(pht('Subscribe')) - ->setIcon('fa-plus-circle') - ->setDisabled(!$can_interact); + ->setIcon('fa-plus-circle'); } if (!$user->isLoggedIn()) { diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index bd066e633b..9460dd3030 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1648,9 +1648,48 @@ abstract class PhabricatorApplicationTransactionEditor // don't enforce it here. return null; case PhabricatorTransactions::TYPE_SUBSCRIBERS: - // TODO: Removing subscribers other than yourself should probably - // require CAN_EDIT permission. You can do this via the API but - // generally can not via the web interface. + // Anyone can subscribe to or unsubscribe from anything they can view, + // with no other permissions. + + $old = array_fuse($xaction->getOldValue()); + $new = array_fuse($xaction->getNewValue()); + + // To remove users other than yourself, you must be able to edit the + // object. + $rem = array_diff_key($old, $new); + foreach ($rem as $phid) { + if ($phid !== $this->getActingAsPHID()) { + return PhabricatorPolicyCapability::CAN_EDIT; + } + } + + // To add users other than yourself, you must be able to interact. + // This allows "@mentioning" users to work as long as you can comment + // on objects. + + // If you can edit, we return that policy instead so that you can + // override a soft lock and still make edits. + + // TODO: This is a little bit hacky. We really want to be able to say + // "this requires either interact or edit", but there's currently no + // way to specify this kind of requirement. + + $can_edit = PhabricatorPolicyFilter::hasCapability( + $this->getActor(), + $this->object, + PhabricatorPolicyCapability::CAN_EDIT); + + $add = array_diff_key($new, $old); + foreach ($add as $phid) { + if ($phid !== $this->getActingAsPHID()) { + if ($can_edit) { + return PhabricatorPolicyCapability::CAN_EDIT; + } else { + return PhabricatorPolicyCapability::CAN_INTERACT; + } + } + } + return null; case PhabricatorTransactions::TYPE_TOKEN: // TODO: This technically requires CAN_INTERACT, like comments. From 8d348e2eebbe9dbfaf6e8e5bb0188af619516c71 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 15 Feb 2019 13:50:24 -0800 Subject: [PATCH 09/30] Clean up a couple of %Q issues in "Has Parents" task queries Summary: Stragglers from the great "%Q" migration. Test Plan: Ran a query for tasks with parent tasks. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20183 --- src/applications/maniphest/query/ManiphestTaskQuery.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index fc5097f4d3..9fb4ecb68c 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -618,9 +618,9 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { $joins = array(); if ($this->hasOpenParents !== null) { if ($this->hasOpenParents) { - $join_type = 'JOIN'; + $join_type = qsprintf($conn, 'JOIN'); } else { - $join_type = 'LEFT JOIN'; + $join_type = qsprintf($conn, 'LEFT JOIN'); } $joins[] = qsprintf( From 92abe3c8fb84bc51b2f845e5bd1fe8da4117e1dd Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 14 Feb 2019 09:54:55 -0800 Subject: [PATCH 10/30] Extract scope line selection logic from the diff rendering engine so it can reasonably be iterated on Summary: Ref T13249. Ref T11738. See PHI985. Currently, we have a crude heuristic for guessing what line in a source file provides the best context. We get it wrong in a lot of cases, sometimes selecting very silly lines like "{". Although we can't always pick the same line a human would pick, we //can// pile on heuristics until this is less frequently completely wrong and perhaps eventually get it to work fairly well most of the time. Pull the logic for this into a separate standalone class and make it testable to prepare for adding heuristics. Test Plan: Ran unit tests, browsed various files in the web UI and saw as-good-or-better context selection. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13249, T11738 Differential Revision: https://secure.phabricator.com/D20171 --- src/__phutil_library_map__.php | 4 + .../parser/DifferentialChangesetParser.php | 49 +----- .../render/DifferentialChangesetRenderer.php | 38 ++++- .../DifferentialChangesetTwoUpRenderer.php | 46 ++++-- .../diff/PhabricatorDiffScopeEngine.php | 156 ++++++++++++++++++ .../PhabricatorDiffScopeEngineTestCase.php | 51 ++++++ .../diff/__tests__/data/zebra.c | 5 + 7 files changed, 286 insertions(+), 63 deletions(-) create mode 100644 src/infrastructure/diff/PhabricatorDiffScopeEngine.php create mode 100644 src/infrastructure/diff/__tests__/PhabricatorDiffScopeEngineTestCase.php create mode 100644 src/infrastructure/diff/__tests__/data/zebra.c diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 37be79ec59..94ba258f53 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2971,6 +2971,8 @@ phutil_register_library_map(array( 'PhabricatorDeveloperPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDeveloperPreferencesSettingsPanel.php', 'PhabricatorDiffInlineCommentQuery' => 'infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php', 'PhabricatorDiffPreferencesSettingsPanel' => 'applications/settings/panel/PhabricatorDiffPreferencesSettingsPanel.php', + 'PhabricatorDiffScopeEngine' => 'infrastructure/diff/PhabricatorDiffScopeEngine.php', + 'PhabricatorDiffScopeEngineTestCase' => 'infrastructure/diff/__tests__/PhabricatorDiffScopeEngineTestCase.php', 'PhabricatorDifferenceEngine' => 'infrastructure/diff/PhabricatorDifferenceEngine.php', 'PhabricatorDifferentialApplication' => 'applications/differential/application/PhabricatorDifferentialApplication.php', 'PhabricatorDifferentialAttachCommitWorkflow' => 'applications/differential/management/PhabricatorDifferentialAttachCommitWorkflow.php', @@ -8850,6 +8852,8 @@ phutil_register_library_map(array( 'PhabricatorDeveloperPreferencesSettingsPanel' => 'PhabricatorEditEngineSettingsPanel', 'PhabricatorDiffInlineCommentQuery' => 'PhabricatorApplicationTransactionCommentQuery', 'PhabricatorDiffPreferencesSettingsPanel' => 'PhabricatorEditEngineSettingsPanel', + 'PhabricatorDiffScopeEngine' => 'Phobject', + 'PhabricatorDiffScopeEngineTestCase' => 'PhabricatorTestCase', 'PhabricatorDifferenceEngine' => 'Phobject', 'PhabricatorDifferentialApplication' => 'PhabricatorApplication', 'PhabricatorDifferentialAttachCommitWorkflow' => 'PhabricatorDifferentialManagementWorkflow', diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index e214aa16a4..27d2a2d845 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1173,7 +1173,7 @@ final class DifferentialChangesetParser extends Phobject { } $range_len = min($range_len, $rows - $range_start); - list($gaps, $mask, $depths) = $this->calculateGapsMaskAndDepths( + list($gaps, $mask) = $this->calculateGapsAndMask( $mask_force, $feedback_mask, $range_start, @@ -1181,8 +1181,7 @@ final class DifferentialChangesetParser extends Phobject { $renderer ->setGaps($gaps) - ->setMask($mask) - ->setDepths($depths); + ->setMask($mask); $html = $renderer->renderTextChange( $range_start, @@ -1208,15 +1207,9 @@ final class DifferentialChangesetParser extends Phobject { * "show more"). The $mask returned is a sparsely populated dictionary * of $visible_line_number => true. * - * Depths - compute how indented any given line is. The $depths returned - * is a sparsely populated dictionary of $visible_line_number => $depth. - * - * This function also has the side effect of modifying member variable - * new such that tabs are normalized to spaces for each line of the diff. - * - * @return array($gaps, $mask, $depths) + * @return array($gaps, $mask) */ - private function calculateGapsMaskAndDepths( + private function calculateGapsAndMask( $mask_force, $feedback_mask, $range_start, @@ -1224,7 +1217,6 @@ final class DifferentialChangesetParser extends Phobject { $lines_context = $this->getLinesOfContext(); - // Calculate gaps and mask first $gaps = array(); $gap_start = 0; $in_gap = false; @@ -1253,38 +1245,7 @@ final class DifferentialChangesetParser extends Phobject { $gaps = array_reverse($gaps); $mask = $base_mask; - // Time to calculate depth. - // We need to go backwards to properly indent whitespace in this code: - // - // 0: class C { - // 1: - // 1: function f() { - // 2: - // 2: return; - // 1: - // 1: } - // 0: - // 0: } - // - $depths = array(); - $last_depth = 0; - $range_end = $range_start + $range_len; - if (!isset($this->new[$range_end])) { - $range_end--; - } - for ($ii = $range_end; $ii >= $range_start; $ii--) { - // We need to expand tabs to process mixed indenting and to round - // correctly later. - $line = str_replace("\t", ' ', $this->new[$ii]['text']); - $trimmed = ltrim($line); - if ($trimmed != '') { - // We round down to flatten "/**" and " *". - $last_depth = floor((strlen($line) - strlen($trimmed)) / 2); - } - $depths[$ii] = $last_depth; - } - - return array($gaps, $mask, $depths); + return array($gaps, $mask); } /** diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index 450d160e23..f295695286 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -28,12 +28,12 @@ abstract class DifferentialChangesetRenderer extends Phobject { private $originalNew; private $gaps; private $mask; - private $depths; private $originalCharacterEncoding; private $showEditAndReplyLinks; private $canMarkDone; private $objectOwnerPHID; private $highlightingDisabled; + private $scopeEngine; private $oldFile = false; private $newFile = false; @@ -76,14 +76,6 @@ abstract class DifferentialChangesetRenderer extends Phobject { return $this->isUndershield; } - public function setDepths($depths) { - $this->depths = $depths; - return $this; - } - protected function getDepths() { - return $this->depths; - } - public function setMask($mask) { $this->mask = $mask; return $this; @@ -678,4 +670,32 @@ abstract class DifferentialChangesetRenderer extends Phobject { return $views; } + + final protected function getScopeEngine() { + if (!$this->scopeEngine) { + $line_map = $this->getNewLineTextMap(); + + $scope_engine = id(new PhabricatorDiffScopeEngine()) + ->setLineTextMap($line_map); + + $this->scopeEngine = $scope_engine; + } + + return $this->scopeEngine; + } + + private function getNewLineTextMap() { + $new = $this->getNewLines(); + + $text_map = array(); + foreach ($new as $new_line) { + if (!isset($new_line['line'])) { + continue; + } + $text_map[$new_line['line']] = $new_line['text']; + } + + return $text_map; + } + } diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index 5d476f5136..f40a7f5e0b 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -3,6 +3,8 @@ final class DifferentialChangesetTwoUpRenderer extends DifferentialChangesetHTMLRenderer { + private $newOffsetMap; + public function isOneUpRenderer() { return false; } @@ -66,9 +68,12 @@ final class DifferentialChangesetTwoUpRenderer $new_render = $this->getNewRender(); $original_left = $this->getOriginalOld(); $original_right = $this->getOriginalNew(); - $depths = $this->getDepths(); $mask = $this->getMask(); + $scope_engine = $this->getScopeEngine(); + + $offset_map = null; + for ($ii = $range_start; $ii < $range_start + $range_len; $ii++) { if (empty($mask[$ii])) { // If we aren't going to show this line, we've just entered a gap. @@ -87,16 +92,19 @@ final class DifferentialChangesetTwoUpRenderer $is_last_block = true; } - $context = null; + $context_text = null; $context_line = null; - if (!$is_last_block && $depths[$ii + $len]) { - for ($l = $ii + $len - 1; $l >= $ii; $l--) { - $line = $new_lines[$l]['text']; - if ($depths[$l] < $depths[$ii + $len] && trim($line) != '') { - $context = $new_render[$l]; - $context_line = $new_lines[$l]['line']; - break; + if (!$is_last_block) { + $target_line = $new_lines[$ii + $len]['line']; + $context_line = $scope_engine->getScopeStart($target_line); + if ($context_line !== null) { + // The scope engine returns a line number in the file. We need + // to map that back to a display offset in the diff. + if (!$offset_map) { + $offset_map = $this->getNewLineToOffsetMap(); } + $offset = $offset_map[$context_line]; + $context_text = $new_render[$offset]; } } @@ -126,7 +134,7 @@ final class DifferentialChangesetTwoUpRenderer 'class' => 'show-context', ), // TODO: [HTML] Escaping model here isn't ideal. - phutil_safe_html($context)), + phutil_safe_html($context_text)), )); $html[] = $container; @@ -386,4 +394,22 @@ final class DifferentialChangesetTwoUpRenderer ->addInlineView($view); } + private function getNewLineToOffsetMap() { + if ($this->newOffsetMap === null) { + $new = $this->getNewLines(); + + $map = array(); + foreach ($new as $offset => $new_line) { + if ($new_line['line'] === null) { + continue; + } + $map[$new_line['line']] = $offset; + } + + $this->newOffsetMap = $map; + } + + return $this->newOffsetMap; + } + } diff --git a/src/infrastructure/diff/PhabricatorDiffScopeEngine.php b/src/infrastructure/diff/PhabricatorDiffScopeEngine.php new file mode 100644 index 0000000000..5ea1ec5021 --- /dev/null +++ b/src/infrastructure/diff/PhabricatorDiffScopeEngine.php @@ -0,0 +1,156 @@ + $value) { + if ($key === $expect) { + $expect++; + continue; + } + + throw new Exception( + pht( + 'ScopeEngine text map must be a contiguous map of '. + 'lines, but is not: found key "%s" where key "%s" was expected.', + $key, + $expect)); + } + + $this->lineTextMap = $map; + + return $this; + } + + public function getLineTextMap() { + if ($this->lineTextMap === null) { + throw new PhutilInvalidStateException('setLineTextMap'); + } + return $this->lineTextMap; + } + + public function getScopeStart($line) { + $text_map = $this->getLineTextMap(); + $depth_map = $this->getLineDepthMap(); + $length = count($text_map); + + // Figure out the effective depth of the line we're getting scope for. + // If the line is just whitespace, it may have no depth on its own. In + // this case, we look for the next line. + $line_depth = null; + for ($ii = $line; $ii <= $length; $ii++) { + if ($depth_map[$ii] !== null) { + $line_depth = $depth_map[$ii]; + break; + } + } + + // If we can't find a line depth for the target line, just bail. + if ($line_depth === null) { + return null; + } + + // Limit the maximum number of lines we'll examine. If a user has a + // million-line diff of nonsense, scanning the whole thing is a waste + // of time. + $search_range = 1000; + $search_until = max(0, $ii - $search_range); + + for ($ii = $line - 1; $ii > $search_until; $ii--) { + $line_text = $text_map[$ii]; + + // This line is in missing context: the diff was diffed with partial + // context, and we ran out of context before finding a good scope line. + // Bail out, we don't want to jump across missing context blocks. + if ($line_text === null) { + return null; + } + + $depth = $depth_map[$ii]; + + // This line is all whitespace. This isn't a possible match. + if ($depth === null) { + continue; + } + + // The depth is the same as (or greater than) the depth we started with, + // so this isn't a possible match. + if ($depth >= $line_depth) { + continue; + } + + // Reject lines which begin with "}" or "{". These lines are probably + // never good matches. + if (preg_match('/^\s*[{}]/i', $line_text)) { + continue; + } + + return $ii; + } + + return null; + } + + private function getLineDepthMap() { + if (!$this->lineDepthMap) { + $this->lineDepthMap = $this->newLineDepthMap(); + } + + return $this->lineDepthMap; + } + + private function newLineDepthMap() { + $text_map = $this->getLineTextMap(); + + // TODO: This should be configurable once we handle tab widths better. + $tab_width = 2; + + $depth_map = array(); + foreach ($text_map as $line_number => $line_text) { + if ($line_text === null) { + $depth_map[$line_number] = null; + continue; + } + + $len = strlen($line_text); + + // If the line has no actual text, don't assign it a depth. + if (!$len || !strlen(trim($line_text))) { + $depth_map[$line_number] = null; + continue; + } + + $count = 0; + for ($ii = 0; $ii < $len; $ii++) { + $c = $line_text[$ii]; + if ($c == ' ') { + $count++; + } else if ($c == "\t") { + $count += $tab_width; + } else { + break; + } + } + + // Round down to cheat our way through the " *" parts of docblock + // comments. This is generally a reasonble heuristic because odd tab + // widths are exceptionally rare. + $depth = ($count >> 1); + + $depth_map[$line_number] = $depth; + } + + return $depth_map; + } + +} diff --git a/src/infrastructure/diff/__tests__/PhabricatorDiffScopeEngineTestCase.php b/src/infrastructure/diff/__tests__/PhabricatorDiffScopeEngineTestCase.php new file mode 100644 index 0000000000..50e23ac31c --- /dev/null +++ b/src/infrastructure/diff/__tests__/PhabricatorDiffScopeEngineTestCase.php @@ -0,0 +1,51 @@ +assertScopeStart('zebra.c', 4, 2); + } + + private function assertScopeStart($file, $line, $expect) { + $engine = $this->getScopeTestEngine($file); + + $actual = $engine->getScopeStart($line); + $this->assertEqual( + $expect, + $actual, + pht( + 'Expect scope for line %s to start on line %s (actual: %s) in "%s".', + $line, + $expect, + $actual, + $file)); + } + + private function getScopeTestEngine($file) { + if (!isset($this->engines[$file])) { + $this->engines[$file] = $this->newScopeTestEngine($file); + } + + return $this->engines[$file]; + } + + private function newScopeTestEngine($file) { + $path = dirname(__FILE__).'/data/'.$file; + $data = Filesystem::readFile($path); + + $lines = phutil_split_lines($data); + $map = array(); + foreach ($lines as $key => $line) { + $map[$key + 1] = $line; + } + + $engine = id(new PhabricatorDiffScopeEngine()) + ->setLineTextMap($map); + + return $engine; + } + +} diff --git a/src/infrastructure/diff/__tests__/data/zebra.c b/src/infrastructure/diff/__tests__/data/zebra.c new file mode 100644 index 0000000000..d587b018a9 --- /dev/null +++ b/src/infrastructure/diff/__tests__/data/zebra.c @@ -0,0 +1,5 @@ +void +ZebraTamer::TameAZebra(nsPoint where, const nsRect& zone, nsAtom* material) +{ + zebra.tame = true; +} From aa470d21549c7928127470e2136adf90f0427ec7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 14 Feb 2019 11:37:42 -0800 Subject: [PATCH 11/30] Show user availability dots (red = away, orange = busy) in typeaheads, tokenizer tokens, and autocompletes Summary: Ref T13249. See PHI810. We currently show availability dots in some interfaces (timeline, mentions) but not others (typeheads/tokenizers). They're potentially quite useful in tokenizers, e.g. when assigning tasks to someone or requesting reviews. Show them in more places. (The actual rendering here isn't terribly clean, and it would be great to try to unify all these various behaviors some day.) Test Plan: {F6212044} {F6212045} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13249 Differential Revision: https://secure.phabricator.com/D20173 --- resources/celerity/map.php | 52 +++++++++--------- .../typeahead/PhabricatorPeopleDatasource.php | 11 +++- .../storage/PhabricatorTypeaheadResult.php | 11 ++++ .../view/PhabricatorTypeaheadTokenView.php | 55 ++++++++++++++++--- .../control/AphrontFormTokenizerControl.php | 4 ++ webroot/rsrc/css/phui/phui-tag-view.css | 8 +++ webroot/rsrc/js/core/Prefab.js | 29 +++++++++- webroot/rsrc/js/phuix/PHUIXAutocomplete.js | 11 +++- 8 files changed, 141 insertions(+), 40 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 19f8924555..26aec85659 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,8 +9,8 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => 'f2319e1f', - 'core.pkg.js' => '5c737607', + 'core.pkg.css' => '261ee8cf', + 'core.pkg.js' => '5ace8a1e', 'differential.pkg.css' => 'b8df73d4', 'differential.pkg.js' => '67c9ea4c', 'diffusion.pkg.css' => '42c75c37', @@ -172,7 +172,7 @@ return array( 'rsrc/css/phui/phui-segment-bar-view.css' => '5166b370', 'rsrc/css/phui/phui-spacing.css' => 'b05cadc3', 'rsrc/css/phui/phui-status.css' => 'e5ff8be0', - 'rsrc/css/phui/phui-tag-view.css' => 'a42fe34f', + 'rsrc/css/phui/phui-tag-view.css' => '29409667', 'rsrc/css/phui/phui-timeline-view.css' => '1e348e4b', 'rsrc/css/phui/phui-two-column-view.css' => '01e6991e', 'rsrc/css/phui/workboards/phui-workboard-color.css' => 'e86de308', @@ -441,7 +441,7 @@ return array( 'rsrc/js/core/KeyboardShortcutManager.js' => '37b8a04a', 'rsrc/js/core/MultirowRowManager.js' => '5b54c823', 'rsrc/js/core/Notification.js' => 'a9b91e3f', - 'rsrc/js/core/Prefab.js' => 'bf457520', + 'rsrc/js/core/Prefab.js' => '5793d835', 'rsrc/js/core/ShapedRequest.js' => 'abf88db8', 'rsrc/js/core/TextAreaUtils.js' => 'f340a484', 'rsrc/js/core/Title.js' => '43bc9360', @@ -505,7 +505,7 @@ return array( 'rsrc/js/phui/behavior-phui-timer-control.js' => 'f84bcbf4', 'rsrc/js/phuix/PHUIXActionListView.js' => 'c68f183f', 'rsrc/js/phuix/PHUIXActionView.js' => 'aaa08f3b', - 'rsrc/js/phuix/PHUIXAutocomplete.js' => '58cc4ab8', + 'rsrc/js/phuix/PHUIXAutocomplete.js' => '8f139ef0', 'rsrc/js/phuix/PHUIXButtonView.js' => '55a24e84', 'rsrc/js/phuix/PHUIXDropdownMenu.js' => 'bdce4d78', 'rsrc/js/phuix/PHUIXExample.js' => 'c2c500a7', @@ -771,7 +771,7 @@ return array( 'phabricator-notification-menu-css' => 'e6962e89', 'phabricator-object-selector-css' => 'ee77366f', 'phabricator-phtize' => '2f1db1ed', - 'phabricator-prefab' => 'bf457520', + 'phabricator-prefab' => '5793d835', 'phabricator-remarkup-css' => '9e627d41', 'phabricator-search-results-css' => '9ea70ace', 'phabricator-shaped-request' => 'abf88db8', @@ -847,7 +847,7 @@ return array( 'phui-segment-bar-view-css' => '5166b370', 'phui-spacing-css' => 'b05cadc3', 'phui-status-list-view-css' => 'e5ff8be0', - 'phui-tag-view-css' => 'a42fe34f', + 'phui-tag-view-css' => '29409667', 'phui-theme-css' => '35883b37', 'phui-timeline-view-css' => '1e348e4b', 'phui-two-column-view-css' => '01e6991e', @@ -857,7 +857,7 @@ return array( 'phui-workpanel-view-css' => 'bd546a49', 'phuix-action-list-view' => 'c68f183f', 'phuix-action-view' => 'aaa08f3b', - 'phuix-autocomplete' => '58cc4ab8', + 'phuix-autocomplete' => '8f139ef0', 'phuix-button-view' => '55a24e84', 'phuix-dropdown-menu' => 'bdce4d78', 'phuix-form-control-view' => '38c1f3fb', @@ -1354,6 +1354,18 @@ return array( 'javelin-stratcom', 'javelin-dom', ), + '5793d835' => array( + 'javelin-install', + 'javelin-util', + 'javelin-dom', + 'javelin-typeahead', + 'javelin-tokenizer', + 'javelin-typeahead-preloaded-source', + 'javelin-typeahead-ondemand-source', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-util', + ), '5803b9e7' => array( 'javelin-behavior', 'javelin-util', @@ -1362,12 +1374,6 @@ return array( 'javelin-vector', 'javelin-typeahead-static-source', ), - '58cc4ab8' => array( - 'javelin-install', - 'javelin-dom', - 'phuix-icon-view', - 'phabricator-prefab', - ), '5902260c' => array( 'javelin-util', 'javelin-magical-init', @@ -1608,6 +1614,12 @@ return array( '8e2d9a28' => array( 'phui-theme-css', ), + '8f139ef0' => array( + 'javelin-install', + 'javelin-dom', + 'phuix-icon-view', + 'phabricator-prefab', + ), '8f959ad0' => array( 'javelin-behavior', 'javelin-dom', @@ -1895,18 +1907,6 @@ return array( 'javelin-vector', 'javelin-stratcom', ), - 'bf457520' => array( - 'javelin-install', - 'javelin-util', - 'javelin-dom', - 'javelin-typeahead', - 'javelin-tokenizer', - 'javelin-typeahead-preloaded-source', - 'javelin-typeahead-ondemand-source', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-util', - ), 'c03f2fb4' => array( 'javelin-install', ), diff --git a/src/applications/people/typeahead/PhabricatorPeopleDatasource.php b/src/applications/people/typeahead/PhabricatorPeopleDatasource.php index df146808bb..d4a5ad96c7 100644 --- a/src/applications/people/typeahead/PhabricatorPeopleDatasource.php +++ b/src/applications/people/typeahead/PhabricatorPeopleDatasource.php @@ -19,7 +19,8 @@ final class PhabricatorPeopleDatasource $viewer = $this->getViewer(); $query = id(new PhabricatorPeopleQuery()) - ->setOrderVector(array('username')); + ->setOrderVector(array('username')) + ->needAvailability(true); if ($this->getPhase() == self::PHASE_PREFIX) { $prefix = $this->getPrefixQuery(); @@ -96,6 +97,14 @@ final class PhabricatorPeopleDatasource $result->setDisplayType($display_type); } + $until = $user->getAwayUntil(); + if ($until) { + $availability = $user->getDisplayAvailability(); + $color = PhabricatorCalendarEventInvitee::getAvailabilityColor( + $availability); + $result->setAvailabilityColor($color); + } + $results[] = $result; } diff --git a/src/applications/typeahead/storage/PhabricatorTypeaheadResult.php b/src/applications/typeahead/storage/PhabricatorTypeaheadResult.php index 14cbe726dc..b13cf351b1 100644 --- a/src/applications/typeahead/storage/PhabricatorTypeaheadResult.php +++ b/src/applications/typeahead/storage/PhabricatorTypeaheadResult.php @@ -19,6 +19,7 @@ final class PhabricatorTypeaheadResult extends Phobject { private $autocomplete; private $attributes = array(); private $phase; + private $availabilityColor; public function setIcon($icon) { $this->icon = $icon; @@ -156,6 +157,7 @@ final class PhabricatorTypeaheadResult extends Phobject { $this->unique ? 1 : null, $this->autocomplete, $this->phase, + $this->availabilityColor, ); while (end($data) === null) { array_pop($data); @@ -222,4 +224,13 @@ final class PhabricatorTypeaheadResult extends Phobject { return $this->phase; } + public function setAvailabilityColor($availability_color) { + $this->availabilityColor = $availability_color; + return $this; + } + + public function getAvailabilityColor() { + return $this->availabilityColor; + } + } diff --git a/src/applications/typeahead/view/PhabricatorTypeaheadTokenView.php b/src/applications/typeahead/view/PhabricatorTypeaheadTokenView.php index 56867d8278..e0a5270e84 100644 --- a/src/applications/typeahead/view/PhabricatorTypeaheadTokenView.php +++ b/src/applications/typeahead/view/PhabricatorTypeaheadTokenView.php @@ -14,6 +14,7 @@ final class PhabricatorTypeaheadTokenView private $inputName; private $value; private $tokenType = self::TYPE_OBJECT; + private $availabilityColor; public static function newFromTypeaheadResult( PhabricatorTypeaheadResult $result) { @@ -41,6 +42,21 @@ final class PhabricatorTypeaheadTokenView $token->setColor($handle->getTagColor()); } + $availability = $handle->getAvailability(); + $color = null; + switch ($availability) { + case PhabricatorObjectHandle::AVAILABILITY_PARTIAL: + $color = PHUITagView::COLOR_ORANGE; + break; + case PhabricatorObjectHandle::AVAILABILITY_NONE: + $color = PHUITagView::COLOR_RED; + break; + } + + if ($color !== null) { + $token->setAvailabilityColor($color); + } + return $token; } @@ -106,6 +122,15 @@ final class PhabricatorTypeaheadTokenView return 'a'; } + public function setAvailabilityColor($availability_color) { + $this->availabilityColor = $availability_color; + return $this; + } + + public function getAvailabilityColor() { + return $this->availabilityColor; + } + protected function getTagAttributes() { $classes = array(); $classes[] = 'jx-tokenizer-token'; @@ -139,20 +164,32 @@ final class PhabricatorTypeaheadTokenView $value = $this->getValue(); + $availability = null; + $availability_color = $this->getAvailabilityColor(); + if ($availability_color) { + $availability = phutil_tag( + 'span', + array( + 'class' => 'phui-tag-dot phui-tag-color-'.$availability_color, + )); + } + + $icon_view = null; $icon = $this->getIcon(); if ($icon) { - $value = array( - phutil_tag( - 'span', - array( - 'class' => 'phui-icon-view phui-font-fa '.$icon, - )), - $value, - ); + $icon_view = phutil_tag( + 'span', + array( + 'class' => 'phui-icon-view phui-font-fa '.$icon, + )); } return array( - $value, + array( + $icon_view, + $availability, + $value, + ), phutil_tag( 'input', array( diff --git a/src/view/form/control/AphrontFormTokenizerControl.php b/src/view/form/control/AphrontFormTokenizerControl.php index 3d65c4e525..fe80c86f81 100644 --- a/src/view/form/control/AphrontFormTokenizerControl.php +++ b/src/view/form/control/AphrontFormTokenizerControl.php @@ -108,6 +108,10 @@ final class AphrontFormTokenizerControl extends AphrontFormControl { 'icons' => mpull($tokens, 'getIcon', 'getKey'), 'types' => mpull($tokens, 'getTokenType', 'getKey'), 'colors' => mpull($tokens, 'getColor', 'getKey'), + 'availabilityColors' => mpull( + $tokens, + 'getAvailabilityColor', + 'getKey'), 'limit' => $this->limit, 'username' => $username, 'placeholder' => $placeholder, diff --git a/webroot/rsrc/css/phui/phui-tag-view.css b/webroot/rsrc/css/phui/phui-tag-view.css index 73675a44d6..57529645a7 100644 --- a/webroot/rsrc/css/phui/phui-tag-view.css +++ b/webroot/rsrc/css/phui/phui-tag-view.css @@ -54,6 +54,14 @@ a.phui-tag-view:hover { border: 1px solid transparent; } +.tokenizer-result .phui-tag-dot { + margin-right: 6px; +} + +.jx-tokenizer-token .phui-tag-dot { + margin-left: 2px; +} + .phui-tag-type-state { color: #ffffff; text-shadow: rgba(100, 100, 100, 0.40) 0px -1px 1px; diff --git a/webroot/rsrc/js/core/Prefab.js b/webroot/rsrc/js/core/Prefab.js index 979ad3473b..ff4467881b 100644 --- a/webroot/rsrc/js/core/Prefab.js +++ b/webroot/rsrc/js/core/Prefab.js @@ -125,15 +125,18 @@ JX.install('Prefab', { var icon; var type; var color; + var availability_color; if (result) { icon = result.icon; value = result.displayName; type = result.tokenType; color = result.color; + availability_color = result.availabilityColor; } else { icon = (config.icons || {})[key]; type = (config.types || {})[key]; color = (config.colors || {})[key]; + availability_color = (config.availabilityColors || {})[key]; } if (icon) { @@ -147,7 +150,16 @@ JX.install('Prefab', { JX.DOM.alterClass(container, color, true); } - return [icon, value]; + var dot; + if (availability_color) { + dot = JX.$N( + 'span', + { + className: 'phui-tag-dot phui-tag-color-' + availability_color + }); + } + + return [icon, dot, value]; }); if (config.placeholder) { @@ -275,10 +287,20 @@ JX.install('Prefab', { icon_ui = JX.Prefab._renderIcon(icon); } + var availability_ui; + var availability_color = fields[16]; + if (availability_color) { + availability_ui = JX.$N( + 'span', + { + className: 'phui-tag-dot phui-tag-color-' + availability_color + }); + } + var display = JX.$N( 'div', {className: 'tokenizer-result'}, - [icon_ui, fields[4] || fields[0], closed_ui]); + [icon_ui, availability_ui, fields[4] || fields[0], closed_ui]); if (closed) { JX.DOM.alterClass(display, 'tokenizer-result-closed', true); } @@ -300,7 +322,8 @@ JX.install('Prefab', { tokenType: fields[12], unique: fields[13] || false, autocomplete: fields[14], - sort: JX.TypeaheadNormalizer.normalize(fields[0]) + sort: JX.TypeaheadNormalizer.normalize(fields[0]), + availabilityColor: availability_color }; }, diff --git a/webroot/rsrc/js/phuix/PHUIXAutocomplete.js b/webroot/rsrc/js/phuix/PHUIXAutocomplete.js index f46e7666e2..deb9f9d100 100644 --- a/webroot/rsrc/js/phuix/PHUIXAutocomplete.js +++ b/webroot/rsrc/js/phuix/PHUIXAutocomplete.js @@ -185,7 +185,16 @@ JX.install('PHUIXAutocomplete', { .getNode(); } - var display = JX.$N('span', {}, [icon, map.displayName]); + var dot; + if (map.availabilityColor) { + dot = JX.$N( + 'span', + { + className: 'phui-tag-dot phui-tag-color-' + map.availabilityColor + }); + } + + var display = JX.$N('span', {}, [icon, dot, map.displayName]); JX.DOM.alterClass(display, 'tokenizer-result-closed', !!map.closed); map.display = display; From 312ba307148552501dcc6dd8e7f82d61310a8cf9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 15 Feb 2019 05:24:17 -0800 Subject: [PATCH 12/30] Don't report search indexing errors to the daemon log except from "bin/search index" Summary: Depends on D20177. Fixes T12425. See . Search indexing currently reports failures to load objects to the log. This log is noisy, not concerning, not actionable, and not locally debuggable (it depends on the reporting user's entire state). I think one common, fully legitimate case of this is indexing temporary files: they may fully legitimately be deleted by the time the indexer runs. Instead of sending these errors to the log, eat them. If users don't notice the indexes aren't working: no harm, no foul. If users do notice, we'll run or have them run `bin/search index` as a first diagnostic step anyway, which will now report an actionable/reproducible error. Test Plan: - Faked errors in both cases (initial load, re-load inside the locked section). - Ran indexes in strict/non-strict mode. - Got exception reports from both branches in strict mode. - Got task success without errors in both cases in non-strict mode. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12425 Differential Revision: https://secure.phabricator.com/D20178 --- ...abricatorSearchManagementIndexWorkflow.php | 9 ++- .../search/worker/PhabricatorSearchWorker.php | 55 +++++++++++++++---- 2 files changed, 51 insertions(+), 13 deletions(-) diff --git a/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php index 6b6d25cb6c..99ee3a3123 100644 --- a/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php @@ -116,6 +116,10 @@ final class PhabricatorSearchManagementIndexWorkflow // them a hint that they might want to use "--force". $track_skips = (!$is_background && !$is_force); + // Activate "strict" error reporting if we're running in the foreground + // so we'll report a wider range of conditions as errors. + $is_strict = !$is_background; + $count_updated = 0; $count_skipped = 0; @@ -125,7 +129,10 @@ final class PhabricatorSearchManagementIndexWorkflow $old_versions = $this->loadIndexVersions($phid); } - PhabricatorSearchWorker::queueDocumentForIndexing($phid, $parameters); + PhabricatorSearchWorker::queueDocumentForIndexing( + $phid, + $parameters, + $is_strict); if ($track_skips) { $new_versions = $this->loadIndexVersions($phid); diff --git a/src/applications/search/worker/PhabricatorSearchWorker.php b/src/applications/search/worker/PhabricatorSearchWorker.php index 361d9c9b6f..31c68d45c8 100644 --- a/src/applications/search/worker/PhabricatorSearchWorker.php +++ b/src/applications/search/worker/PhabricatorSearchWorker.php @@ -2,7 +2,11 @@ final class PhabricatorSearchWorker extends PhabricatorWorker { - public static function queueDocumentForIndexing($phid, $parameters = null) { + public static function queueDocumentForIndexing( + $phid, + $parameters = null, + $is_strict = false) { + if ($parameters === null) { $parameters = array(); } @@ -12,6 +16,7 @@ final class PhabricatorSearchWorker extends PhabricatorWorker { array( 'documentPHID' => $phid, 'parameters' => $parameters, + 'strict' => $is_strict, ), array( 'priority' => parent::PRIORITY_INDEX, @@ -23,7 +28,25 @@ final class PhabricatorSearchWorker extends PhabricatorWorker { $data = $this->getTaskData(); $object_phid = idx($data, 'documentPHID'); - $object = $this->loadObjectForIndexing($object_phid); + // See T12425. By the time we run an indexing task, the object it indexes + // may have been deleted. This is unusual, but not concerning, and failing + // to index these objects is correct. + + // To avoid showing these non-actionable errors to users, don't report + // indexing exceptions unless we're in "strict" mode. This mode is set by + // the "bin/search index" tool. + + $is_strict = idx($data, 'strict', false); + + try { + $object = $this->loadObjectForIndexing($object_phid); + } catch (PhabricatorWorkerPermanentFailureException $ex) { + if ($is_strict) { + throw $ex; + } else { + return; + } + } $engine = id(new PhabricatorIndexEngine()) ->setObject($object); @@ -35,8 +58,11 @@ final class PhabricatorSearchWorker extends PhabricatorWorker { return; } - $key = "index.{$object_phid}"; - $lock = PhabricatorGlobalLock::newLock($key); + $lock = PhabricatorGlobalLock::newLock( + 'index', + array( + 'objectPHID' => $object_phid, + )); try { $lock->lock(1); @@ -48,29 +74,34 @@ final class PhabricatorSearchWorker extends PhabricatorWorker { throw new PhabricatorWorkerYieldException(15); } + $caught = null; try { // Reload the object now that we have a lock, to make sure we have the // most current version. $object = $this->loadObjectForIndexing($object->getPHID()); $engine->setObject($object); - $engine->indexObject(); } catch (Exception $ex) { - $lock->unlock(); + $caught = $ex; + } - if (!($ex instanceof PhabricatorWorkerPermanentFailureException)) { - $ex = new PhabricatorWorkerPermanentFailureException( + // Release the lock before we deal with the exception. + $lock->unlock(); + + if ($caught) { + if (!($caught instanceof PhabricatorWorkerPermanentFailureException)) { + $caught = new PhabricatorWorkerPermanentFailureException( pht( 'Failed to update search index for document "%s": %s', $object_phid, - $ex->getMessage())); + $caught->getMessage())); } - throw $ex; + if ($is_strict) { + throw $caught; + } } - - $lock->unlock(); } private function loadObjectForIndexing($phid) { From deea2f01f5d285aa0692803b285ff7f46706f0e2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 15 Feb 2019 06:24:49 -0800 Subject: [PATCH 13/30] Allow unit tests to have arbitrarily long names (>255 characters) Summary: Depends on D20179. Ref T13088. See PHI351. See PHI1018. In various cases, unit tests names are 19 paths mashed together. This is probably not an ideal name, and the test harness should probably pick a better name, but if users are fine with it and don't want to do the work to summarize on their own, accept them. We may summarize with "..." in some cases depending on how this fares in the UI. The actual implementation is a separate "strings" table which is just ``. The unit message table can end up being mostly strings, so this should reduce storage requirements a bit. For now, I'm not forcing a migration: new writes use the new table, existing rows retain the data. I plan to provide a migration tool, recommend migration, then force migration eventually. Prior to that, I'm likely to move at least some other columns to use this table (e.g., lint names), since we have a lot of similar data (arbitrarily long user string constants that we are unlikely to need to search or filter). Test Plan: {F6213819} Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D20180 --- .../20190215.harbor.01.stringindex.sql | 6 +++ .../20190215.harbor.02.stringcol.sql | 2 + src/__phutil_library_map__.php | 2 + .../HarbormasterBuildUnitMessageQuery.php | 31 +++++++++++ .../storage/HarbormasterString.php | 54 +++++++++++++++++++ .../build/HarbormasterBuildUnitMessage.php | 29 ++++++++++ 6 files changed, 124 insertions(+) create mode 100644 resources/sql/autopatches/20190215.harbor.01.stringindex.sql create mode 100644 resources/sql/autopatches/20190215.harbor.02.stringcol.sql create mode 100644 src/applications/harbormaster/storage/HarbormasterString.php diff --git a/resources/sql/autopatches/20190215.harbor.01.stringindex.sql b/resources/sql/autopatches/20190215.harbor.01.stringindex.sql new file mode 100644 index 0000000000..e94b240bab --- /dev/null +++ b/resources/sql/autopatches/20190215.harbor.01.stringindex.sql @@ -0,0 +1,6 @@ +CREATE TABLE {$NAMESPACE}_harbormaster.harbormaster_string ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + stringIndex BINARY(12) NOT NULL, + stringValue LONGTEXT NOT NULL, + UNIQUE KEY `key_string` (stringIndex) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20190215.harbor.02.stringcol.sql b/resources/sql/autopatches/20190215.harbor.02.stringcol.sql new file mode 100644 index 0000000000..acfdb0f869 --- /dev/null +++ b/resources/sql/autopatches/20190215.harbor.02.stringcol.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildunitmessage + ADD nameIndex BINARY(12) NOT NULL; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 94ba258f53..ea3bf7d32d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1441,6 +1441,7 @@ phutil_register_library_map(array( 'HarbormasterStepDeleteController' => 'applications/harbormaster/controller/HarbormasterStepDeleteController.php', 'HarbormasterStepEditController' => 'applications/harbormaster/controller/HarbormasterStepEditController.php', 'HarbormasterStepViewController' => 'applications/harbormaster/controller/HarbormasterStepViewController.php', + 'HarbormasterString' => 'applications/harbormaster/storage/HarbormasterString.php', 'HarbormasterTargetEngine' => 'applications/harbormaster/engine/HarbormasterTargetEngine.php', 'HarbormasterTargetSearchAPIMethod' => 'applications/harbormaster/conduit/HarbormasterTargetSearchAPIMethod.php', 'HarbormasterTargetWorker' => 'applications/harbormaster/worker/HarbormasterTargetWorker.php', @@ -7070,6 +7071,7 @@ phutil_register_library_map(array( 'HarbormasterStepDeleteController' => 'HarbormasterPlanController', 'HarbormasterStepEditController' => 'HarbormasterPlanController', 'HarbormasterStepViewController' => 'HarbormasterPlanController', + 'HarbormasterString' => 'HarbormasterDAO', 'HarbormasterTargetEngine' => 'Phobject', 'HarbormasterTargetSearchAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'HarbormasterTargetWorker' => 'HarbormasterWorker', diff --git a/src/applications/harbormaster/query/HarbormasterBuildUnitMessageQuery.php b/src/applications/harbormaster/query/HarbormasterBuildUnitMessageQuery.php index fbd33a5d95..f73016a29f 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildUnitMessageQuery.php +++ b/src/applications/harbormaster/query/HarbormasterBuildUnitMessageQuery.php @@ -57,6 +57,37 @@ final class HarbormasterBuildUnitMessageQuery return $where; } + protected function didFilterPage(array $messages) { + $indexes = array(); + foreach ($messages as $message) { + $index = $message->getNameIndex(); + if (strlen($index)) { + $indexes[$index] = $index; + } + } + + if ($indexes) { + $map = HarbormasterString::newIndexMap($indexes); + + foreach ($messages as $message) { + $index = $message->getNameIndex(); + + if (!strlen($index)) { + continue; + } + + $name = idx($map, $index); + if ($name === null) { + $name = pht('Unknown Unit Message ("%s")', $index); + } + + $message->setName($name); + } + } + + return $messages; + } + public function getQueryApplicationClass() { return 'PhabricatorHarbormasterApplication'; } diff --git a/src/applications/harbormaster/storage/HarbormasterString.php b/src/applications/harbormaster/storage/HarbormasterString.php new file mode 100644 index 0000000000..7493e60e21 --- /dev/null +++ b/src/applications/harbormaster/storage/HarbormasterString.php @@ -0,0 +1,54 @@ + false, + self::CONFIG_COLUMN_SCHEMA => array( + 'stringIndex' => 'bytes12', + 'stringValue' => 'text', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_string' => array( + 'columns' => array('stringIndex'), + 'unique' => true, + ), + ), + ) + parent::getConfiguration(); + } + + public static function newIndex($string) { + $index = PhabricatorHash::digestForIndex($string); + + $table = new self(); + $conn = $table->establishConnection('w'); + + queryfx( + $conn, + 'INSERT IGNORE INTO %R (stringIndex, stringValue) VALUES (%s, %s)', + $table, + $index, + $string); + + return $index; + } + + public static function newIndexMap(array $indexes) { + $table = new self(); + $conn = $table->establishConnection('r'); + + $rows = queryfx_all( + $conn, + 'SELECT stringIndex, stringValue FROM %R WHERE stringIndex IN (%Ls)', + $table, + $indexes); + + return ipull($rows, 'stringValue', 'stringIndex'); + } + +} diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php b/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php index 1f5e5e1440..9e437efaba 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildUnitMessage.php @@ -8,6 +8,7 @@ final class HarbormasterBuildUnitMessage protected $engine; protected $namespace; protected $name; + protected $nameIndex; protected $result; protected $duration; protected $properties = array(); @@ -132,6 +133,7 @@ final class HarbormasterBuildUnitMessage 'engine' => 'text255', 'namespace' => 'text255', 'name' => 'text255', + 'nameIndex' => 'bytes12', 'result' => 'text32', 'duration' => 'double?', ), @@ -260,6 +262,33 @@ final class HarbormasterBuildUnitMessage return implode("\0", $parts); } + public function save() { + if ($this->nameIndex === null) { + $this->nameIndex = HarbormasterString::newIndex($this->getName()); + } + + // See T13088. While we're letting installs do online migrations to avoid + // downtime, don't populate the "name" column for new writes. New writes + // use the "HarbormasterString" table instead. + $old_name = $this->name; + $this->name = ''; + + $caught = null; + try { + $result = parent::save(); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->name = $old_name; + + if ($caught) { + throw $caught; + } + + return $result; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ From 661c758ff9d1421d2298cac902ca001e3f30a313 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 15 Feb 2019 08:10:56 -0800 Subject: [PATCH 14/30] Render indent depth changes more clearly Summary: Ref T13161. See PHI723. Our whitespace handling is based on whitespace flags like `diff -bw`, mostly just for historical reasons: long ago, the easiest way to minimize the visual impact of indentation changes was to literally use `diff -bw`. However, this approach is very coarse and has a lot of problems, like detecting `"ab" -> "a b"` as "only a whitespace change" even though this is always semantic. It also causes problems in YAML, Python, etc. Over time, we've added a lot of stuff to mitigate the downsides to this approach. We also no longer get any benefits from this approach being simple: we need faithful diffs as the authoritative source, and have to completely rebuild the diff to `diff -bw` it. In the UI, we have a "whitespace mode" flag. We have the "whitespace matters" configuration. I think ReviewBoard generally has a better approach to indent depth changes than we do (see T13161) where it detects them and renders them in a minimal way with low visual impact. This is ultimately what we want: reduce visual clutter for depth-only changes, but preserve whitespace changes in strings, etc. Move toward detecting and rendering indent depth changes. Followup work: - These should get colorblind colors and the design can probably use a little more tweaking. - The OneUp mode is okay, but could be improved. - Whitespace mode can now be removed completely. - I'm trying to handle tabs correctly, but since we currently mangle them into spaces today, it's hard to be sure I actually got it right. Test Plan: {F6214084} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13161 Differential Revision: https://secure.phabricator.com/D20181 --- resources/celerity/map.php | 14 +- .../CelerityDefaultPostprocessor.php | 2 + .../data/whitespace.diff.one.whitespace | 2 +- .../data/whitespace.diff.two.whitespace | 2 +- .../parser/DifferentialChangesetParser.php | 17 +- .../parser/DifferentialHunkParser.php | 171 +++++++++++++++++- .../render/DifferentialChangesetRenderer.php | 10 + .../DifferentialChangesetTestRenderer.php | 4 + .../DifferentialChangesetTwoUpRenderer.php | 24 ++- .../differential/changeset-view.css | 21 +++ webroot/rsrc/image/chevron-in.png | Bin 0 -> 1409 bytes webroot/rsrc/image/chevron-out.png | Bin 0 -> 1417 bytes 12 files changed, 251 insertions(+), 16 deletions(-) create mode 100644 webroot/rsrc/image/chevron-in.png create mode 100644 webroot/rsrc/image/chevron-out.png diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 26aec85659..056535074c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ return array( 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '261ee8cf', 'core.pkg.js' => '5ace8a1e', - 'differential.pkg.css' => 'b8df73d4', + 'differential.pkg.css' => 'c3f15714', 'differential.pkg.js' => '67c9ea4c', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', @@ -61,7 +61,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '4267d6c6', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', - 'rsrc/css/application/differential/changeset-view.css' => '73660575', + 'rsrc/css/application/differential/changeset-view.css' => '783a9206', 'rsrc/css/application/differential/core.css' => 'bdb93065', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', @@ -275,6 +275,8 @@ return array( 'rsrc/image/checker_dark.png' => '7fc8fa7b', 'rsrc/image/checker_light.png' => '3157a202', 'rsrc/image/checker_lighter.png' => 'c45928c1', + 'rsrc/image/chevron-in.png' => '1aa2f88f', + 'rsrc/image/chevron-out.png' => 'c815e272', 'rsrc/image/controls/checkbox-checked.png' => '1770d7a0', 'rsrc/image/controls/checkbox-unchecked.png' => 'e1deba0a', 'rsrc/image/d5d8e1.png' => '6764616e', @@ -539,7 +541,7 @@ return array( 'conpherence-thread-manager' => 'aec8e38c', 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => 'd67475f5', - 'differential-changeset-view-css' => '73660575', + 'differential-changeset-view-css' => '783a9206', 'differential-core-view-css' => 'bdb93065', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', @@ -1490,9 +1492,6 @@ return array( 'javelin-dom', 'javelin-uri', ), - 73660575 => array( - 'phui-inline-comment-view-css', - ), '73ecc1f8' => array( 'javelin-behavior', 'javelin-behavior-device', @@ -1514,6 +1513,9 @@ return array( 'javelin-uri', 'javelin-request', ), + '783a9206' => array( + 'phui-inline-comment-view-css', + ), '78bc5d94' => array( 'javelin-behavior', 'javelin-uri', diff --git a/src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php b/src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php index 61f6176f15..d848fb81e8 100644 --- a/src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php +++ b/src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php @@ -199,8 +199,10 @@ final class CelerityDefaultPostprocessor 'diff.background' => '#fff', 'new-background' => 'rgba(151, 234, 151, .3)', 'new-bright' => 'rgba(151, 234, 151, .6)', + 'new-background-strong' => 'rgba(151, 234, 151, 1)', 'old-background' => 'rgba(251, 175, 175, .3)', 'old-bright' => 'rgba(251, 175, 175, .7)', + 'old-background-strong' => 'rgba(251, 175, 175, 1)', 'move-background' => '#fdf5d4', 'copy-background' => '#f1c40f', diff --git a/src/applications/differential/__tests__/data/whitespace.diff.one.whitespace b/src/applications/differential/__tests__/data/whitespace.diff.one.whitespace index f4a5af6f3e..db43e02158 100644 --- a/src/applications/differential/__tests__/data/whitespace.diff.one.whitespace +++ b/src/applications/differential/__tests__/data/whitespace.diff.one.whitespace @@ -1,2 +1,2 @@ O 1 - -=[-Rocket-Ship>\n~ -N 1 + {( )}-=[-Rocket-Ship>\n~ +N 1 + {> )}-=[-Rocket-Ship>\n~ diff --git a/src/applications/differential/__tests__/data/whitespace.diff.two.whitespace b/src/applications/differential/__tests__/data/whitespace.diff.two.whitespace index f4a5af6f3e..db43e02158 100644 --- a/src/applications/differential/__tests__/data/whitespace.diff.two.whitespace +++ b/src/applications/differential/__tests__/data/whitespace.diff.two.whitespace @@ -1,2 +1,2 @@ O 1 - -=[-Rocket-Ship>\n~ -N 1 + {( )}-=[-Rocket-Ship>\n~ +N 1 + {> )}-=[-Rocket-Ship>\n~ diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 27d2a2d845..22f31b3e9f 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -8,6 +8,7 @@ final class DifferentialChangesetParser extends Phobject { protected $new = array(); protected $old = array(); protected $intra = array(); + protected $depthOnlyLines = array(); protected $newRender = null; protected $oldRender = null; @@ -190,7 +191,7 @@ final class DifferentialChangesetParser extends Phobject { return $this; } - const CACHE_VERSION = 11; + const CACHE_VERSION = 12; const CACHE_MAX_SIZE = 8e6; const ATTR_GENERATED = 'attr:generated'; @@ -224,6 +225,15 @@ final class DifferentialChangesetParser extends Phobject { return $this; } + public function setDepthOnlyLines(array $lines) { + $this->depthOnlyLines = $lines; + return $this; + } + + public function getDepthOnlyLines() { + return $this->depthOnlyLines; + } + public function setVisibileLinesMask(array $mask) { $this->visible = $mask; return $this; @@ -450,6 +460,7 @@ final class DifferentialChangesetParser extends Phobject { 'new', 'old', 'intra', + 'depthOnlyLines', 'newRender', 'oldRender', 'specialAttributes', @@ -754,6 +765,7 @@ final class DifferentialChangesetParser extends Phobject { $this->setOldLines($hunk_parser->getOldLines()); $this->setNewLines($hunk_parser->getNewLines()); $this->setIntraLineDiffs($hunk_parser->getIntraLineDiffs()); + $this->setDepthOnlyLines($hunk_parser->getDepthOnlyLines()); $this->setVisibileLinesMask($hunk_parser->getVisibleLinesMask()); $this->hunkStartLines = $hunk_parser->getHunkStartLines( $changeset->getHunks()); @@ -914,7 +926,8 @@ final class DifferentialChangesetParser extends Phobject { ->setShowEditAndReplyLinks($this->getShowEditAndReplyLinks()) ->setCanMarkDone($this->getCanMarkDone()) ->setObjectOwnerPHID($this->getObjectOwnerPHID()) - ->setHighlightingDisabled($this->highlightingDisabled); + ->setHighlightingDisabled($this->highlightingDisabled) + ->setDepthOnlyLines($this->getDepthOnlyLines()); $shield = null; if ($this->isTopLevel && !$this->comments) { diff --git a/src/applications/differential/parser/DifferentialHunkParser.php b/src/applications/differential/parser/DifferentialHunkParser.php index 5bd98e9012..e7b9ce21a9 100644 --- a/src/applications/differential/parser/DifferentialHunkParser.php +++ b/src/applications/differential/parser/DifferentialHunkParser.php @@ -5,6 +5,7 @@ final class DifferentialHunkParser extends Phobject { private $oldLines; private $newLines; private $intraLineDiffs; + private $depthOnlyLines; private $visibleLinesMask; private $whitespaceMode; @@ -115,6 +116,14 @@ final class DifferentialHunkParser extends Phobject { return $this; } + public function setDepthOnlyLines(array $map) { + $this->depthOnlyLines = $map; + return $this; + } + + public function getDepthOnlyLines() { + return $this->depthOnlyLines; + } public function setWhitespaceMode($white_space_mode) { $this->whitespaceMode = $white_space_mode; @@ -334,6 +343,7 @@ final class DifferentialHunkParser extends Phobject { $new = $this->getNewLines(); $diffs = array(); + $depth_only = array(); foreach ($old as $key => $o) { $n = $new[$key]; @@ -342,13 +352,75 @@ final class DifferentialHunkParser extends Phobject { } if ($o['type'] != $n['type']) { - $diffs[$key] = ArcanistDiffUtils::generateIntralineDiff( - $o['text'], - $n['text']); + $o_segments = array(); + $n_segments = array(); + $tab_width = 2; + + $o_text = $o['text']; + $n_text = $n['text']; + + if ($o_text !== $n_text) { + $o_depth = $this->getIndentDepth($o_text, $tab_width); + $n_depth = $this->getIndentDepth($n_text, $tab_width); + + if ($o_depth < $n_depth) { + $segment_type = '>'; + $segment_width = $this->getCharacterCountForVisualWhitespace( + $n_text, + ($n_depth - $o_depth), + $tab_width); + if ($segment_width) { + $n_text = substr($n_text, $segment_width); + $n_segments[] = array( + $segment_type, + $segment_width, + ); + } + } else if ($o_depth > $n_depth) { + $segment_type = '<'; + $segment_width = $this->getCharacterCountForVisualWhitespace( + $o_text, + ($o_depth - $n_depth), + $tab_width); + if ($segment_width) { + $o_text = substr($o_text, $segment_width); + $o_segments[] = array( + $segment_type, + $segment_width, + ); + } + } + + // If there are no remaining changes to this line after we've marked + // off the indent depth changes, this line was only modified by + // changing the indent depth. Mark it for later so we can change how + // it is displayed. + if ($o_text === $n_text) { + $depth_only[$key] = $segment_type; + } + } + + $intraline_segments = ArcanistDiffUtils::generateIntralineDiff( + $o_text, + $n_text); + + foreach ($intraline_segments[0] as $o_segment) { + $o_segments[] = $o_segment; + } + + foreach ($intraline_segments[1] as $n_segment) { + $n_segments[] = $n_segment; + } + + $diffs[$key] = array( + $o_segments, + $n_segments, + ); } } $this->setIntraLineDiffs($diffs); + $this->setDepthOnlyLines($depth_only); return $this; } @@ -671,4 +743,97 @@ final class DifferentialHunkParser extends Phobject { return $offsets; } + + private function getIndentDepth($text, $tab_width) { + $len = strlen($text); + + $depth = 0; + for ($ii = 0; $ii < $len; $ii++) { + $c = $text[$ii]; + + // If this is a space, increase the indent depth by 1. + if ($c == ' ') { + $depth++; + continue; + } + + // If this is a tab, increase the indent depth to the next tabstop. + + // For example, if the tab width is 4, these sequences both lead us to + // a visual width of 8, i.e. the cursor will be in the 8th column: + // + // + // + + if ($c == "\t") { + $depth = ($depth + $tab_width); + $depth = $depth - ($depth % $tab_width); + continue; + } + + break; + } + + return $depth; + } + + private function getCharacterCountForVisualWhitespace( + $text, + $depth, + $tab_width) { + + // Here, we know the visual indent depth of a line has been increased by + // some amount (for example, 6 characters). + + // We want to find the largest whitespace prefix of the string we can + // which still fits into that amount of visual space. + + // In most cases, this is very easy. For example, if the string has been + // indented by two characters and the string begins with two spaces, that's + // a perfect match. + + // However, if the string has been indented by 7 characters, the tab width + // is 8, and the string begins with "", we can only + // mark the two spaces as an indent change. These cases are unusual. + + $character_depth = 0; + $visual_depth = 0; + + $len = strlen($text); + for ($ii = 0; $ii < $len; $ii++) { + if ($visual_depth >= $depth) { + break; + } + + $c = $text[$ii]; + + if ($c == ' ') { + $character_depth++; + $visual_depth++; + continue; + } + + if ($c == "\t") { + // Figure out how many visual spaces we have until the next tabstop. + $tab_visual = ($visual_depth + $tab_width); + $tab_visual = $tab_visual - ($tab_visual % $tab_width); + $tab_visual = ($tab_visual - $visual_depth); + + // If this tab would take us over the limit, we're all done. + $remaining_depth = ($depth - $visual_depth); + if ($remaining_depth < $tab_visual) { + break; + } + + $character_depth++; + $visual_depth += $tab_visual; + continue; + } + + break; + } + + return $character_depth; + } + } diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index f295695286..c5d033a4a9 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -34,6 +34,7 @@ abstract class DifferentialChangesetRenderer extends Phobject { private $objectOwnerPHID; private $highlightingDisabled; private $scopeEngine; + private $depthOnlyLines; private $oldFile = false; private $newFile = false; @@ -92,6 +93,15 @@ abstract class DifferentialChangesetRenderer extends Phobject { return $this->gaps; } + public function setDepthOnlyLines(array $lines) { + $this->depthOnlyLines = $lines; + return $this; + } + + public function getDepthOnlyLines() { + return $this->depthOnlyLines; + } + public function attachOldFile(PhabricatorFile $old = null) { $this->oldFile = $old; return $this; diff --git a/src/applications/differential/render/DifferentialChangesetTestRenderer.php b/src/applications/differential/render/DifferentialChangesetTestRenderer.php index a0d1fad0eb..c7b35d1fb4 100644 --- a/src/applications/differential/render/DifferentialChangesetTestRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTestRenderer.php @@ -96,10 +96,14 @@ abstract class DifferentialChangesetTestRenderer array( '', '', + '', + '', ), array( '{(', ')}', + '{<', + '{>', ), $render); diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index f40a7f5e0b..b4936201e0 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -71,8 +71,8 @@ final class DifferentialChangesetTwoUpRenderer $mask = $this->getMask(); $scope_engine = $this->getScopeEngine(); - $offset_map = null; + $depth_only = $this->getDepthOnlyLines(); for ($ii = $range_start; $ii < $range_start + $range_len; $ii++) { if (empty($mask[$ii])) { @@ -196,11 +196,29 @@ final class DifferentialChangesetTwoUpRenderer } else if (empty($old_lines[$ii])) { $n_class = 'new new-full'; } else { - $n_class = 'new'; + + // NOTE: At least for the moment, I'm intentionally clearing the + // line highlighting only on the right side of the diff when a + // line has only depth changes. When a block depth is decreased, + // this gives us a large color block on the left (to make it easy + // to see the depth change) but a clean diff on the right (to make + // it easy to pick out actual code changes). + + if (isset($depth_only[$ii])) { + $n_class = ''; + } else { + $n_class = 'new'; + } } $n_classes = $n_class; - if ($new_lines[$ii]['type'] == '\\' || !isset($copy_lines[$n_num])) { + $not_copied = + // If this line only changed depth, copy markers are pointless. + (!isset($copy_lines[$n_num])) || + (isset($depth_only[$ii])) || + ($new_lines[$ii]['type'] == '\\'); + + if ($not_copied) { $n_copy = phutil_tag('td', array('class' => "copy {$n_class}")); } else { list($orig_file, $orig_line, $orig_type) = $copy_lines[$n_num]; diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index b9683dc7c6..2cfa753a42 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -135,12 +135,33 @@ background: {$old-bright}; } + .differential-diff td.new span.bright, .differential-diff td.new-full, .prose-diff span.new { background: {$new-bright}; } +.differential-diff td span.depth-out, +.differential-diff td span.depth-in { + padding: 2px 0; + background-size: 12px 12px; + background-repeat: no-repeat; + background-position: left center; +} + +.differential-diff td span.depth-out { + background-image: url(/rsrc/image/chevron-out.png); + background-color: {$old-background-strong}; +} + +.differential-diff td span.depth-in { + background-position: 2px center; + background-image: url(/rsrc/image/chevron-in.png); + background-color: {$new-background-strong}; +} + + .differential-diff td.copy { min-width: 0.5%; width: 0.5%; diff --git a/webroot/rsrc/image/chevron-in.png b/webroot/rsrc/image/chevron-in.png new file mode 100644 index 0000000000000000000000000000000000000000..373d39cfe1a0b85b7afae5cb8e7e77c63f763f37 GIT binary patch literal 1409 zcmbVMYfKzf6rQDpQUpTQv{lm7={8hAXXmlAj~Q9ZV?kC}3foN}fhzL?rm!>X?9g2( z1jJCgBD6r%SfNcUg%qQrXdop-Y@@XirD{#7T7qd}qroE8HbPVN4zTDC(jQLl%)R&A z^PThE^O#U!!BcTDi7^la#W`~wZZKoo8yy9{#Sx1yg6Uy3yF}e5SEvC-5uglSW(C;k zWhw=?!0>g?T@=zFXhTr+l&B@He43NJMn=;y2E9IjhM=@PK_A0a3o6VCm7-)f%#VF! zfJNSJC^ow=moH1G5_9VnVPAcLhpVsVDBiH=NjNP?0|Q<`W#FK%z|JzjoD}dhgTm1 z@TTw;wA+!h>I>}bhALI{(I^@S1dIWbQC2EZoT4ZcBT#}sfCl2PlT;>%Nd8@63l720 zDWXpmWeL_SGOS#y+6{o|$`!mmmutbqoK}SXbNc zsq+b_Tky-Z3J3C0u`3J)vAe$`O%br6_bMVt3RB~dxmvFvsZNL808Wg&$kU{iCAsN<+*leVL@GQ=n%{*^2 zQ&z(&p8vlQL;)#OE0_P2Pgn$MQd_Py1K6y!lOTZ}Qb4ohPi_4Hf^;XHjto!mySdlj z9X^M}UMP-#p{KW}QQzi>r`j5t9@-kpW;=(whr16+YT!)i+1+I8wY>R3c&y>p^*3TC zZ5>ySPwg&UEP^5n!D#+FZfO5f3x0TMa(uk$j-_lM`TLCAsE*i24)pitZoW5tw^}Y} zxbyMM?<0St>k^U{7Ly))rY{soC^{hee*DDq*(;xR=$iiAm_DD<&i*t!FdjGxU&eKl zUHtPepGePx#>`%hZkyQdPHrXyNb*O)v z?udQU&13k2{%Im~8tVErQx7>uzM6(A9XCgB|J9fJ#TZ24Er&uC?PoH#KRWsioeEtE z#m_C}&(^f|md6cDL+v+`t_61FKhgPG{Vy+d&W#kEY8p;$jz|V$;`mfvvksckpE`Pv S*@kOBMQ3(_l3^w)^1&PVosU-?Y zsp*+{wo31J?^jaDOtDo8H}y5}EpSfF$n>ZxN)4{^3rViZPPR-@vbR&PsjvbXkegbP zs8ErclUHn2VXFi-*9yo63F|8hqhMrUXs&Nys&8PXYhY+)U}0rsr~m~@K--E^(yW49+@N*=dA3R!B_#z``ugSN z<$C4Ddih1^`i7R4mih)p`bI{&Koz>hm3bwJ6}oxF$}kgLQj3#|G7CyF^YauyCMG83 zmzLNn0bL65LT-VtFWlg~VrW1CgG|37u|VHY&pf(~1RD?6IsxA(xEJ)Q4 zN-fSWElLK)N18HBGcfG%TLe-Fbd8mNQ6?}_5_4SglS^|`^GZBjY?XjAdMTMHRwm{q z$;JkjNxGIvX$HC`W`<_ENd^{%x=E=?rsf7lNrs7L#xQfR={K@4Fm^OHb1||sGcz=F zwX|?Ha-hCuU;$XqSVBa{GyQj{2W*+ z2*}7U$uG{xFHmrH2F1FCf`)Hma%LV#P!kkU5P!R*7G;*DrnnX5=PH0h+A0%^E0Piu zjg1XVEOb+nO^kF+5|hkzEi6n@byJcIlT1xhQY=g@&6S|~Q^*ZLeW0WCLCFOv`M`vL zX%fVQX9ge#o}E(jfO)70m{}CP9)>V5FfR9WaSW-r^=77a|6v23hUu3ZwRL25Y%Q!R zN(#O__`BlZ#e=dkGPV&>7Euv5EGMbU&X|0oSJ-!xYW8HS)&GR{Puf{IFMPdD{%Ub? zW&;KgicopK(!S)JZsmFP7wj+Ok83PFIqgQH{)N34)>$=_*PPyx=~!QI{Zs3N1Dx;w z%*wpJFqYZs8q<&WEbiM@CNAbwC|uIuFQXPeyuq8YvCw)@W$^917!!E^1~<`?ZdUNOJRcX12P#2dLRX0GR% z=O$d5(kFHP_@67?x)%H%#SLD+GWeGKIL25}^Y`4H(vw&0(V78r}tV z+*26>=DK+^9$D3EqR;l>ZN#U5dMEdleR~uSy>WOG5dApp(E3jkEtPySiq~H3T>0^l zg2W{~ri!iA3k?ONA|Gzm-=?zgV}rj{*t0cG8y_z}b$7+)dQIK~3_M_@bt<-jk>PUD V61L*pm{3sR=;`X`vd$@?2>`Zs_fh}= literal 0 HcmV?d00001 From 5310f1cdd90ec51331facb79fc1e47a7a824b20e Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 15 Feb 2019 10:23:36 -0800 Subject: [PATCH 15/30] Remove all whitespace options/configuration everywhere Summary: Depends on D20181. Depends on D20182. Fixes T3498. Ref T13161. My claim, at least, is that D20181 can be tweaked to be good enough to throw away this "feature" completely. I think this feature was sort of a mistake, where the ease of access to `diff -bw` shaped behavior a very long time ago and then the train just ran a long way down the tracks in the same direction. Test Plan: Grepped for `whitespace`, deleted almost everything. Poked around the UI a bit. I'm expecting the whitespace changes to get some more iteration this week so I not being hugely pedantic about testing this stuff exhaustively. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13161, T3498 Differential Revision: https://secure.phabricator.com/D20185 --- resources/celerity/map.php | 28 +++--- .../PhabricatorExtraConfigSetupCheck.php | 2 + .../DifferentialParseRenderTestCase.php | 15 +-- .../data/generated.diff.one.unshielded | 3 +- .../data/generated.diff.two.unshielded | 4 +- .../__tests__/data/whitespace.diff.one.expect | 3 +- .../__tests__/data/whitespace.diff.two.expect | 3 +- .../PhabricatorDifferentialConfigOptions.php | 12 --- .../DifferentialRevisionViewController.php | 8 +- .../parser/DifferentialChangesetParser.php | 99 +------------------ .../parser/DifferentialHunkParser.php | 83 ---------------- .../parser/DifferentialLineAdjustmentMap.php | 1 - .../DifferentialChangesetHTMLRenderer.php | 5 - .../render/DifferentialChangesetRenderer.php | 3 - .../storage/DifferentialChangeset.php | 11 --- .../view/DifferentialChangesetDetailView.php | 11 --- .../view/DifferentialChangesetListView.php | 10 +- .../DifferentialRevisionUpdateHistoryView.php | 35 ------- .../controller/DiffusionChangeController.php | 3 - .../controller/DiffusionDiffController.php | 3 - .../user/userguide/differential_faq.diviner | 16 --- .../diff/PhabricatorDifferenceEngine.php | 47 +-------- .../rsrc/js/application/diff/DiffChangeset.js | 3 - 23 files changed, 33 insertions(+), 375 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 056535074c..65eed7916a 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,7 +12,7 @@ return array( 'core.pkg.css' => '261ee8cf', 'core.pkg.js' => '5ace8a1e', 'differential.pkg.css' => 'c3f15714', - 'differential.pkg.js' => '67c9ea4c', + 'differential.pkg.js' => 'be031567', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', 'maniphest.pkg.css' => '35995d6d', @@ -374,7 +374,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '076bd092', 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '9b1cbd76', - 'rsrc/js/application/diff/DiffChangeset.js' => 'e7cf10d6', + 'rsrc/js/application/diff/DiffChangeset.js' => 'd0a85a85', 'rsrc/js/application/diff/DiffChangesetList.js' => 'b91204e9', 'rsrc/js/application/diff/DiffInline.js' => 'a4a14a94', 'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17', @@ -753,7 +753,7 @@ return array( 'phabricator-darklog' => '3b869402', 'phabricator-darkmessage' => '26cd4b73', 'phabricator-dashboard-css' => '4267d6c6', - 'phabricator-diff-changeset' => 'e7cf10d6', + 'phabricator-diff-changeset' => 'd0a85a85', 'phabricator-diff-changeset-list' => 'b91204e9', 'phabricator-diff-inline' => 'a4a14a94', 'phabricator-drag-and-drop-file-upload' => '4370900d', @@ -1973,6 +1973,17 @@ return array( 'javelin-stratcom', 'javelin-util', ), + 'd0a85a85' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-workflow', + 'javelin-router', + 'javelin-behavior-device', + 'javelin-vector', + 'phabricator-diff-inline', + ), 'd12d214f' => array( 'javelin-install', 'javelin-dom', @@ -2038,17 +2049,6 @@ return array( 'javelin-dom', 'phabricator-draggable-list', ), - 'e7cf10d6' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - 'phabricator-diff-inline', - ), 'e8240b50' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index a742e3b82b..b676063e8c 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -533,6 +533,8 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'This ancient extension point has been replaced with other '. 'mechanisms, including "AphrontSite".'), + 'differential.whitespace-matters' => pht( + 'Whitespace rendering is now handled automatically.'), ); return $ancient_config; diff --git a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php b/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php index c278ff0c67..259bb671de 100644 --- a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php +++ b/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php @@ -31,7 +31,6 @@ final class DifferentialParseRenderTestCase extends PhabricatorTestCase { foreach (array('one', 'two') as $type) { $this->runParser($type, $data, $file, 'expect'); $this->runParser($type, $data, $file, 'unshielded'); - $this->runParser($type, $data, $file, 'whitespace'); } } } @@ -44,25 +43,20 @@ final class DifferentialParseRenderTestCase extends PhabricatorTestCase { } $unshielded = false; - $whitespace = false; switch ($extension) { case 'unshielded': $unshielded = true; break; - case 'whitespace'; - $unshielded = true; - $whitespace = true; - break; } $parsers = $this->buildChangesetParsers($type, $data, $file); - $actual = $this->renderParsers($parsers, $unshielded, $whitespace); + $actual = $this->renderParsers($parsers, $unshielded); $expect = Filesystem::readFile($test_file); $this->assertEqual($expect, $actual, basename($test_file)); } - private function renderParsers(array $parsers, $unshield, $whitespace) { + private function renderParsers(array $parsers, $unshield) { $result = array(); foreach ($parsers as $parser) { if ($unshield) { @@ -73,11 +67,6 @@ final class DifferentialParseRenderTestCase extends PhabricatorTestCase { $e_range = null; } - if ($whitespace) { - $parser->setWhitespaceMode( - DifferentialChangesetParser::WHITESPACE_SHOW_ALL); - } - $result[] = $parser->render($s_range, $e_range, array()); } return implode(str_repeat('~', 80)."\n", $result); diff --git a/src/applications/differential/__tests__/data/generated.diff.one.unshielded b/src/applications/differential/__tests__/data/generated.diff.one.unshielded index ca4b1b167e..acfa701c8b 100644 --- a/src/applications/differential/__tests__/data/generated.diff.one.unshielded +++ b/src/applications/differential/__tests__/data/generated.diff.one.unshielded @@ -1,6 +1,5 @@ N 1 . @generated\n~ -O 2 - \n~ +N 2 . \n~ O 3 - This is a generated file.\n~ -N 2 + \n~ N 3 + This is a generated file{(, full of generated code)}.\n~ N 4 . \n~ diff --git a/src/applications/differential/__tests__/data/generated.diff.two.unshielded b/src/applications/differential/__tests__/data/generated.diff.two.unshielded index 967f6220de..183a0b6edc 100644 --- a/src/applications/differential/__tests__/data/generated.diff.two.unshielded +++ b/src/applications/differential/__tests__/data/generated.diff.two.unshielded @@ -1,7 +1,7 @@ O 1 . @generated\n~ N 1 . @generated\n~ -O 2 - \n~ -N 2 + \n~ +O 2 . \n~ +N 2 . \n~ O 3 - This is a generated file.\n~ N 3 + This is a generated file{(, full of generated code)}.\n~ O 4 . \n~ diff --git a/src/applications/differential/__tests__/data/whitespace.diff.one.expect b/src/applications/differential/__tests__/data/whitespace.diff.one.expect index 5b229959d3..87ad1dcdd9 100644 --- a/src/applications/differential/__tests__/data/whitespace.diff.one.expect +++ b/src/applications/differential/__tests__/data/whitespace.diff.one.expect @@ -2,4 +2,5 @@ CTYPE 2 1 (unforced) WHITESPACE WHITESPACE - -SHIELD (whitespace) This file was changed only by adding or removing whitespace. +O 1 - -=[-Rocket-Ship>\n~ +N 1 + {> )}-=[-Rocket-Ship>\n~ diff --git a/src/applications/differential/__tests__/data/whitespace.diff.two.expect b/src/applications/differential/__tests__/data/whitespace.diff.two.expect index 5b229959d3..87ad1dcdd9 100644 --- a/src/applications/differential/__tests__/data/whitespace.diff.two.expect +++ b/src/applications/differential/__tests__/data/whitespace.diff.two.expect @@ -2,4 +2,5 @@ CTYPE 2 1 (unforced) WHITESPACE WHITESPACE - -SHIELD (whitespace) This file was changed only by adding or removing whitespace. +O 1 - -=[-Rocket-Ship>\n~ +N 1 + {> )}-=[-Rocket-Ship>\n~ diff --git a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php index ec2099f8dd..9634756dac 100644 --- a/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php +++ b/src/applications/differential/config/PhabricatorDifferentialConfigOptions.php @@ -80,18 +80,6 @@ EOHELP "Select and reorder revision fields.\n\n". "NOTE: This feature is under active development and subject ". "to change.")), - $this->newOption( - 'differential.whitespace-matters', - 'list', - array( - '/\.py$/', - '/\.l?hs$/', - '/\.ya?ml$/', - )) - ->setDescription( - pht( - "List of file regexps where whitespace is meaningful and should ". - "not use 'ignore-all' by default")), $this->newOption('differential.require-test-plan-field', 'bool', true) ->setBoolOptions( array( diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 9bc6345576..2ac1d30eca 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -305,10 +305,6 @@ final class DifferentialRevisionViewController $details = $this->buildDetails($revision, $field_list); $curtain = $this->buildCurtain($revision); - $whitespace = $request->getStr( - 'whitespace', - DifferentialChangesetParser::WHITESPACE_IGNORE_MOST); - $repository = $revision->getRepository(); if ($repository) { $symbol_indexes = $this->buildSymbolIndexes( @@ -383,7 +379,6 @@ final class DifferentialRevisionViewController ->setDiff($target) ->setRenderingReferences($rendering_references) ->setVsMap($vs_map) - ->setWhitespace($whitespace) ->setSymbolIndexes($symbol_indexes) ->setTitle(pht('Diff %s', $target->getID())) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY); @@ -412,7 +407,6 @@ final class DifferentialRevisionViewController ->setDiffUnitStatuses($broken_diffs) ->setSelectedVersusDiffID($diff_vs) ->setSelectedDiffID($target->getID()) - ->setSelectedWhitespace($whitespace) ->setCommitsForLinks($commits_for_links); $local_table = id(new DifferentialLocalCommitsView()) @@ -1095,7 +1089,7 @@ final class DifferentialRevisionViewController // this ends up being something like // D123.diff // or the verbose - // D123.vs123.id123.whitespaceignore-all.diff + // D123.vs123.id123.highlightjs.diff // lame but nice to include these options $file_name = ltrim($request_uri->getPath(), '/').'.'; foreach ($request_uri->getQueryParamsAsPairList() as $pair) { diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 22f31b3e9f..caa7463672 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -19,7 +19,6 @@ final class DifferentialChangesetParser extends Phobject { protected $specialAttributes = array(); protected $changeset; - protected $whitespaceMode = null; protected $renderCacheKey = null; @@ -163,7 +162,6 @@ final class DifferentialChangesetParser extends Phobject { } public function readParametersFromRequest(AphrontRequest $request) { - $this->setWhitespaceMode($request->getStr('whitespace')); $this->setCharacterEncoding($request->getStr('encoding')); $this->setHighlightAs($request->getStr('highlight')); @@ -191,20 +189,14 @@ final class DifferentialChangesetParser extends Phobject { return $this; } - const CACHE_VERSION = 12; + const CACHE_VERSION = 13; const CACHE_MAX_SIZE = 8e6; const ATTR_GENERATED = 'attr:generated'; const ATTR_DELETED = 'attr:deleted'; const ATTR_UNCHANGED = 'attr:unchanged'; - const ATTR_WHITELINES = 'attr:white'; const ATTR_MOVEAWAY = 'attr:moveaway'; - const WHITESPACE_SHOW_ALL = 'show-all'; - const WHITESPACE_IGNORE_TRAILING = 'ignore-trailing'; - const WHITESPACE_IGNORE_MOST = 'ignore-most'; - const WHITESPACE_IGNORE_ALL = 'ignore-all'; - public function setOldLines(array $lines) { $this->old = $lines; return $this; @@ -336,11 +328,6 @@ final class DifferentialChangesetParser extends Phobject { return $this; } - public function setWhitespaceMode($whitespace_mode) { - $this->whitespaceMode = $whitespace_mode; - return $this; - } - public function setRenderingReference($ref) { $this->renderingReference = $ref; return $this; @@ -574,10 +561,6 @@ final class DifferentialChangesetParser extends Phobject { return idx($this->specialAttributes, self::ATTR_UNCHANGED, false); } - public function isWhitespaceOnly() { - return idx($this->specialAttributes, self::ATTR_WHITELINES, false); - } - public function isMoveAway() { return idx($this->specialAttributes, self::ATTR_MOVEAWAY, false); } @@ -624,18 +607,8 @@ final class DifferentialChangesetParser extends Phobject { } private function tryCacheStuff() { - $whitespace_mode = $this->whitespaceMode; - switch ($whitespace_mode) { - case self::WHITESPACE_SHOW_ALL: - case self::WHITESPACE_IGNORE_TRAILING: - case self::WHITESPACE_IGNORE_ALL: - break; - default: - $whitespace_mode = self::WHITESPACE_IGNORE_MOST; - break; - } + $skip_cache = false; - $skip_cache = ($whitespace_mode != self::WHITESPACE_IGNORE_MOST); if ($this->disableCache) { $skip_cache = true; } @@ -648,8 +621,6 @@ final class DifferentialChangesetParser extends Phobject { $skip_cache = true; } - $this->whitespaceMode = $whitespace_mode; - $changeset = $this->changeset; if ($changeset->getFileType() != DifferentialChangeType::FILE_TEXT && @@ -668,71 +639,10 @@ final class DifferentialChangesetParser extends Phobject { } private function process() { - $whitespace_mode = $this->whitespaceMode; $changeset = $this->changeset; - $ignore_all = (($whitespace_mode == self::WHITESPACE_IGNORE_MOST) || - ($whitespace_mode == self::WHITESPACE_IGNORE_ALL)); - - $force_ignore = ($whitespace_mode == self::WHITESPACE_IGNORE_ALL); - - if (!$force_ignore) { - if ($ignore_all && $changeset->getWhitespaceMatters()) { - $ignore_all = false; - } - } - - // The "ignore all whitespace" algorithm depends on rediffing the - // files, and we currently need complete representations of both - // files to do anything reasonable. If we only have parts of the files, - // don't use the "ignore all" algorithm. - if ($ignore_all) { - $hunks = $changeset->getHunks(); - if (count($hunks) !== 1) { - $ignore_all = false; - } else { - $first_hunk = reset($hunks); - if ($first_hunk->getOldOffset() != 1 || - $first_hunk->getNewOffset() != 1) { - $ignore_all = false; - } - } - } - - if ($ignore_all) { - $old_file = $changeset->makeOldFile(); - $new_file = $changeset->makeNewFile(); - if ($old_file == $new_file) { - // If the old and new files are exactly identical, the synthetic - // diff below will give us nonsense and whitespace modes are - // irrelevant anyway. This occurs when you, e.g., copy a file onto - // itself in Subversion (see T271). - $ignore_all = false; - } - } - $hunk_parser = new DifferentialHunkParser(); - $hunk_parser->setWhitespaceMode($whitespace_mode); $hunk_parser->parseHunksForLineData($changeset->getHunks()); - - // Depending on the whitespace mode, we may need to compute a different - // set of changes than the set of changes in the hunk data (specifically, - // we might want to consider changed lines which have only whitespace - // changes as unchanged). - if ($ignore_all) { - $engine = new PhabricatorDifferenceEngine(); - $engine->setIgnoreWhitespace(true); - $no_whitespace_changeset = $engine->generateChangesetFromFileContent( - $old_file, - $new_file); - - $type_parser = new DifferentialHunkParser(); - $type_parser->parseHunksForLineData($no_whitespace_changeset->getHunks()); - - $hunk_parser->setOldLineTypeMap($type_parser->getOldLineTypeMap()); - $hunk_parser->setNewLineTypeMap($type_parser->getNewLineTypeMap()); - } - $hunk_parser->reparseHunksForSpecialAttributes(); $unchanged = false; @@ -753,7 +663,6 @@ final class DifferentialChangesetParser extends Phobject { $this->setSpecialAttributes(array( self::ATTR_UNCHANGED => $unchanged, self::ATTR_DELETED => $hunk_parser->getIsDeleted(), - self::ATTR_WHITELINES => !$hunk_parser->getHasTextChanges(), self::ATTR_MOVEAWAY => $moveaway, )); @@ -971,10 +880,6 @@ final class DifferentialChangesetParser extends Phobject { pht('The contents of this file were not changed.'), $type); } - } else if ($this->isWhitespaceOnly()) { - $shield = $renderer->renderShield( - pht('This file was changed only by adding or removing whitespace.'), - 'whitespace'); } else if ($this->isDeleted()) { $shield = $renderer->renderShield( pht('This file was completely deleted.')); diff --git a/src/applications/differential/parser/DifferentialHunkParser.php b/src/applications/differential/parser/DifferentialHunkParser.php index e7b9ce21a9..d89089a7e7 100644 --- a/src/applications/differential/parser/DifferentialHunkParser.php +++ b/src/applications/differential/parser/DifferentialHunkParser.php @@ -7,7 +7,6 @@ final class DifferentialHunkParser extends Phobject { private $intraLineDiffs; private $depthOnlyLines; private $visibleLinesMask; - private $whitespaceMode; /** * Get a map of lines on which hunks start, other than line 1. This @@ -125,21 +124,6 @@ final class DifferentialHunkParser extends Phobject { return $this->depthOnlyLines; } - public function setWhitespaceMode($white_space_mode) { - $this->whitespaceMode = $white_space_mode; - return $this; - } - - private function getWhitespaceMode() { - if ($this->whitespaceMode === null) { - throw new Exception( - pht( - 'You must %s before accessing this data.', - 'setWhitespaceMode')); - } - return $this->whitespaceMode; - } - public function getIsDeleted() { foreach ($this->getNewLines() as $line) { if ($line) { @@ -159,13 +143,6 @@ final class DifferentialHunkParser extends Phobject { return false; } - /** - * Returns true if the hunks change any text, not just whitespace. - */ - public function getHasTextChanges() { - return $this->getHasChanges('text'); - } - /** * Returns true if the hunks change anything, including whitespace. */ @@ -193,9 +170,6 @@ final class DifferentialHunkParser extends Phobject { } if ($o['type'] !== $n['type']) { - // The types are different, so either the underlying text is actually - // different or whatever whitespace rules we're using consider them - // different. return true; } @@ -278,63 +252,6 @@ final class DifferentialHunkParser extends Phobject { $this->setOldLines($rebuild_old); $this->setNewLines($rebuild_new); - $this->updateChangeTypesForWhitespaceMode(); - - return $this; - } - - private function updateChangeTypesForWhitespaceMode() { - $mode = $this->getWhitespaceMode(); - - $mode_show_all = DifferentialChangesetParser::WHITESPACE_SHOW_ALL; - if ($mode === $mode_show_all) { - // If we're showing all whitespace, we don't need to perform any updates. - return; - } - - $mode_trailing = DifferentialChangesetParser::WHITESPACE_IGNORE_TRAILING; - $is_trailing = ($mode === $mode_trailing); - - $new = $this->getNewLines(); - $old = $this->getOldLines(); - foreach ($old as $key => $o) { - $n = $new[$key]; - - if (!$o || !$n) { - continue; - } - - if ($is_trailing) { - // In "trailing" mode, we need to identify lines which are marked - // changed but differ only by trailing whitespace. We mark these lines - // unchanged. - if ($o['type'] != $n['type']) { - if (rtrim($o['text']) === rtrim($n['text'])) { - $old[$key]['type'] = null; - $new[$key]['type'] = null; - } - } - } else { - // In "ignore most" and "ignore all" modes, we need to identify lines - // which are marked unchanged but have internal whitespace changes. - // We want to ignore leading and trailing whitespace changes only, not - // internal whitespace changes (`diff` doesn't have a mode for this, so - // we have to fix it here). If the text is marked unchanged but the - // old and new text differs by internal space, mark the lines changed. - if ($o['type'] === null && $n['type'] === null) { - if ($o['text'] !== $n['text']) { - if (trim($o['text']) !== trim($n['text'])) { - $old[$key]['type'] = '-'; - $new[$key]['type'] = '+'; - } - } - } - } - } - - $this->setOldLines($old); - $this->setNewLines($new); - return $this; } diff --git a/src/applications/differential/parser/DifferentialLineAdjustmentMap.php b/src/applications/differential/parser/DifferentialLineAdjustmentMap.php index fde8f61f7d..e30f2ca866 100644 --- a/src/applications/differential/parser/DifferentialLineAdjustmentMap.php +++ b/src/applications/differential/parser/DifferentialLineAdjustmentMap.php @@ -359,7 +359,6 @@ final class DifferentialLineAdjustmentMap extends Phobject { } $changeset = id(new PhabricatorDifferenceEngine()) - ->setIgnoreWhitespace(true) ->generateChangesetFromFileContent($u_old, $v_old); $results[$u][$v] = self::newFromHunks( diff --git a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php index 30acc71c88..2e760d99bb 100644 --- a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php @@ -367,7 +367,6 @@ abstract class DifferentialChangesetHTMLRenderer $reference = $this->getRenderingReference(); if ($force !== 'text' && - $force !== 'whitespace' && $force !== 'none' && $force !== 'default') { throw new Exception( @@ -388,10 +387,6 @@ abstract class DifferentialChangesetHTMLRenderer 'range' => $range, ); - if ($force == 'whitespace') { - $meta['whitespace'] = DifferentialChangesetParser::WHITESPACE_SHOW_ALL; - } - $content = array(); $content[] = $message; if ($force !== 'none') { diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index c5d033a4a9..866ae15ac9 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -406,9 +406,6 @@ abstract class DifferentialChangesetRenderer extends Phobject { * important (e.g., generated code). * - `"text"`: Force the text to be shown. This is probably only relevant * when a file is not changed. - * - `"whitespace"`: Force the text to be shown, and the diff to be - * rendered with all whitespace shown. This is probably only relevant - * when a file is changed only by altering whitespace. * - `"none"`: Don't show the link (e.g., text not available). * * @param string Message explaining why the diff is hidden. diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php index 00af84bc4e..03400d7a16 100644 --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -249,17 +249,6 @@ final class DifferentialChangeset return $path; } - public function getWhitespaceMatters() { - $config = PhabricatorEnv::getEnvConfig('differential.whitespace-matters'); - foreach ($config as $regexp) { - if (preg_match($regexp, $this->getFilename())) { - return true; - } - } - - return false; - } - public function attachDiff(DifferentialDiff $diff) { $this->diff = $diff; return $this; diff --git a/src/applications/differential/view/DifferentialChangesetDetailView.php b/src/applications/differential/view/DifferentialChangesetDetailView.php index cb697c2e9d..211403c8bd 100644 --- a/src/applications/differential/view/DifferentialChangesetDetailView.php +++ b/src/applications/differential/view/DifferentialChangesetDetailView.php @@ -9,7 +9,6 @@ final class DifferentialChangesetDetailView extends AphrontView { private $id; private $vsChangesetID; private $renderURI; - private $whitespace; private $renderingRef; private $autoload; private $loaded; @@ -42,15 +41,6 @@ final class DifferentialChangesetDetailView extends AphrontView { return $this->renderingRef; } - public function setWhitespace($whitespace) { - $this->whitespace = $whitespace; - return $this; - } - - public function getWhitespace() { - return $this->whitespace; - } - public function setRenderURI($render_uri) { $this->renderURI = $render_uri; return $this; @@ -196,7 +186,6 @@ final class DifferentialChangesetDetailView extends AphrontView { 'left' => $left_id, 'right' => $right_id, 'renderURI' => $this->getRenderURI(), - 'whitespace' => $this->getWhitespace(), 'highlight' => null, 'renderer' => $this->getRenderer(), 'ref' => $this->getRenderingRef(), diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index 367991497c..67568005fa 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -7,7 +7,6 @@ final class DifferentialChangesetListView extends AphrontView { private $references = array(); private $inlineURI; private $renderURI = '/differential/changeset/'; - private $whitespace; private $background; private $header; private $isStandalone; @@ -100,11 +99,6 @@ final class DifferentialChangesetListView extends AphrontView { return $this; } - public function setWhitespace($whitespace) { - $this->whitespace = $whitespace; - return $this; - } - public function setVsMap(array $vs_map) { $this->vsMap = $vs_map; return $this; @@ -180,7 +174,6 @@ final class DifferentialChangesetListView extends AphrontView { $detail->setRenderingRef($ref); $detail->setRenderURI($this->renderURI); - $detail->setWhitespace($this->whitespace); $detail->setRenderer($renderer); if ($this->getParser()) { @@ -352,8 +345,7 @@ final class DifferentialChangesetListView extends AphrontView { $meta = array(); $qparams = array( - 'ref' => $ref, - 'whitespace' => $this->whitespace, + 'ref' => $ref, ); if ($this->standaloneURI) { diff --git a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php index a77b320d82..07ca983bc0 100644 --- a/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php +++ b/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php @@ -5,7 +5,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { private $diffs = array(); private $selectedVersusDiffID; private $selectedDiffID; - private $selectedWhitespace; private $commitsForLinks = array(); private $unitStatus = array(); @@ -25,11 +24,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { return $this; } - public function setSelectedWhitespace($whitespace) { - $this->selectedWhitespace = $whitespace; - return $this; - } - public function setCommitsForLinks(array $commits) { assert_instances_of($commits, 'PhabricatorRepositoryCommit'); $this->commitsForLinks = $commits; @@ -224,28 +218,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { 'radios' => $radios, )); - $options = array( - DifferentialChangesetParser::WHITESPACE_IGNORE_ALL => pht('Ignore All'), - DifferentialChangesetParser::WHITESPACE_IGNORE_MOST => pht('Ignore Most'), - DifferentialChangesetParser::WHITESPACE_IGNORE_TRAILING => - pht('Ignore Trailing'), - DifferentialChangesetParser::WHITESPACE_SHOW_ALL => pht('Show All'), - ); - - foreach ($options as $value => $label) { - $options[$value] = phutil_tag( - 'option', - array( - 'value' => $value, - 'selected' => ($value == $this->selectedWhitespace) - ? 'selected' - : null, - ), - $label); - } - $select = phutil_tag('select', array('name' => 'whitespace'), $options); - - $table = id(new AphrontTableView($rows)); $table->setHeaders( array( @@ -291,13 +263,6 @@ final class DifferentialRevisionUpdateHistoryView extends AphrontView { 'class' => 'differential-update-history-footer', ), array( - phutil_tag( - 'label', - array(), - array( - pht('Whitespace Changes:'), - $select, - )), phutil_tag( 'button', array(), diff --git a/src/applications/diffusion/controller/DiffusionChangeController.php b/src/applications/diffusion/controller/DiffusionChangeController.php index 90258134c4..0120c978d0 100644 --- a/src/applications/diffusion/controller/DiffusionChangeController.php +++ b/src/applications/diffusion/controller/DiffusionChangeController.php @@ -64,9 +64,6 @@ final class DiffusionChangeController extends DiffusionController { $changeset_view->setRawFileURIs($left_uri, $right_uri); $changeset_view->setRenderURI($repository->getPathURI('diff/')); - - $changeset_view->setWhitespace( - DifferentialChangesetParser::WHITESPACE_SHOW_ALL); $changeset_view->setUser($viewer); $changeset_view->setHeader($changeset_header); diff --git a/src/applications/diffusion/controller/DiffusionDiffController.php b/src/applications/diffusion/controller/DiffusionDiffController.php index 86409c6faa..a0111d001f 100644 --- a/src/applications/diffusion/controller/DiffusionDiffController.php +++ b/src/applications/diffusion/controller/DiffusionDiffController.php @@ -88,9 +88,6 @@ final class DiffusionDiffController extends DiffusionController { ($viewer->getPHID() == $commit->getAuthorPHID())); $parser->setObjectOwnerPHID($commit->getAuthorPHID()); - $parser->setWhitespaceMode( - DifferentialChangesetParser::WHITESPACE_SHOW_ALL); - $inlines = PhabricatorAuditInlineComment::loadDraftAndPublishedComments( $viewer, $commit->getPHID(), diff --git a/src/docs/user/userguide/differential_faq.diviner b/src/docs/user/userguide/differential_faq.diviner index aea49c4ce6..0df1f7b1c9 100644 --- a/src/docs/user/userguide/differential_faq.diviner +++ b/src/docs/user/userguide/differential_faq.diviner @@ -51,22 +51,6 @@ You need to install and configure **Pygments** to highlight anything else than PHP. See the `pygments.enabled` configuration setting. -= What do the whitespace options mean? = - -Most of these are pretty straightforward, but "Ignore Most" is not: - - - **Show All**: Show all whitespace. - - **Ignore Trailing**: Ignore changes which only affect trailing whitespace. - - **Ignore Most**: Ignore changes which only affect leading or trailing - whitespace (but not whitespace changes between non-whitespace characters) - in files which are not marked as having significant whitespace. - In those files, show whitespace changes. By default, Python (.py) and - Haskell (.lhs, .hs) are marked as having significant whitespace, but this - can be changed in the `differential.whitespace-matters` configuration - setting. - - **Ignore All**: Ignore all whitespace changes in all files. - - = What do the very light green and red backgrounds mean? = Differential uses these colors to mark changes coming from rebase: they are diff --git a/src/infrastructure/diff/PhabricatorDifferenceEngine.php b/src/infrastructure/diff/PhabricatorDifferenceEngine.php index 3b4eb55473..90f1d9b10e 100644 --- a/src/infrastructure/diff/PhabricatorDifferenceEngine.php +++ b/src/infrastructure/diff/PhabricatorDifferenceEngine.php @@ -10,7 +10,6 @@ final class PhabricatorDifferenceEngine extends Phobject { - private $ignoreWhitespace; private $oldName; private $newName; @@ -18,19 +17,6 @@ final class PhabricatorDifferenceEngine extends Phobject { /* -( Configuring the Engine )--------------------------------------------- */ - /** - * If true, ignore whitespace when computing differences. - * - * @param bool Ignore whitespace? - * @return this - * @task config - */ - public function setIgnoreWhitespace($ignore_whitespace) { - $this->ignoreWhitespace = $ignore_whitespace; - return $this; - } - - /** * Set the name to identify the old file with. Primarily cosmetic. * @@ -73,9 +59,6 @@ final class PhabricatorDifferenceEngine extends Phobject { public function generateRawDiffFromFileContent($old, $new) { $options = array(); - if ($this->ignoreWhitespace) { - $options[] = '-bw'; - } // Generate diffs with full context. $options[] = '-U65535'; @@ -100,12 +83,10 @@ final class PhabricatorDifferenceEngine extends Phobject { $new_tmp); if (!$err) { - // This indicates that the two files are the same (or, possibly, the - // same modulo whitespace differences, which is why we can't do this - // check trivially before running `diff`). Build a synthetic, changeless - // diff so that we can still render the raw, unchanged file instead of - // being forced to just say "this file didn't change" since we don't have - // the content. + // This indicates that the two files are the same. Build a synthetic, + // changeless diff so that we can still render the raw, unchanged file + // instead of being forced to just say "this file didn't change" since we + // don't have the content. $entire_file = explode("\n", $old); foreach ($entire_file as $k => $line) { @@ -123,26 +104,6 @@ final class PhabricatorDifferenceEngine extends Phobject { "+++ {$new_name}\n". "@@ -1,{$len} +1,{$len} @@\n". $entire_file."\n"; - } else { - if ($this->ignoreWhitespace) { - - // Under "-bw", `diff` is inconsistent about emitting "\ No newline - // at end of file". For instance, a long file with a change in the - // middle will emit a contextless "\ No newline..." at the end if a - // newline is removed, but not if one is added. A file with a change - // at the end will emit the "old" "\ No newline..." block only, even - // if the newline was not removed. Since we're ostensibly ignoring - // whitespace changes, just drop these lines if they appear anywhere - // in the diff. - - $lines = explode("\n", $diff); - foreach ($lines as $key => $line) { - if (isset($line[0]) && $line[0] == '\\') { - unset($lines[$key]); - } - } - $diff = implode("\n", $lines); - } } return $diff; diff --git a/webroot/rsrc/js/application/diff/DiffChangeset.js b/webroot/rsrc/js/application/diff/DiffChangeset.js index 24d734573d..754f3b16e4 100644 --- a/webroot/rsrc/js/application/diff/DiffChangeset.js +++ b/webroot/rsrc/js/application/diff/DiffChangeset.js @@ -22,7 +22,6 @@ JX.install('DiffChangeset', { this._renderURI = data.renderURI; this._ref = data.ref; - this._whitespace = data.whitespace; this._renderer = data.renderer; this._highlight = data.highlight; this._encoding = data.encoding; @@ -46,7 +45,6 @@ JX.install('DiffChangeset', { _renderURI: null, _ref: null, - _whitespace: null, _renderer: null, _highlight: null, _encoding: null, @@ -310,7 +308,6 @@ JX.install('DiffChangeset', { _getViewParameters: function() { return { ref: this._ref, - whitespace: this._whitespace || '', renderer: this.getRenderer() || '', highlight: this._highlight || '', encoding: this._encoding || '' From 3f8eccdaec8f85933bfea6ce5348bde25ced0489 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 16 Feb 2019 07:21:31 -0800 Subject: [PATCH 16/30] Put some whitespace behaviors back, but only for "diff alignment", not display Summary: Depends on D20185. Ref T13161. Fixes T6791. See some discusison in T13161. I want to move to a world where: - whitespace changes are always shown, so users writing YAML and Python are happy without adjusting settings; - the visual impact of indentation-only whitespace changes is significanlty reduced, so indentation changes are easy to read and users writing Javascript or other flavors of Javascript are happy. D20181 needs a little more work, but generally tackles these visual changes and lets us always show whitespace changes, but show them in a very low-impact way when they're relatively unimportant. However, a second aspect to this is how the diff is "aligned". If this file: ``` A ``` ..is changed to this file: ``` X A Y Z ``` ...diff tools will generally produce this diff: ``` + X A + Y + Z ``` This is good, and easy to read, and what humans expect, and it will "align" in two-up like this: ``` 1 X 1 A 2 A 3 Y 4 Z ``` However, if the new file looks like this instead: ``` X A' Y Z ``` ...we get a diff like this: ``` - A + X + A' + Y + Z ``` This one aligns like this: ``` 1 A 1 X 2 A' 3 Y 4 Z ``` This is correct if `A` and `A'` are totally different lines. However, if `A'` is pretty much the same as `A` and it just had a whitespace change, human viewers would prefer this alignment: ``` 1 X 1 A 2 A' 3 Y 4 Z ``` Note that `A` and `A'` are different, but we've aligned them on the same line. `diff`, `git diff`, etc., won't do this automatically, and a `.diff` doesn't have a way to say "these lines are more or less the same even though they're different", although some other visual diff tools will do this. Although `diff` can't do this for us, we can do it ourselves, and already have the code to do it, because we already nearly did this in the changes removed in D20185: in "Ignore All" or "Ignore Most" mode, we pretty much did this already. This mostly just restores a bit of the code from D20185, with some adjustments/simplifications. Here's how it works: - Rebuild the text of the old and new files from the diff we got out of `arc`, `git diff`, etc. - Normalize the files (for example, by removing whitespace from each line). - Diff the normalized files to produce a second diff. - Parse that diff. - Take the "alignment" from the normalized diff (whitespace removed) and the actual text from the original diff (whitespace preserved) to build a new diff with the correct text, but also better diff alignment. Originally, we normalized with `diff -bw`. I've replaced that with `preg_replace()` here mostly just so that we have more control over things. I believe the two behaviors are pretty much identical, but this way lets us see more of the pipeline and possibly add more behaviors in the future to improve diff quality (e.g., normalize case? normalize text encoding?). Test Plan: {F6217133} (Also, fix a unit test.) Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13161, T6791 Differential Revision: https://secure.phabricator.com/D20187 --- .../DifferentialParseRenderTestCase.php | 4 ++ .../__tests__/data/generated.diff | 2 +- .../parser/DifferentialChangesetParser.php | 50 +++++++++++++++ .../parser/DifferentialHunkParser.php | 63 +++++++++++++++++++ .../diff/PhabricatorDifferenceEngine.php | 40 ++++++++++++ 5 files changed, 158 insertions(+), 1 deletion(-) diff --git a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php b/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php index 259bb671de..ce93bbe65a 100644 --- a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php +++ b/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php @@ -14,6 +14,10 @@ final class DifferentialParseRenderTestCase extends PhabricatorTestCase { } $data = Filesystem::readFile($dir.$file); + // Strip trailing "~" characters from inputs so they may contain + // trailing whitespace. + $data = preg_replace('/~$/m', '', $data); + $opt_file = $dir.$file.'.options'; if (Filesystem::pathExists($opt_file)) { $options = Filesystem::readFile($opt_file); diff --git a/src/applications/differential/__tests__/data/generated.diff b/src/applications/differential/__tests__/data/generated.diff index 7846c9a494..c130993cf7 100644 --- a/src/applications/differential/__tests__/data/generated.diff +++ b/src/applications/differential/__tests__/data/generated.diff @@ -4,7 +4,7 @@ index 5dcff7f..eff82ef 100644 +++ b/GENERATED @@ -1,4 +1,4 @@ @generated - + ~ -This is a generated file. +This is a generated file, full of generated code. diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index caa7463672..f0f952328a 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -643,6 +643,9 @@ final class DifferentialChangesetParser extends Phobject { $hunk_parser = new DifferentialHunkParser(); $hunk_parser->parseHunksForLineData($changeset->getHunks()); + + $this->realignDiff($changeset, $hunk_parser); + $hunk_parser->reparseHunksForSpecialAttributes(); $unchanged = false; @@ -1366,4 +1369,51 @@ final class DifferentialChangesetParser extends Phobject { return $key; } + private function realignDiff( + DifferentialChangeset $changeset, + DifferentialHunkParser $hunk_parser) { + // Normalizing and realigning the diff depends on rediffing the files, and + // we currently need complete representations of both files to do anything + // reasonable. If we only have parts of the files, skip realignment. + + // We have more than one hunk, so we're definitely missing part of the file. + $hunks = $changeset->getHunks(); + if (count($hunks) !== 1) { + return null; + } + + // The first hunk doesn't start at the beginning of the file, so we're + // missing some context. + $first_hunk = head($hunks); + if ($first_hunk->getOldOffset() != 1 || $first_hunk->getNewOffset() != 1) { + return null; + } + + $old_file = $changeset->makeOldFile(); + $new_file = $changeset->makeNewFile(); + if ($old_file === $new_file) { + // If the old and new files are exactly identical, the synthetic + // diff below will give us nonsense and whitespace modes are + // irrelevant anyway. This occurs when you, e.g., copy a file onto + // itself in Subversion (see T271). + return null; + } + + + $engine = id(new PhabricatorDifferenceEngine()) + ->setNormalize(true); + + $normalized_changeset = $engine->generateChangesetFromFileContent( + $old_file, + $new_file); + + $type_parser = new DifferentialHunkParser(); + $type_parser->parseHunksForLineData($normalized_changeset->getHunks()); + + $hunk_parser->setNormalized(true); + $hunk_parser->setOldLineTypeMap($type_parser->getOldLineTypeMap()); + $hunk_parser->setNewLineTypeMap($type_parser->getNewLineTypeMap()); + } + + } diff --git a/src/applications/differential/parser/DifferentialHunkParser.php b/src/applications/differential/parser/DifferentialHunkParser.php index d89089a7e7..d59358bc82 100644 --- a/src/applications/differential/parser/DifferentialHunkParser.php +++ b/src/applications/differential/parser/DifferentialHunkParser.php @@ -7,6 +7,7 @@ final class DifferentialHunkParser extends Phobject { private $intraLineDiffs; private $depthOnlyLines; private $visibleLinesMask; + private $normalized; /** * Get a map of lines on which hunks start, other than line 1. This @@ -124,6 +125,15 @@ final class DifferentialHunkParser extends Phobject { return $this->depthOnlyLines; } + public function setNormalized($normalized) { + $this->normalized = $normalized; + return $this; + } + + public function getNormalized() { + return $this->normalized; + } + public function getIsDeleted() { foreach ($this->getNewLines() as $line) { if ($line) { @@ -252,6 +262,8 @@ final class DifferentialHunkParser extends Phobject { $this->setOldLines($rebuild_old); $this->setNewLines($rebuild_new); + $this->updateChangeTypesForNormalization(); + return $this; } @@ -753,4 +765,55 @@ final class DifferentialHunkParser extends Phobject { return $character_depth; } + private function updateChangeTypesForNormalization() { + if (!$this->getNormalized()) { + return; + } + + // If we've parsed based on a normalized diff alignment, we may currently + // believe some lines are unchanged when they have actually changed. This + // happens when: + // + // - a line changes; + // - the change is a kind of change we normalize away when aligning the + // diff, like an indentation change; + // - we normalize the change away to align the diff; and so + // - the old and new copies of the line are now aligned in the new + // normalized diff. + // + // Then we end up with an alignment where the two lines that differ only + // in some some trivial way are aligned. This is great, and exactly what + // we're trying to accomplish by doing all this alignment stuff in the + // first place. + // + // However, in this case the correctly-aligned lines will be incorrectly + // marked as unchanged because the diff alorithm was fed normalized copies + // of the lines, and these copies truly weren't any different. + // + // When lines are aligned and marked identical, but they're not actually + // identcal, we now mark them as changed. The rest of the processing will + // figure out how to render them appropritely. + + $new = $this->getNewLines(); + $old = $this->getOldLines(); + foreach ($old as $key => $o) { + $n = $new[$key]; + + if (!$o || !$n) { + continue; + } + + if ($o['type'] === null && $n['type'] === null) { + if ($o['text'] !== $n['text']) { + $old[$key]['type'] = '-'; + $new[$key]['type'] = '+'; + } + } + } + + $this->setOldLines($old); + $this->setNewLines($new); + } + + } diff --git a/src/infrastructure/diff/PhabricatorDifferenceEngine.php b/src/infrastructure/diff/PhabricatorDifferenceEngine.php index 90f1d9b10e..84e88ceaaa 100644 --- a/src/infrastructure/diff/PhabricatorDifferenceEngine.php +++ b/src/infrastructure/diff/PhabricatorDifferenceEngine.php @@ -12,6 +12,7 @@ final class PhabricatorDifferenceEngine extends Phobject { private $oldName; private $newName; + private $normalize; /* -( Configuring the Engine )--------------------------------------------- */ @@ -43,6 +44,16 @@ final class PhabricatorDifferenceEngine extends Phobject { } + public function setNormalize($normalize) { + $this->normalize = $normalize; + return $this; + } + + public function getNormalize() { + return $this->normalize; + } + + /* -( Generating Diffs )--------------------------------------------------- */ @@ -71,6 +82,12 @@ final class PhabricatorDifferenceEngine extends Phobject { $options[] = '-L'; $options[] = $new_name; + $normalize = $this->getNormalize(); + if ($normalize) { + $old = $this->normalizeFile($old); + $new = $this->normalizeFile($new); + } + $old_tmp = new TempFile(); $new_tmp = new TempFile(); @@ -129,4 +146,27 @@ final class PhabricatorDifferenceEngine extends Phobject { return head($diff->getChangesets()); } + private function normalizeFile($corpus) { + // We can freely apply any other transformations we want to here: we have + // no constraints on what we need to preserve. If we normalize every line + // to "cat", the diff will still work, the alignment of the "-" / "+" + // lines will just be very hard to read. + + // In general, we'll make the diff better if we normalize two lines that + // humans think are the same. + + // We'll make the diff worse if we normalize two lines that humans think + // are different. + + + // Strip all whitespace present anywhere in the diff, since humans never + // consider whitespace changes to alter the line into a "different line" + // even when they're semantic (e.g., in a string constant). This covers + // indentation changes, trailing whitepspace, and formatting changes + // like "+/-". + $corpus = preg_replace('/[ \t]/', '', $corpus); + + return $corpus; + } + } From 98fe8fae4a9001c17af2e625c64760da55618a49 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 16 Feb 2019 09:50:20 -0800 Subject: [PATCH 17/30] Use `` instead of `3` for line numbers Summary: Ref T13161. Ref T12822. See PHI870. Long ago, the web was simple. You could leave your doors unlocked, you knew all your neighbors, crime hadn't been invented yet, and `3` was a perfectly fine way to render a line number cell containing the number "3". But times have changed! - In PHI870, this isn't good for screenreaders. We can't do much about this, so switch to ``. - In D19349 / T13105 and elsewhere, this `::after { content: attr(data-n); }` approach seems like the least bad general-purpose approach for preventing line numbers from being copied. Although Differential needs even more magic beyond this in the two-up view, this is likely good enough for the one-up view, and is consistent with other views (paste, harbormaster logs, general source display) where this technique is sufficient on its own. The chance this breaks //something// is pretty much 100%, but we've got a week to figure out what it breaks. I couldn't find any issues immediately. Test Plan: - Created, edited, deleted inlines in 1-up and 2-up views. - Replied, keyboard-navigated, keyboard-replied, drag-selected, poked and prodded everything. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13161, T12822 Differential Revision: https://secure.phabricator.com/D20188 --- resources/celerity/map.php | 26 +++++----- .../DifferentialChangesetOneUpRenderer.php | 36 +++++++++----- .../DifferentialChangesetTwoUpRenderer.php | 20 +++++++- .../PHUIDiffOneUpInlineCommentRowScaffold.php | 4 +- .../PHUIDiffTwoUpInlineCommentRowScaffold.php | 4 +- .../differential/changeset-view.css | 49 +++++++++++-------- .../js/application/diff/DiffChangesetList.js | 12 ++--- 7 files changed, 92 insertions(+), 59 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 65eed7916a..441f7108d1 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,8 +11,8 @@ return array( 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '261ee8cf', 'core.pkg.js' => '5ace8a1e', - 'differential.pkg.css' => 'c3f15714', - 'differential.pkg.js' => 'be031567', + 'differential.pkg.css' => '801c5653', + 'differential.pkg.js' => '1f211736', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', 'maniphest.pkg.css' => '35995d6d', @@ -61,7 +61,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '4267d6c6', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', - 'rsrc/css/application/differential/changeset-view.css' => '783a9206', + 'rsrc/css/application/differential/changeset-view.css' => '8a997ed9', 'rsrc/css/application/differential/core.css' => 'bdb93065', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', @@ -375,7 +375,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '9b1cbd76', 'rsrc/js/application/diff/DiffChangeset.js' => 'd0a85a85', - 'rsrc/js/application/diff/DiffChangesetList.js' => 'b91204e9', + 'rsrc/js/application/diff/DiffChangesetList.js' => '26fb79ba', 'rsrc/js/application/diff/DiffInline.js' => 'a4a14a94', 'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17', 'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd', @@ -541,7 +541,7 @@ return array( 'conpherence-thread-manager' => 'aec8e38c', 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => 'd67475f5', - 'differential-changeset-view-css' => '783a9206', + 'differential-changeset-view-css' => '8a997ed9', 'differential-core-view-css' => 'bdb93065', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', @@ -754,7 +754,7 @@ return array( 'phabricator-darkmessage' => '26cd4b73', 'phabricator-dashboard-css' => '4267d6c6', 'phabricator-diff-changeset' => 'd0a85a85', - 'phabricator-diff-changeset-list' => 'b91204e9', + 'phabricator-diff-changeset-list' => '26fb79ba', 'phabricator-diff-inline' => 'a4a14a94', 'phabricator-drag-and-drop-file-upload' => '4370900d', 'phabricator-draggable-list' => '3c6bd549', @@ -1087,6 +1087,10 @@ return array( 'javelin-json', 'phabricator-draggable-list', ), + '26fb79ba' => array( + 'javelin-install', + 'phuix-button-view', + ), '27daef73' => array( 'multirow-row-manager', 'javelin-install', @@ -1513,9 +1517,6 @@ return array( 'javelin-uri', 'javelin-request', ), - '783a9206' => array( - 'phui-inline-comment-view-css', - ), '78bc5d94' => array( 'javelin-behavior', 'javelin-uri', @@ -1586,6 +1587,9 @@ return array( '8a16f91b' => array( 'syntax-default-css', ), + '8a997ed9' => array( + 'phui-inline-comment-view-css', + ), '8ac32fd9' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1895,10 +1899,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - 'b91204e9' => array( - 'javelin-install', - 'phuix-button-view', - ), 'bd546a49' => array( 'phui-workcard-view-css', ), diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php index 90c3977907..289b802485 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -92,19 +92,23 @@ final class DifferentialChangesetOneUpRenderer $line = $p['line']; $cells[] = phutil_tag( - 'th', + 'td', array( 'id' => $left_id, - 'class' => $class, - ), - $line); + 'class' => $class.' n', + 'data-n' => $line, + )); $render = $p['render']; if ($aural !== null) { $render = array($aural, $render); } - $cells[] = phutil_tag('th', array('class' => $class)); + $cells[] = phutil_tag( + 'td', + array( + 'class' => $class.' n', + )); $cells[] = $no_copy; $cells[] = phutil_tag('td', array('class' => $class), $render); $cells[] = $no_coverage; @@ -115,7 +119,11 @@ final class DifferentialChangesetOneUpRenderer } else { $class = 'right new'; } - $cells[] = phutil_tag('th', array('class' => $class)); + $cells[] = phutil_tag( + 'td', + array( + 'class' => $class.' n', + )); $aural = $aural_plus; } else { $class = 'right'; @@ -127,7 +135,13 @@ final class DifferentialChangesetOneUpRenderer $oline = $p['oline']; - $cells[] = phutil_tag('th', array('id' => $left_id), $oline); + $cells[] = phutil_tag( + 'td', + array( + 'id' => $left_id, + 'class' => 'n', + 'data-n' => $oline, + )); $aural = null; } @@ -144,12 +158,12 @@ final class DifferentialChangesetOneUpRenderer $line = $p['line']; $cells[] = phutil_tag( - 'th', + 'td', array( 'id' => $right_id, - 'class' => $class, - ), - $line); + 'class' => $class.' n', + 'data-n' => $line, + )); $render = $p['render']; if ($aural !== null) { diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index b4936201e0..baf08aac77 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -306,10 +306,26 @@ final class DifferentialChangesetTwoUpRenderer // clipboard. See the 'phabricator-oncopy' behavior. $zero_space = "\xE2\x80\x8B"; + $old_number = phutil_tag( + 'td', + array( + 'id' => $o_id, + 'class' => $o_classes.' n', + 'data-n' => $o_num, + )); + + $new_number = phutil_tag( + 'td', + array( + 'id' => $n_id, + 'class' => $n_classes.' n', + 'data-n' => $n_num, + )); + $html[] = phutil_tag('tr', array(), array( - phutil_tag('th', array('id' => $o_id, 'class' => $o_classes), $o_num), + $old_number, phutil_tag('td', array('class' => $o_classes), $o_text), - phutil_tag('th', array('id' => $n_id, 'class' => $n_classes), $n_num), + $new_number, $n_copy, phutil_tag( 'td', diff --git a/src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php b/src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php index 53c2255dc8..1f8e05bc27 100644 --- a/src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php +++ b/src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php @@ -31,8 +31,8 @@ final class PHUIDiffOneUpInlineCommentRowScaffold } $cells = array( - phutil_tag('th', array(), $left_hidden), - phutil_tag('th', array(), $right_hidden), + phutil_tag('td', array('class' => 'n'), $left_hidden), + phutil_tag('td', array('class' => 'n'), $right_hidden), phutil_tag('td', $attrs, $inline), ); diff --git a/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php b/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php index 81b0edaf49..769ad84d1f 100644 --- a/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php +++ b/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php @@ -71,9 +71,9 @@ final class PHUIDiffTwoUpInlineCommentRowScaffold ); $cells = array( - phutil_tag('th', array(), $left_hidden), + phutil_tag('td', array('class' => 'n'), $left_hidden), phutil_tag('td', $left_attrs, $left_side), - phutil_tag('th', array(), $right_hidden), + phutil_tag('td', array('class' => 'n'), $right_hidden), phutil_tag('td', $right_attrs, $right_side), ); diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 2cfa753a42..a49dd4905b 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -72,23 +72,6 @@ width: 0; } -.differential-diff th { - text-align: right; - padding: 1px 6px 1px 0; - vertical-align: top; - background: {$lightbluebackground}; - color: {$bluetext}; - cursor: pointer; - border-right: 1px solid {$thinblueborder}; - overflow: hidden; - - -moz-user-select: -moz-none; - -khtml-user-select: none; - -webkit-user-select: none; - -ms-user-select: none; - user-select: none; -} - .prose-diff { padding: 12px 0; white-space: pre-wrap; @@ -182,6 +165,34 @@ background: #dddddd; } +.differential-diff .inline > td { + padding: 0; +} + +/* Specify line number behaviors after other behaviors because line numbers +should always have a boring grey background. */ + +.differential-diff td.n { + text-align: right; + padding: 1px 6px 1px 0; + vertical-align: top; + background: {$lightbluebackground}; + color: {$bluetext}; + cursor: pointer; + border-right: 1px solid {$thinblueborder}; + overflow: hidden; + + -moz-user-select: -moz-none; + -khtml-user-select: none; + -webkit-user-select: none; + -ms-user-select: none; + user-select: none; +} + +.differential-diff td.n::before { + content: attr(data-n); +} + .differential-diff td.cov { padding: 0; } @@ -316,10 +327,6 @@ td.cov-I { pointer-events: none; } -.differential-diff .inline > td { - padding: 0; -} - .differential-loading { border-top: 1px solid {$gentle.highlight.border}; border-bottom: 1px solid {$gentle.highlight.border}; diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 5ba43a7e6d..8a1306967a 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -70,13 +70,13 @@ JX.install('DiffChangesetList', { var onrangedown = JX.bind(this, this._ifawake, this._onrangedown); JX.Stratcom.listen( 'mousedown', - ['differential-changeset', 'tag:th'], + ['differential-changeset', 'tag:td'], onrangedown); var onrangemove = JX.bind(this, this._ifawake, this._onrangemove); JX.Stratcom.listen( ['mouseover', 'mouseout'], - ['differential-changeset', 'tag:th'], + ['differential-changeset', 'tag:td'], onrangemove); var onrangeup = JX.bind(this, this._ifawake, this._onrangeup); @@ -360,7 +360,7 @@ JX.install('DiffChangesetList', { while (row) { var header = row.firstChild; while (header) { - if (JX.DOM.isType(header, 'th')) { + if (this.getLineNumberFromHeader(header)) { if (header.className.indexOf('old') !== -1) { old_list.push(header); } else if (header.className.indexOf('new') !== -1) { @@ -1247,11 +1247,7 @@ JX.install('DiffChangesetList', { }, getLineNumberFromHeader: function(th) { - try { - return parseInt(th.id.match(/^C\d+[ON]L(\d+)$/)[1], 10); - } catch (x) { - return null; - } + return parseInt(th.getAttribute('data-n')); }, getDisplaySideFromHeader: function(th) { From d4b96bcf6b017678cbf5f966ba7a9ab9a458969f Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 17 Feb 2019 04:06:27 -0800 Subject: [PATCH 18/30] Remove hidden zero-width spaces affecting copy behavior Summary: Ref T13161. Ref T12822. Today, we use invisible Zero-Width Spaces to try to improve copy/paste behavior from Differential. After D20188, we no longer need ZWS characters to avoid copying line numbers. Get rid of these secret invisible semantic ZWS characters completely. This means that both the left-hand and right-hand side of diffs become copyable, which isn't desired. I'll fix that with a hundred thousand lines of Javascript in the next change: this is a step toward everything working better, but doesn't fix everything yet. Test Plan: - Grepped for `zws`, `grep -i zero | grep -i width`. - Copied text out of Differential: got both sides of the diff (not ideal). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13161, T12822 Differential Revision: https://secure.phabricator.com/D20189 --- resources/celerity/map.php | 30 +++++++++---------- .../DifferentialChangesetTwoUpRenderer.php | 10 +------ .../differential/changeset-view.css | 5 ---- .../repository/repository-crossreference.js | 5 ---- 4 files changed, 16 insertions(+), 34 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 441f7108d1..6b13a2dace 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,8 +11,8 @@ return array( 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '261ee8cf', 'core.pkg.js' => '5ace8a1e', - 'differential.pkg.css' => '801c5653', - 'differential.pkg.js' => '1f211736', + 'differential.pkg.css' => 'fcc82bc0', + 'differential.pkg.js' => '0e2b0e2c', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', 'maniphest.pkg.css' => '35995d6d', @@ -61,7 +61,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '4267d6c6', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', - 'rsrc/css/application/differential/changeset-view.css' => '8a997ed9', + 'rsrc/css/application/differential/changeset-view.css' => '58236820', 'rsrc/css/application/differential/core.css' => 'bdb93065', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', @@ -420,7 +420,7 @@ return array( 'rsrc/js/application/releeph/releeph-preview-branch.js' => '75184d68', 'rsrc/js/application/releeph/releeph-request-state-change.js' => '9f081f05', 'rsrc/js/application/releeph/releeph-request-typeahead.js' => 'aa3a100c', - 'rsrc/js/application/repository/repository-crossreference.js' => 'db0c0214', + 'rsrc/js/application/repository/repository-crossreference.js' => 'c15122b4', 'rsrc/js/application/search/behavior-reorder-profile-menu-items.js' => 'e5bdb730', 'rsrc/js/application/search/behavior-reorder-queries.js' => 'b86f297f', 'rsrc/js/application/transactions/behavior-comment-actions.js' => '4dffaeb2', @@ -541,7 +541,7 @@ return array( 'conpherence-thread-manager' => 'aec8e38c', 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => 'd67475f5', - 'differential-changeset-view-css' => '8a997ed9', + 'differential-changeset-view-css' => '58236820', 'differential-core-view-css' => 'bdb93065', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', @@ -671,7 +671,7 @@ return array( 'javelin-behavior-reorder-applications' => 'aa371860', 'javelin-behavior-reorder-columns' => '8ac32fd9', 'javelin-behavior-reorder-profile-menu-items' => 'e5bdb730', - 'javelin-behavior-repository-crossreference' => 'db0c0214', + 'javelin-behavior-repository-crossreference' => 'c15122b4', 'javelin-behavior-scrollbar' => '92388bae', 'javelin-behavior-search-reorder-queries' => 'b86f297f', 'javelin-behavior-select-content' => 'e8240b50', @@ -1380,6 +1380,9 @@ return array( 'javelin-vector', 'javelin-typeahead-static-source', ), + 58236820 => array( + 'phui-inline-comment-view-css', + ), '5902260c' => array( 'javelin-util', 'javelin-magical-init', @@ -1587,9 +1590,6 @@ return array( '8a16f91b' => array( 'syntax-default-css', ), - '8a997ed9' => array( - 'phui-inline-comment-view-css', - ), '8ac32fd9' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1912,6 +1912,12 @@ return array( 'c03f2fb4' => array( 'javelin-install', ), + 'c15122b4' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-uri', + ), 'c2c500a7' => array( 'javelin-install', 'javelin-dom', @@ -2008,12 +2014,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - 'db0c0214' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-uri', - ), 'dfa1d313' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index baf08aac77..d975d82522 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -301,11 +301,6 @@ final class DifferentialChangesetTwoUpRenderer } } - // NOTE: This is a unicode zero-width space, which we use as a hint when - // intercepting 'copy' events to make sure sensible text ends up on the - // clipboard. See the 'phabricator-oncopy' behavior. - $zero_space = "\xE2\x80\x8B"; - $old_number = phutil_tag( 'td', array( @@ -330,10 +325,7 @@ final class DifferentialChangesetTwoUpRenderer phutil_tag( 'td', array('class' => $n_classes, 'colspan' => $n_colspan), - array( - phutil_tag('span', array('class' => 'zwsp'), $zero_space), - $n_text, - )), + $n_text), $n_cov, )); diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index a49dd4905b..59faa8399e 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -67,11 +67,6 @@ padding: 1px 4px; } -.differential-diff td .zwsp { - position: absolute; - width: 0; -} - .prose-diff { padding: 12px 0; white-space: pre-wrap; diff --git a/webroot/rsrc/js/application/repository/repository-crossreference.js b/webroot/rsrc/js/application/repository/repository-crossreference.js index 548ef6173b..d6ff2a06aa 100644 --- a/webroot/rsrc/js/application/repository/repository-crossreference.js +++ b/webroot/rsrc/js/application/repository/repository-crossreference.js @@ -237,11 +237,6 @@ JX.behavior('repository-crossreference', function(config, statics) { } var content = '' + node.textContent; - - // Strip off any ZWS characters. These are marker characters used to - // improve copy/paste behavior. - content = content.replace(/\u200B/g, ''); - char += content.length; } From 3ded5d3e8ca8717ac04cbee179a9e839ec5d7b6c Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 17 Feb 2019 06:24:08 -0800 Subject: [PATCH 19/30] Disable the JSHint "function called before it is defined" and "unused parameter" warnings Summary: Ref T12822. The next change hits these warnings but I think neither is a net positive. The "function called before it is defined" error alerts on this kind of thing: ``` function a() { b(); } function b() { } a(); ``` Here, `b()` is called before it is defined. This code, as written, is completely safe. Although it's possible that this kind of construct may be unsafe, I think the number of programs where there's unsafe behavior here AND the whole thing doesn't immediately break when you run it at all is very very small. Complying with this warning is sometimes impossible -- at least without cheating/restructuring/abuse -- for example, if you have two functions which are mutually recursive. Although compliance is usually possible, it forces you to define all your small utility functions at the top of a behavior. This isn't always the most logical or comprehensible order. I think we also have some older code which did `var a = function() { ... }` to escape this, which I think is just silly/confusing. Bascially, this is almost always a false positive and I think it makes the code worse more often than it makes it better. --- The "unused function parameter" error warns about this: ``` function onevent(e) { do_something(); ``` We aren't using `e`, so this warning is correct. However, when the function is a callback (as here), I think it's generally good hygiene to include the callback parameters in the signature (`onresponse(response)`, `onevent(event)`, etc), even if you aren't using any/all of them. This is a useful hint to future editors that the function is a callback. Although this //can// catch mistakes, I think this is also a situation where the number of cases where it catches a mistake and even the most cursory execution of the code doesn't catch the mistake is vanishingly small. Test Plan: Egregiously violated both rules in the next diff. Before change: complaints. After change: no complaints. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12822 Differential Revision: https://secure.phabricator.com/D20190 --- support/lint/browser.jshintrc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/support/lint/browser.jshintrc b/support/lint/browser.jshintrc index b88c931eee..2a9c65bdd2 100644 --- a/support/lint/browser.jshintrc +++ b/support/lint/browser.jshintrc @@ -4,12 +4,12 @@ "freeze": true, "immed": true, "indent": 2, - "latedef": true, + "latedef": "nofunc", "newcap": true, "noarg": true, "quotmark": "single", "undef": true, - "unused": true, + "unused": "vars", "expr": true, "loopfunc": true, From 37f12a05ea621788f83a97ef5ab7ed9ffd7c1aaa Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 16 Feb 2019 09:32:13 -0800 Subject: [PATCH 20/30] Behold! Copy text from either side of a diff! MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Ref T12822. Ref T13161. By default, when users select text from a diff and copy it to the clipboard, they get both sides of the diff and all the line numbers. This is usually not what they intended to copy. As of D20188, we use `content: attr(...)` to render line numbers. No browser copies this text, so that fixes line numbers. We can use "user-select" CSS to visually prevent selection of line numbers and other stuff we don't want to copy. In Firefox and Chrome, "user-select" also applies to copied text, so getting "user-select" on the right nodes is largely good enough to do what we want. In Safari, "user-select" is only visual, so we always need to crawl the DOM to figure out what text to pull out of it anyway. In all browsers, we likely want to crawl the DOM anyway because this will let us show one piece of text and copy a different piece of text. We probably want to do this in the future to preserve "\t" tabs, and possibly to let us render certain character codes in one way but copy their original values. For example, we could render "\x07" as "␇". Finally, we have to figure out which side of the diff we're copying from. The rule here is: - If you start the selection by clicking somewhere on the left or right side of the diff, that's what you're copying. - Otherwise, use normal document copy rules. So the overall flow here is: - Listen for clicks. - When the user clicks the left or right side of the diff, store what they clicked. - When a selection starts, and something is actually selected, check if it was initiated by clicking a diff. If it was, apply a visual effect to get "user-select" where it needs to go and show the user what we think they're doing and what we're going to copy. - (Then, try to handle a bunch of degenerate cases where you start a selection and then click inside that selection.) - When a user clicks elsewhere or ends the selection with nothing selected, clear the selection mode. - When a user copies text, if we have an active selection mode, pull all the selected nodes out of the DOM and filter out the ones we don't want to copy, then stitch the text back together. Although I believe this didn't work well in ~2010, it appears to work well today. Test Plan: This mostly seems to work in Safari, Chrome, and Firefox. T12822 has some errata. I haven't tested touch events but am satisfied if the touch event story is anything better than "permanently destroys data". Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13161, T12822 Differential Revision: https://secure.phabricator.com/D20191 --- resources/celerity/map.php | 26 +- .../DifferentialChangesetHTMLRenderer.php | 2 +- .../DifferentialChangesetTwoUpRenderer.php | 14 +- .../differential/changeset-view.css | 36 ++- webroot/rsrc/js/core/behavior-oncopy.js | 299 +++++++++++++++--- 5 files changed, 309 insertions(+), 68 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 6b13a2dace..086cb1b8b3 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,8 +10,8 @@ return array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '261ee8cf', - 'core.pkg.js' => '5ace8a1e', - 'differential.pkg.css' => 'fcc82bc0', + 'core.pkg.js' => '5ba0b6d7', + 'differential.pkg.css' => 'd1b29c9c', 'differential.pkg.js' => '0e2b0e2c', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', @@ -61,7 +61,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '4267d6c6', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', - 'rsrc/css/application/differential/changeset-view.css' => '58236820', + 'rsrc/css/application/differential/changeset-view.css' => 'e2b81e85', 'rsrc/css/application/differential/core.css' => 'bdb93065', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', @@ -473,7 +473,7 @@ return array( 'rsrc/js/core/behavior-linked-container.js' => '74446546', 'rsrc/js/core/behavior-more.js' => '506aa3f4', 'rsrc/js/core/behavior-object-selector.js' => 'a4af0b4a', - 'rsrc/js/core/behavior-oncopy.js' => '418f6684', + 'rsrc/js/core/behavior-oncopy.js' => 'f20d66c1', 'rsrc/js/core/behavior-phabricator-nav.js' => 'f166c949', 'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => '2f80333f', 'rsrc/js/core/behavior-read-only-warning.js' => 'b9109f8f', @@ -541,7 +541,7 @@ return array( 'conpherence-thread-manager' => 'aec8e38c', 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => 'd67475f5', - 'differential-changeset-view-css' => '58236820', + 'differential-changeset-view-css' => 'e2b81e85', 'differential-core-view-css' => 'bdb93065', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', @@ -636,7 +636,7 @@ return array( 'javelin-behavior-phabricator-nav' => 'f166c949', 'javelin-behavior-phabricator-notification-example' => '29819b75', 'javelin-behavior-phabricator-object-selector' => 'a4af0b4a', - 'javelin-behavior-phabricator-oncopy' => '418f6684', + 'javelin-behavior-phabricator-oncopy' => 'f20d66c1', 'javelin-behavior-phabricator-remarkup-assist' => '2f80333f', 'javelin-behavior-phabricator-reveal-content' => 'b105a3a6', 'javelin-behavior-phabricator-search-typeahead' => '1cb7d027', @@ -1222,10 +1222,6 @@ return array( 'javelin-behavior', 'javelin-uri', ), - '418f6684' => array( - 'javelin-behavior', - 'javelin-dom', - ), '42c7a5a7' => array( 'javelin-install', 'javelin-dom', @@ -1380,9 +1376,6 @@ return array( 'javelin-vector', 'javelin-typeahead-static-source', ), - 58236820 => array( - 'phui-inline-comment-view-css', - ), '5902260c' => array( 'javelin-util', 'javelin-magical-init', @@ -2039,6 +2032,9 @@ return array( 'javelin-dom', 'javelin-stratcom', ), + 'e2b81e85' => array( + 'phui-inline-comment-view-css', + ), 'e562708c' => array( 'javelin-install', ), @@ -2090,6 +2086,10 @@ return array( 'javelin-request', 'javelin-util', ), + 'f20d66c1' => array( + 'javelin-behavior', + 'javelin-dom', + ), 'f340a484' => array( 'javelin-install', 'javelin-dom', diff --git a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php index 2e760d99bb..e4320b712b 100644 --- a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php @@ -436,7 +436,7 @@ abstract class DifferentialChangesetHTMLRenderer 'table', array( 'class' => implode(' ', $classes), - 'sigil' => 'differential-diff', + 'sigil' => 'differential-diff intercept-copy', ), array( $this->renderColgroup(), diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index d975d82522..290272db49 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -319,12 +319,22 @@ final class DifferentialChangesetTwoUpRenderer $html[] = phutil_tag('tr', array(), array( $old_number, - phutil_tag('td', array('class' => $o_classes), $o_text), + phutil_tag( + 'td', + array( + 'class' => $o_classes, + 'data-copy-mode' => 'copy-l', + ), + $o_text), $new_number, $n_copy, phutil_tag( 'td', - array('class' => $n_classes, 'colspan' => $n_colspan), + array( + 'class' => $n_classes, + 'colspan' => $n_colspan, + 'data-copy-mode' => 'copy-r', + ), $n_text), $n_cov, )); diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 59faa8399e..bc1ca8b6ea 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -176,12 +176,6 @@ should always have a boring grey background. */ cursor: pointer; border-right: 1px solid {$thinblueborder}; overflow: hidden; - - -moz-user-select: -moz-none; - -khtml-user-select: none; - -webkit-user-select: none; - -ms-user-select: none; - user-select: none; } .differential-diff td.n::before { @@ -430,3 +424,33 @@ tr.differential-inline-loading { .diff-banner-buttons { float: right; } + +/* In Firefox, making the table unselectable and then making cells selectable +does not work: the cells remain unselectable. Narrowly mark the cells as +unselectable. */ + +.differential-diff.copy-l > tbody > tr > td, +.differential-diff.copy-r > tbody > tr > td { + -moz-user-select: -moz-none; + -khtml-user-select: none; + -ms-user-select: none; + -webkit-user-select: none; + user-select: none; +} + +.differential-diff.copy-l > tbody > tr > td, +.differential-diff.copy-r > tbody > tr > td { + opacity: 0.5; +} + +.differential-diff.copy-l > tbody > tr > td:nth-child(2) { + -webkit-user-select: auto; + user-select: auto; + opacity: 1; +} + +.differential-diff.copy-r > tbody > tr > td:nth-child(5) { + -webkit-user-select: auto; + user-select: auto; + opacity: 1; +} diff --git a/webroot/rsrc/js/core/behavior-oncopy.js b/webroot/rsrc/js/core/behavior-oncopy.js index aa8c684fee..a37474abf9 100644 --- a/webroot/rsrc/js/core/behavior-oncopy.js +++ b/webroot/rsrc/js/core/behavior-oncopy.js @@ -4,62 +4,269 @@ * javelin-dom */ -/** - * Tools like Paste and Differential don't normally respond to the clipboard - * 'copy' operation well, because when a user copies text they'll get line - * numbers and other metadata. - * - * To improve this behavior, applications can embed markers that delimit - * metadata (left of the marker) from content (right of the marker). When - * we get a copy event, we strip out all the metadata and just copy the - * actual text. - */ JX.behavior('phabricator-oncopy', function() { + var copy_root; + var copy_mode; - var zws = '\u200B'; // Unicode Zero-Width Space + function onstartselect(e) { + var target = e.getTarget(); - JX.enableDispatch(document.body, 'copy'); - JX.Stratcom.listen( - ['copy'], - null, - function(e) { + var container; + try { + // NOTE: For now, all elements with custom oncopy behavior are tables, + // so this tag selection will hit everything we need it to. + container = JX.DOM.findAbove(target, 'table', 'intercept-copy'); + } catch (ex) { + container = null; + } - var selection; - var text; - if (window.getSelection) { - selection = window.getSelection(); - text = selection.toString(); - } else { - selection = document.selection; - text = selection.createRange().text; - } + var old_mode = copy_mode; + clear_selection_mode(); - if (text.indexOf(zws) == -1) { - // If there's no marker in the text, just let it copy normally. + if (!container) { + return; + } + + // If the potential selection is starting inside an inline comment, + // don't do anything special. + try { + if (JX.DOM.findAbove(target, 'div', 'differential-inline-comment')) { return; } + } catch (ex) { + // Continue. + } - var result = []; - - // Strip everything before the marker (and the marker itself) out of the - // text. If a line doesn't have the marker, throw it away (the assumption - // is that it's a line number or part of some other meta-text). - var lines = text.split('\n'); - var pos; - for (var ii = 0; ii < lines.length; ii++) { - pos = lines[ii].indexOf(zws); - if (pos == -1 && ii !== 0) { - continue; - } - result.push(lines[ii].substring(pos + 1)); + // Find the row and cell we're copying from. If we don't find anything, + // don't do anything special. + var row; + var cell; + try { + // The target may be the cell we're after, particularly if you click + // in the white area to the right of the text, towards the end of a line. + if (JX.DOM.isType(target, 'td')) { + cell = target; + } else { + cell = JX.DOM.findAbove(target, 'td'); } - result = result.join('\n'); + row = JX.DOM.findAbove(target, 'tr'); + } catch (ex) { + return; + } + + // If the row doesn't have enough nodes, bail out. Note that it's okay + // to begin a selection in the whitespace on the opposite side of an inline + // comment. For example, if there's an inline comment on the right side of + // a diff, it's okay to start selecting the left side of the diff by + // clicking the corresponding empty space on the left side. + if (row.childNodes.length < 4) { + return; + } + + // If the selection's cell is in the "old" diff or the "new" diff, we'll + // activate an appropriate copy mode. + var mode; + if (cell === row.childNodes[1]) { + mode = 'copy-l'; + } else if ((row.childNodes.length >= 4) && (cell === row.childNodes[4])) { + mode = 'copy-r'; + } else { + return; + } + + // We found a copy mode, so set it as the current active mode. + copy_root = container; + copy_mode = mode; + + // If the user makes a selection, then clicks again inside the same + // selection, browsers retain the selection. This is because the user may + // want to drag-and-drop the text to another window. + + // Handle special cases when the click is inside an existing selection. + + var ranges = get_selected_ranges(); + if (ranges.length) { + // We'll have an existing selection if the user selects text on the right + // side of a diff, then clicks the selection on the left side of the + // diff, even if the second click is clicking part of the selection + // range where the selection highlight is currently invisible because + // of CSS rules. + + // This behavior looks and feels glitchy: an invisible selection range + // suddenly pops into existence and there's a bunch of flicker. If we're + // switching selection modes, clear the old selection to avoid this: + // assume the user is not trying to drag-and-drop text which is not + // visually selected. + + if (old_mode !== copy_mode) { + window.getSelection().removeAllRanges(); + } + + // In the more mundane case, if the user selects some text on one side + // of a diff and then clicks that same selection in a normal way (in + // the visible part of the highlighted text), we may either be altering + // the selection range or may be initiating a text drag depending on how + // long they hold the button for. Regardless of what we're doing, we're + // still in a selection mode, so keep the visual hints active. + + JX.DOM.alterClass(copy_root, copy_mode, true); + } + + // We've chosen a mode and saved it now, but we don't actually update to + // apply any visual changes until the user actually starts making some + // kind of selection. + } + + // When the selection range changes, apply CSS classes if the selection is + // nonempty. We don't want to make visual changes to the document immediately + // when the user press the mouse button, since we aren't yet sure that + // they are starting a selection: instead, wait for them to actually select + // something. + function onchangeselect() { + if (!copy_mode) { + return; + } + + var ranges = get_selected_ranges(); + JX.DOM.alterClass(copy_root, copy_mode, !!ranges.length); + } + + // When the user releases the mouse, get rid of the selection mode if we + // don't have a selection. + function onendselect(e) { + if (!copy_mode) { + return; + } + + var ranges = get_selected_ranges(); + if (!ranges.length) { + clear_selection_mode(); + } + } + + function get_selected_ranges() { + var ranges = []; + + if (!window.getSelection) { + return ranges; + } + + var selection = window.getSelection(); + for (var ii = 0; ii < selection.rangeCount; ii++) { + var range = selection.getRangeAt(ii); + if (range.collapsed) { + continue; + } + + ranges.push(range); + } + + return ranges; + } + + function clear_selection_mode() { + if (!copy_root) { + return; + } + + JX.DOM.alterClass(copy_root, copy_mode, false); + copy_root = null; + copy_mode = null; + } + + function oncopy(e) { + // If we aren't in a special copy mode, just fall back to default + // behavior. + if (!copy_mode) { + return; + } + + var ranges = get_selected_ranges(); + if (!ranges.length) { + return; + } + + var text_nodes = []; + for (var ii = 0; ii < ranges.length; ii++) { + var range = ranges[ii]; + + var fragment = range.cloneContents(); + if (!fragment.children.length) { + continue; + } + + // In Chrome and Firefox, because we've already applied "user-select" + // CSS to everything we don't intend to copy, the text in the selection + // range is correct, and the range will include only the correct text + // nodes. + + // However, in Safari, "user-select" does not apply to clipboard + // operations, so we get everything in the document between the beginning + // and end of the selection, even if it isn't visibly selected. + + // Even in Chrome and Firefox, we can get partial empty nodes: for + // example, where a "" is selectable but no content in the node is + // selectable. (We have to leave the "" itself selectable because + // of how Firefox applies "user-select" rules.) + + // The nodes we get here can also start and end more or less anywhere. + + // One saving grace is that we use "content: attr(data-n);" to render + // the line numbers and no browsers copy this content, so we don't have + // to worry about figuring out when text is line numbers. + + for (var jj = 0; jj < fragment.childNodes.length; jj++) { + var node = fragment.childNodes[jj]; + if (JX.DOM.isType(node, 'tr')) { + // This is an inline comment row, so we never want to copy any + // content inside of it. + if (JX.Stratcom.hasSigil(node, 'inline-row')) { + continue; + } + + // Assume anything else is a source code row. Keep only "" cells + // with the correct mode. + for (var kk = 0; kk < node.childNodes.length; kk++) { + var child = node.childNodes[kk]; + + var node_mode = child.getAttribute('data-copy-mode'); + if (node_mode === copy_mode) { + text_nodes.push(child); + } + } + } else { + // For anything else, assume this is a text fragment or part of + // a table cell or something and should be included in the selection + // range. + text_nodes.push(node); + } + } + + var text = []; + for (ii = 0; ii < text_nodes.length; ii++) { + text.push(text_nodes[ii].textContent); + } + text = text.join(''); var rawEvent = e.getRawEvent(); - var clipboardData = 'clipboardData' in rawEvent ? - rawEvent.clipboardData : - window.clipboardData; - clipboardData.setData('Text', result); + var data; + if ('clipboardData' in rawEvent) { + data = rawEvent.clipboardData; + } else { + data = window.clipboardData; + } + data.setData('Text', text); + e.prevent(); - }); + } + } + + JX.enableDispatch(document.body, 'copy'); + JX.enableDispatch(window, 'selectionchange'); + + JX.Stratcom.listen('mousedown', null, onstartselect); + JX.Stratcom.listen('selectionchange', null, onchangeselect); + JX.Stratcom.listen('mouseup', null, onendselect); + + JX.Stratcom.listen('copy', null, oncopy); }); From efccd75ae37174e370a9c88197e60b178fc8e5fe Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 17 Feb 2019 07:58:26 -0800 Subject: [PATCH 21/30] Correct various minor diff copy behaviors Summary: Ref T12822. Fixes a few things: - Firefox selection of weird ranges with an inline between the start and end of the range now works correctly. - "Show More Context" rows now render, highlight, and select properly. - Prepares for nodes to have copy-text which is different from display-text. - Don't do anything too fancy in 1-up/unified mode. We don't copy line numbers after the `content: attr(data-n)` change, but that's as far as we go, because trying to do more than that is kind of weird and not terribly intuitive. Test Plan: - Selected and copied weird ranges in Firefox. - Kept an eye on "Show More Context" rows across select and copy operations. - Generally poked around in Safari/Firefox/Chrome. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12822 Differential Revision: https://secure.phabricator.com/D20192 --- resources/celerity/map.php | 40 +++--- .../DifferentialChangesetHTMLRenderer.php | 12 +- .../DifferentialChangesetTwoUpRenderer.php | 20 ++- .../differential/changeset-view.css | 31 ++++- .../js/application/diff/DiffChangesetList.js | 22 ++- webroot/rsrc/js/core/behavior-oncopy.js | 127 ++++++++++++------ 6 files changed, 175 insertions(+), 77 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 086cb1b8b3..04036e70e4 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,9 +10,9 @@ return array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '261ee8cf', - 'core.pkg.js' => '5ba0b6d7', - 'differential.pkg.css' => 'd1b29c9c', - 'differential.pkg.js' => '0e2b0e2c', + 'core.pkg.js' => 'e368deda', + 'differential.pkg.css' => '249b542d', + 'differential.pkg.js' => '53f8d00c', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', 'maniphest.pkg.css' => '35995d6d', @@ -61,7 +61,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '4267d6c6', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', - 'rsrc/css/application/differential/changeset-view.css' => 'e2b81e85', + 'rsrc/css/application/differential/changeset-view.css' => 'cc3fd795', 'rsrc/css/application/differential/core.css' => 'bdb93065', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', @@ -375,7 +375,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '1e413dc9', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '9b1cbd76', 'rsrc/js/application/diff/DiffChangeset.js' => 'd0a85a85', - 'rsrc/js/application/diff/DiffChangesetList.js' => '26fb79ba', + 'rsrc/js/application/diff/DiffChangesetList.js' => '04023d82', 'rsrc/js/application/diff/DiffInline.js' => 'a4a14a94', 'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17', 'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd', @@ -473,7 +473,7 @@ return array( 'rsrc/js/core/behavior-linked-container.js' => '74446546', 'rsrc/js/core/behavior-more.js' => '506aa3f4', 'rsrc/js/core/behavior-object-selector.js' => 'a4af0b4a', - 'rsrc/js/core/behavior-oncopy.js' => 'f20d66c1', + 'rsrc/js/core/behavior-oncopy.js' => 'de59bf15', 'rsrc/js/core/behavior-phabricator-nav.js' => 'f166c949', 'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => '2f80333f', 'rsrc/js/core/behavior-read-only-warning.js' => 'b9109f8f', @@ -541,7 +541,7 @@ return array( 'conpherence-thread-manager' => 'aec8e38c', 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => 'd67475f5', - 'differential-changeset-view-css' => 'e2b81e85', + 'differential-changeset-view-css' => 'cc3fd795', 'differential-core-view-css' => 'bdb93065', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', @@ -636,7 +636,7 @@ return array( 'javelin-behavior-phabricator-nav' => 'f166c949', 'javelin-behavior-phabricator-notification-example' => '29819b75', 'javelin-behavior-phabricator-object-selector' => 'a4af0b4a', - 'javelin-behavior-phabricator-oncopy' => 'f20d66c1', + 'javelin-behavior-phabricator-oncopy' => 'de59bf15', 'javelin-behavior-phabricator-remarkup-assist' => '2f80333f', 'javelin-behavior-phabricator-reveal-content' => 'b105a3a6', 'javelin-behavior-phabricator-search-typeahead' => '1cb7d027', @@ -754,7 +754,7 @@ return array( 'phabricator-darkmessage' => '26cd4b73', 'phabricator-dashboard-css' => '4267d6c6', 'phabricator-diff-changeset' => 'd0a85a85', - 'phabricator-diff-changeset-list' => '26fb79ba', + 'phabricator-diff-changeset-list' => '04023d82', 'phabricator-diff-inline' => 'a4a14a94', 'phabricator-drag-and-drop-file-upload' => '4370900d', 'phabricator-draggable-list' => '3c6bd549', @@ -907,6 +907,10 @@ return array( 'javelin-uri', 'javelin-util', ), + '04023d82' => array( + 'javelin-install', + 'phuix-button-view', + ), '04f8a1e3' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1087,10 +1091,6 @@ return array( 'javelin-json', 'phabricator-draggable-list', ), - '26fb79ba' => array( - 'javelin-install', - 'phuix-button-view', - ), '27daef73' => array( 'multirow-row-manager', 'javelin-install', @@ -1961,6 +1961,9 @@ return array( 'javelin-util', 'phabricator-keyboard-shortcut-manager', ), + 'cc3fd795' => array( + 'phui-inline-comment-view-css', + ), 'cf32921f' => array( 'javelin-behavior', 'javelin-dom', @@ -2007,6 +2010,10 @@ return array( 'javelin-uri', 'phabricator-notification', ), + 'de59bf15' => array( + 'javelin-behavior', + 'javelin-dom', + ), 'dfa1d313' => array( 'javelin-behavior', 'javelin-dom', @@ -2032,9 +2039,6 @@ return array( 'javelin-dom', 'javelin-stratcom', ), - 'e2b81e85' => array( - 'phui-inline-comment-view-css', - ), 'e562708c' => array( 'javelin-install', ), @@ -2086,10 +2090,6 @@ return array( 'javelin-request', 'javelin-util', ), - 'f20d66c1' => array( - 'javelin-behavior', - 'javelin-dom', - ), 'f340a484' => array( 'javelin-install', 'javelin-dom', diff --git a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php index e4320b712b..df59db9228 100644 --- a/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetHTMLRenderer.php @@ -432,11 +432,17 @@ abstract class DifferentialChangesetHTMLRenderer $classes[] = 'PhabricatorMonospaced'; $classes[] = $this->getRendererTableClass(); + $sigils = array(); + $sigils[] = 'differential-diff'; + foreach ($this->getTableSigils() as $sigil) { + $sigils[] = $sigil; + } + return javelin_tag( 'table', array( 'class' => implode(' ', $classes), - 'sigil' => 'differential-diff intercept-copy', + 'sigil' => implode(' ', $sigils), ), array( $this->renderColgroup(), @@ -444,6 +450,10 @@ abstract class DifferentialChangesetHTMLRenderer )); } + protected function getTableSigils() { + return array(); + } + protected function buildInlineComment( PhabricatorInlineCommentInterface $comment, $on_right = false) { diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index 290272db49..7c728c1ff7 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -117,16 +117,20 @@ final class DifferentialChangesetTwoUpRenderer phutil_tag( 'td', array( - 'colspan' => 2, + 'class' => 'show-context-line n left-context', + )), + phutil_tag( + 'td', + array( 'class' => 'show-more', ), $contents), phutil_tag( - 'th', + 'td', array( - 'class' => 'show-context-line', - ), - $context_line ? (int)$context_line : null), + 'class' => 'show-context-line n', + 'data-n' => $context_line, + )), phutil_tag( 'td', array( @@ -448,4 +452,10 @@ final class DifferentialChangesetTwoUpRenderer return $this->newOffsetMap; } + protected function getTableSigils() { + return array( + 'intercept-copy', + ); + } + } diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index bc1ca8b6ea..7d578ccc03 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -87,7 +87,7 @@ color: {$darkgreytext}; } -.differential-changeset-immutable .differential-diff th { +.differential-changeset-immutable .differential-diff td { cursor: auto; } @@ -182,6 +182,10 @@ should always have a boring grey background. */ content: attr(data-n); } +.differential-diff td.show-context-line.n { + cursor: auto; +} + .differential-diff td.cov { padding: 0; } @@ -222,7 +226,7 @@ td.cov-I { } .differential-diff td.show-more, -.differential-diff th.show-context-line, +.differential-diff td.show-context-line, .differential-diff td.show-context, .differential-diff td.differential-shield { background: {$lightbluebackground}; @@ -232,7 +236,7 @@ td.cov-I { } .device .differential-diff td.show-more, -.device .differential-diff th.show-context-line, +.device .differential-diff td.show-context-line, .device .differential-diff td.show-context, .device .differential-diff td.differential-shield { padding: 6px 0; @@ -250,10 +254,14 @@ td.cov-I { color: {$bluetext}; } -.differential-diff th.show-context-line { +.differential-diff td.show-context-line { padding-right: 6px; } +.differential-diff td.show-context-line.left-context { + border-right: none; +} + .differential-diff td.show-context { padding-left: 14px; } @@ -431,8 +439,7 @@ unselectable. */ .differential-diff.copy-l > tbody > tr > td, .differential-diff.copy-r > tbody > tr > td { - -moz-user-select: -moz-none; - -khtml-user-select: none; + -moz-user-select: none; -ms-user-select: none; -webkit-user-select: none; user-select: none; @@ -444,12 +451,24 @@ unselectable. */ } .differential-diff.copy-l > tbody > tr > td:nth-child(2) { + -moz-user-select: auto; + -ms-user-select: auto; -webkit-user-select: auto; user-select: auto; opacity: 1; } +.differential-diff.copy-l > tbody > tr > td.show-more:nth-child(2) { + -moz-user-select: none; + -ms-user-select: none; + -webkit-user-select: none; + user-select: none; + opacity: 0.25; +} + .differential-diff.copy-r > tbody > tr > td:nth-child(5) { + -moz-user-select: auto; + -ms-user-select: auto; -webkit-user-select: auto; user-select: auto; opacity: 1; diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 8a1306967a..572faad987 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -1246,8 +1246,24 @@ JX.install('DiffChangesetList', { return changeset.getInlineForRow(inline_row); }, - getLineNumberFromHeader: function(th) { - return parseInt(th.getAttribute('data-n')); + getLineNumberFromHeader: function(node) { + var n = parseInt(node.getAttribute('data-n')); + + if (!n) { + return null; + } + + // If this is a line number that's part of a row showing more context, + // we don't want to let users leave inlines here. + + try { + JX.DOM.findAbove(node, 'tr', 'context-target'); + return null; + } catch (ex) { + // Ignore. + } + + return n; }, getDisplaySideFromHeader: function(th) { @@ -1295,7 +1311,7 @@ JX.install('DiffChangesetList', { }, _updateRange: function(target, is_out) { - // Don't update the range if this "" doesn't correspond to a line + // Don't update the range if this target doesn't correspond to a line // number. For instance, this may be a dead line number, like the empty // line numbers on the left hand side of a newly added file. var number = this.getLineNumberFromHeader(target); diff --git a/webroot/rsrc/js/core/behavior-oncopy.js b/webroot/rsrc/js/core/behavior-oncopy.js index a37474abf9..e7309a1e18 100644 --- a/webroot/rsrc/js/core/behavior-oncopy.js +++ b/webroot/rsrc/js/core/behavior-oncopy.js @@ -186,12 +186,12 @@ JX.behavior('phabricator-oncopy', function() { return; } - var text_nodes = []; + var text = []; for (var ii = 0; ii < ranges.length; ii++) { var range = ranges[ii]; var fragment = range.cloneContents(); - if (!fragment.children.length) { + if (!fragment.childNodes.length) { continue; } @@ -217,48 +217,91 @@ JX.behavior('phabricator-oncopy', function() { for (var jj = 0; jj < fragment.childNodes.length; jj++) { var node = fragment.childNodes[jj]; - if (JX.DOM.isType(node, 'tr')) { - // This is an inline comment row, so we never want to copy any - // content inside of it. - if (JX.Stratcom.hasSigil(node, 'inline-row')) { - continue; - } - - // Assume anything else is a source code row. Keep only "" cells - // with the correct mode. - for (var kk = 0; kk < node.childNodes.length; kk++) { - var child = node.childNodes[kk]; - - var node_mode = child.getAttribute('data-copy-mode'); - if (node_mode === copy_mode) { - text_nodes.push(child); - } - } - } else { - // For anything else, assume this is a text fragment or part of - // a table cell or something and should be included in the selection - // range. - text_nodes.push(node); - } + text.push(extract_text(node)); } - - var text = []; - for (ii = 0; ii < text_nodes.length; ii++) { - text.push(text_nodes[ii].textContent); - } - text = text.join(''); - - var rawEvent = e.getRawEvent(); - var data; - if ('clipboardData' in rawEvent) { - data = rawEvent.clipboardData; - } else { - data = window.clipboardData; - } - data.setData('Text', text); - - e.prevent(); } + + text = flatten_list(text); + text = text.join(''); + + var rawEvent = e.getRawEvent(); + var data; + if ('clipboardData' in rawEvent) { + data = rawEvent.clipboardData; + } else { + data = window.clipboardData; + } + data.setData('Text', text); + + e.prevent(); + } + + function extract_text(node) { + var ii; + var text = []; + + if (JX.DOM.isType(node, 'tr')) { + // This is an inline comment row, so we never want to copy any + // content inside of it. + if (JX.Stratcom.hasSigil(node, 'inline-row')) { + return null; + } + + // This is a "Show More Context" row, so we never want to copy any + // of the content inside. + if (JX.Stratcom.hasSigil(node, 'context-target')) { + return null; + } + + // Assume anything else is a source code row. Keep only "" cells + // with the correct mode. + for (ii = 0; ii < node.childNodes.length; ii++) { + text.push(extract_text(node.childNodes[ii])); + } + + return text; + } + + if (JX.DOM.isType(node, 'td')) { + var node_mode = node.getAttribute('data-copy-mode'); + if (node_mode !== copy_mode) { + return; + } + + // Otherwise, fall through and extract this node's text normally. + } + + if (!node.childNodes || !node.childNodes.length) { + return node.textContent; + } + + for (ii = 0; ii < node.childNodes.length; ii++) { + var child = node.childNodes[ii]; + text.push(extract_text(child)); + } + + return text; + } + + function flatten_list(list) { + var stack = [list]; + var result = []; + while (stack.length) { + var next = stack.pop(); + if (JX.isArray(next)) { + for (var ii = 0; ii < next.length; ii++) { + stack.push(next[ii]); + } + } else if (next === null) { + continue; + } else if (next === undefined) { + continue; + } else { + result.push(next); + } + } + + return result.reverse(); } JX.enableDispatch(document.body, 'copy'); From fe7047d12d6904696a2eb9f2c2b2cbe8831625fe Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 17 Feb 2019 13:39:46 -0800 Subject: [PATCH 22/30] Display some invisible/nonprintable characters in diffs by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Ref T12822. Ref T2495. This is the good version of D20193. Currently, we display various nonprintable characters (ZWS, nonbreaking space, various control characters) as themselves, so they're generally invisible. In T12822, one user reports that all their engineers frequently type ZWS characters into source somehow? I don't really believe this (??), and this should be fixed in lint. That said, the only real reason not to show these weird characters in a special way was that it would break copy/paste: if we render ZWS as "🐑", and a user copy-pastes the line including the ZWS, they'll get a sheep. At least, they would have, until D20191. Now that this whole thing is end-to-end Javascript magic, we can copy whatever we want. In particular, we can render any character `X` as `X`, and then copy "Y" instead of "X" when the user copies the node. Limitations: - If users select only "X", they'll get "X" on their clipboard. This seems fine. If you're selecting our ZWS marker *only*, you probably want to copy it? - If "X" is more than one character long, users will get the full "Y" if they select any part of "X". At least here, this only matters when "X" is several spaces and "Y" is a tab. This also seems fine. - We have to be kind of careful because this approach involves editing an HTML blob directly. However, we already do that elsewhere and this isn't really too hard to get right. With those tools in hand: - Replace "\t" (raw text / what gets copied) with the number of spaces to the next tab stop for display. - Replace ZWS and NBSP (raw text) with a special marker for display. - Replace control characters 0x00-0x19 and 0x7F, except for "\t", "\r", and "\n", with the special unicode "control character pictures" reserved for this purpose. Test Plan: - Generated and viewed a file like this one: {F6220422} - Copied text out of it, got authentic raw original source text instead of displayed text. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12822, T2495 Differential Revision: https://secure.phabricator.com/D20194 --- resources/celerity/map.php | 26 +-- .../parser/DifferentialChangesetParser.php | 190 +++++++++++++++++- .../render/DifferentialChangesetRenderer.php | 14 +- .../DifferentialChangesetTestRenderer.php | 2 +- webroot/rsrc/css/core/syntax.css | 6 + webroot/rsrc/js/core/behavior-oncopy.js | 7 + 6 files changed, 221 insertions(+), 24 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 04036e70e4..98d44804d3 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,8 +9,8 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '261ee8cf', - 'core.pkg.js' => 'e368deda', + 'core.pkg.css' => 'e3c1a8f2', + 'core.pkg.js' => '2cda17a4', 'differential.pkg.css' => '249b542d', 'differential.pkg.js' => '53f8d00c', 'diffusion.pkg.css' => '42c75c37', @@ -112,7 +112,7 @@ return array( 'rsrc/css/application/uiexample/example.css' => 'b4795059', 'rsrc/css/core/core.css' => '1b29ed61', 'rsrc/css/core/remarkup.css' => '9e627d41', - 'rsrc/css/core/syntax.css' => '8a16f91b', + 'rsrc/css/core/syntax.css' => '4234f572', 'rsrc/css/core/z-index.css' => '99c0f5eb', 'rsrc/css/diviner/diviner-shared.css' => '4bd263b0', 'rsrc/css/font/font-awesome.css' => '3883938a', @@ -473,7 +473,7 @@ return array( 'rsrc/js/core/behavior-linked-container.js' => '74446546', 'rsrc/js/core/behavior-more.js' => '506aa3f4', 'rsrc/js/core/behavior-object-selector.js' => 'a4af0b4a', - 'rsrc/js/core/behavior-oncopy.js' => 'de59bf15', + 'rsrc/js/core/behavior-oncopy.js' => 'ff7b3f22', 'rsrc/js/core/behavior-phabricator-nav.js' => 'f166c949', 'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => '2f80333f', 'rsrc/js/core/behavior-read-only-warning.js' => 'b9109f8f', @@ -636,7 +636,7 @@ return array( 'javelin-behavior-phabricator-nav' => 'f166c949', 'javelin-behavior-phabricator-notification-example' => '29819b75', 'javelin-behavior-phabricator-object-selector' => 'a4af0b4a', - 'javelin-behavior-phabricator-oncopy' => 'de59bf15', + 'javelin-behavior-phabricator-oncopy' => 'ff7b3f22', 'javelin-behavior-phabricator-remarkup-assist' => '2f80333f', 'javelin-behavior-phabricator-reveal-content' => 'b105a3a6', 'javelin-behavior-phabricator-search-typeahead' => '1cb7d027', @@ -878,7 +878,7 @@ return array( 'sprite-login-css' => '18b368a6', 'sprite-tokens-css' => 'f1896dc5', 'syntax-default-css' => '055fc231', - 'syntax-highlighting-css' => '8a16f91b', + 'syntax-highlighting-css' => '4234f572', 'tokens-css' => 'ce5a50bd', 'typeahead-browse-css' => 'b7ed02d2', 'unhandled-exception-css' => '9ecfc00d', @@ -1222,6 +1222,9 @@ return array( 'javelin-behavior', 'javelin-uri', ), + '4234f572' => array( + 'syntax-default-css', + ), '42c7a5a7' => array( 'javelin-install', 'javelin-dom', @@ -1580,9 +1583,6 @@ return array( 'javelin-stratcom', 'javelin-install', ), - '8a16f91b' => array( - 'syntax-default-css', - ), '8ac32fd9' => array( 'javelin-behavior', 'javelin-stratcom', @@ -2010,10 +2010,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - 'de59bf15' => array( - 'javelin-behavior', - 'javelin-dom', - ), 'dfa1d313' => array( 'javelin-behavior', 'javelin-dom', @@ -2147,6 +2143,10 @@ return array( 'owners-path-editor', 'javelin-behavior', ), + 'ff7b3f22' => array( + 'javelin-behavior', + 'javelin-dom', + ), ), 'packages' => array( 'conpherence.pkg.css' => array( diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index f0f952328a..8ed6d80eed 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -189,7 +189,7 @@ final class DifferentialChangesetParser extends Phobject { return $this; } - const CACHE_VERSION = 13; + const CACHE_VERSION = 14; const CACHE_MAX_SIZE = 8e6; const ATTR_GENERATED = 'attr:generated'; @@ -568,11 +568,17 @@ final class DifferentialChangesetParser extends Phobject { private function applyIntraline(&$render, $intra, $corpus) { foreach ($render as $key => $text) { + $result = $text; + if (isset($intra[$key])) { - $render[$key] = ArcanistDiffUtils::applyIntralineDiff( - $text, + $result = ArcanistDiffUtils::applyIntralineDiff( + $result, $intra[$key]); } + + $result = $this->adjustRenderedLineForDisplay($result); + + $render[$key] = $result; } } @@ -1415,5 +1421,183 @@ final class DifferentialChangesetParser extends Phobject { $hunk_parser->setNewLineTypeMap($type_parser->getNewLineTypeMap()); } + private function adjustRenderedLineForDisplay($line) { + // IMPORTANT: We're using "str_replace()" against raw HTML here, which can + // easily become unsafe. The input HTML has already had syntax highlighting + // and intraline diff highlighting applied, so it's full of "" tags. + + static $search; + static $replace; + if ($search === null) { + $rules = $this->newSuspiciousCharacterRules(); + + $map = array(); + foreach ($rules as $key => $spec) { + $tag = phutil_tag( + 'span', + array( + 'data-copy-text' => $key, + 'class' => $spec['class'], + 'title' => $spec['title'], + ), + $spec['replacement']); + $map[$key] = phutil_string_cast($tag); + } + + $search = array_keys($map); + $replace = array_values($map); + } + + $is_html = false; + if ($line instanceof PhutilSafeHTML) { + $is_html = true; + $line = hsprintf('%s', $line); + } + + $line = phutil_string_cast($line); + + if (strpos($line, "\t") !== false) { + $line = $this->replaceTabsWithSpaces($line); + } + $line = str_replace($search, $replace, $line); + + if ($is_html) { + $line = phutil_safe_html($line); + } + + return $line; + } + + private function newSuspiciousCharacterRules() { + // The "title" attributes are cached in the database, so they're + // intentionally not wrapped in "pht(...)". + + $rules = array( + "\xE2\x80\x8B" => array( + 'title' => 'ZWS', + 'class' => 'suspicious-character', + 'replacement' => '!', + ), + "\xC2\xA0" => array( + 'title' => 'NBSP', + 'class' => 'suspicious-character', + 'replacement' => '!', + ), + "\x7F" => array( + 'title' => 'DEL (0x7F)', + 'class' => 'suspicious-character', + 'replacement' => "\xE2\x90\xA1", + ), + ); + + // Unicode defines special pictures for the control characters in the + // range between "0x00" and "0x1F". + + $control = array( + 'NULL', + 'SOH', + 'STX', + 'ETX', + 'EOT', + 'ENQ', + 'ACK', + 'BEL', + 'BS', + null, // "\t" Tab + null, // "\n" New Line + 'VT', + 'FF', + null, // "\r" Carriage Return, + 'SO', + 'SI', + 'DLE', + 'DC1', + 'DC2', + 'DC3', + 'DC4', + 'NAK', + 'SYN', + 'ETB', + 'CAN', + 'EM', + 'SUB', + 'ESC', + 'FS', + 'GS', + 'RS', + 'US', + ); + + foreach ($control as $idx => $label) { + if ($label === null) { + continue; + } + + $rules[chr($idx)] = array( + 'title' => sprintf('%s (0x%02X)', $label, $idx), + 'class' => 'suspicious-character', + 'replacement' => "\xE2\x90".chr(0x80 + $idx), + ); + } + + return $rules; + } + + private function replaceTabsWithSpaces($line) { + // TODO: This should be flexible, eventually. + $tab_width = 2; + + static $tags; + if ($tags === null) { + $tags = array(); + for ($ii = 1; $ii <= $tab_width; $ii++) { + $tag = phutil_tag( + 'span', + array( + 'data-copy-text' => "\t", + ), + str_repeat(' ', $ii)); + $tag = phutil_string_cast($tag); + $tags[$ii] = $tag; + } + } + + // If the line is particularly long, don't try to vectorize it. Use a + // faster approximation of the correct tabstop expansion instead. This + // usually still arrives at the right result. + if (strlen($line) > 256) { + return str_replace("\t", $tags[$tab_width], $line); + } + + $line = phutil_utf8v_combined($line); + $in_tag = false; + $pos = 0; + foreach ($line as $key => $char) { + if ($char === '<') { + $in_tag = true; + continue; + } + + if ($char === '>') { + $in_tag = false; + continue; + } + + if ($in_tag) { + continue; + } + + if ($char === "\t") { + $count = $tab_width - ($pos % $tab_width); + $pos += $count; + $line[$key] = $tags[$count]; + continue; + } + + $pos++; + } + + return implode('', $line); + } } diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index 866ae15ac9..b7f78b5b68 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -363,14 +363,14 @@ abstract class DifferentialChangesetRenderer extends Phobject { $undershield = $this->renderUndershieldHeader(); } - $result = $notice.$props.$undershield.$content; + $result = array( + $notice, + $props, + $undershield, + $content, + ); - // TODO: Let the user customize their tab width / display style. - // TODO: We should possibly post-process "\r" as well. - // TODO: Both these steps should happen earlier. - $result = str_replace("\t", ' ', $result); - - return phutil_safe_html($result); + return hsprintf('%s', $result); } abstract public function isOneUpRenderer(); diff --git a/src/applications/differential/render/DifferentialChangesetTestRenderer.php b/src/applications/differential/render/DifferentialChangesetTestRenderer.php index c7b35d1fb4..e2bd3f53ed 100644 --- a/src/applications/differential/render/DifferentialChangesetTestRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTestRenderer.php @@ -131,7 +131,7 @@ abstract class DifferentialChangesetTestRenderer } $out = implode("\n", $out)."\n"; - return $out; + return phutil_safe_html($out); } diff --git a/webroot/rsrc/css/core/syntax.css b/webroot/rsrc/css/core/syntax.css index cfc82da09b..90f2981ba6 100644 --- a/webroot/rsrc/css/core/syntax.css +++ b/webroot/rsrc/css/core/syntax.css @@ -29,3 +29,9 @@ span.crossreference-item { color: #222222; background: #dddddd; } + +.suspicious-character { + background: #ff7700; + color: #ffffff; + cursor: default; +} diff --git a/webroot/rsrc/js/core/behavior-oncopy.js b/webroot/rsrc/js/core/behavior-oncopy.js index e7309a1e18..b56e83ab32 100644 --- a/webroot/rsrc/js/core/behavior-oncopy.js +++ b/webroot/rsrc/js/core/behavior-oncopy.js @@ -271,6 +271,13 @@ JX.behavior('phabricator-oncopy', function() { // Otherwise, fall through and extract this node's text normally. } + if (node.getAttribute) { + var copy_text = node.getAttribute('data-copy-text'); + if (copy_text) { + return copy_text; + } + } + if (!node.childNodes || !node.childNodes.length) { return node.textContent; } From cf048f4402feb6eddb6d97dd037bab5f77d845fe Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 19 Feb 2019 14:39:08 -0800 Subject: [PATCH 23/30] Tweak some display behaviors for indent indicators Summary: Ref T13161. - Don't show ">>" when the line indentation changed but the text also changed, this is just "the line changed". - The indicator seems a little cleaner if we just reuse the existing "bright" colors, which already have colorblind colors anyway. Test Plan: Got slightly better rendering for some diffs locally. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13161 Differential Revision: https://secure.phabricator.com/D20195 --- resources/celerity/map.php | 12 ++++++------ .../postprocessor/CelerityDefaultPostprocessor.php | 2 -- .../differential/parser/DifferentialHunkParser.php | 2 +- .../css/application/differential/changeset-view.css | 6 +++--- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 98d44804d3..0fb2a50904 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ return array( 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => 'e3c1a8f2', 'core.pkg.js' => '2cda17a4', - 'differential.pkg.css' => '249b542d', + 'differential.pkg.css' => '9f215e54', 'differential.pkg.js' => '53f8d00c', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', @@ -61,7 +61,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '4267d6c6', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', - 'rsrc/css/application/differential/changeset-view.css' => 'cc3fd795', + 'rsrc/css/application/differential/changeset-view.css' => 'de570228', 'rsrc/css/application/differential/core.css' => 'bdb93065', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', @@ -541,7 +541,7 @@ return array( 'conpherence-thread-manager' => 'aec8e38c', 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => 'd67475f5', - 'differential-changeset-view-css' => 'cc3fd795', + 'differential-changeset-view-css' => 'de570228', 'differential-core-view-css' => 'bdb93065', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', @@ -1961,9 +1961,6 @@ return array( 'javelin-util', 'phabricator-keyboard-shortcut-manager', ), - 'cc3fd795' => array( - 'phui-inline-comment-view-css', - ), 'cf32921f' => array( 'javelin-behavior', 'javelin-dom', @@ -2010,6 +2007,9 @@ return array( 'javelin-uri', 'phabricator-notification', ), + 'de570228' => array( + 'phui-inline-comment-view-css', + ), 'dfa1d313' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php b/src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php index d848fb81e8..61f6176f15 100644 --- a/src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php +++ b/src/applications/celerity/postprocessor/CelerityDefaultPostprocessor.php @@ -199,10 +199,8 @@ final class CelerityDefaultPostprocessor 'diff.background' => '#fff', 'new-background' => 'rgba(151, 234, 151, .3)', 'new-bright' => 'rgba(151, 234, 151, .6)', - 'new-background-strong' => 'rgba(151, 234, 151, 1)', 'old-background' => 'rgba(251, 175, 175, .3)', 'old-bright' => 'rgba(251, 175, 175, .7)', - 'old-background-strong' => 'rgba(251, 175, 175, 1)', 'move-background' => '#fdf5d4', 'copy-background' => '#f1c40f', diff --git a/src/applications/differential/parser/DifferentialHunkParser.php b/src/applications/differential/parser/DifferentialHunkParser.php index d59358bc82..8667c032b7 100644 --- a/src/applications/differential/parser/DifferentialHunkParser.php +++ b/src/applications/differential/parser/DifferentialHunkParser.php @@ -288,7 +288,7 @@ final class DifferentialHunkParser extends Phobject { $o_text = $o['text']; $n_text = $n['text']; - if ($o_text !== $n_text) { + if ($o_text !== $n_text && (ltrim($o_text) === ltrim($n_text))) { $o_depth = $this->getIndentDepth($o_text, $tab_width); $n_depth = $this->getIndentDepth($n_text, $tab_width); diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 7d578ccc03..55cc5e8fd3 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -130,13 +130,13 @@ .differential-diff td span.depth-out { background-image: url(/rsrc/image/chevron-out.png); - background-color: {$old-background-strong}; + background-color: {$old-bright}; } .differential-diff td span.depth-in { - background-position: 2px center; + background-position: 1px center; background-image: url(/rsrc/image/chevron-in.png); - background-color: {$new-background-strong}; + background-color: {$new-bright}; } From c10b283b927053eaa434e15d1d59551645641d7d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 20 Feb 2019 09:25:44 -0800 Subject: [PATCH 24/30] Remove some ancient daemon log code Summary: Ref T13253. Long ago, daemon logs were visible in the web UI. They were removed because access to logs generally does not conform to policy rules, and may leak the existence (and sometimes contents) of hidden objects, occasionally leak credentials in certain error messages, etc. These bits and pieces were missed. Test Plan: Grepped for removed symbols. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13253 Differential Revision: https://secure.phabricator.com/D20199 --- src/__phutil_library_map__.php | 4 - .../PhabricatorDaemonsApplication.php | 1 - ...habricatorDaemonLogEventViewController.php | 47 ------- .../view/PhabricatorDaemonLogEventsView.php | 131 ------------------ 4 files changed, 183 deletions(-) delete mode 100644 src/applications/daemon/controller/PhabricatorDaemonLogEventViewController.php delete mode 100644 src/applications/daemon/view/PhabricatorDaemonLogEventsView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index ea3bf7d32d..f4ee380cc0 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2860,8 +2860,6 @@ phutil_register_library_map(array( 'PhabricatorDaemonLog' => 'applications/daemon/storage/PhabricatorDaemonLog.php', 'PhabricatorDaemonLogEvent' => 'applications/daemon/storage/PhabricatorDaemonLogEvent.php', 'PhabricatorDaemonLogEventGarbageCollector' => 'applications/daemon/garbagecollector/PhabricatorDaemonLogEventGarbageCollector.php', - 'PhabricatorDaemonLogEventViewController' => 'applications/daemon/controller/PhabricatorDaemonLogEventViewController.php', - 'PhabricatorDaemonLogEventsView' => 'applications/daemon/view/PhabricatorDaemonLogEventsView.php', 'PhabricatorDaemonLogGarbageCollector' => 'applications/daemon/garbagecollector/PhabricatorDaemonLogGarbageCollector.php', 'PhabricatorDaemonLogListController' => 'applications/daemon/controller/PhabricatorDaemonLogListController.php', 'PhabricatorDaemonLogListView' => 'applications/daemon/view/PhabricatorDaemonLogListView.php', @@ -8725,8 +8723,6 @@ phutil_register_library_map(array( ), 'PhabricatorDaemonLogEvent' => 'PhabricatorDaemonDAO', 'PhabricatorDaemonLogEventGarbageCollector' => 'PhabricatorGarbageCollector', - 'PhabricatorDaemonLogEventViewController' => 'PhabricatorDaemonController', - 'PhabricatorDaemonLogEventsView' => 'AphrontView', 'PhabricatorDaemonLogGarbageCollector' => 'PhabricatorGarbageCollector', 'PhabricatorDaemonLogListController' => 'PhabricatorDaemonController', 'PhabricatorDaemonLogListView' => 'AphrontView', diff --git a/src/applications/daemon/application/PhabricatorDaemonsApplication.php b/src/applications/daemon/application/PhabricatorDaemonsApplication.php index a0fb77beb0..08e81d5d7e 100644 --- a/src/applications/daemon/application/PhabricatorDaemonsApplication.php +++ b/src/applications/daemon/application/PhabricatorDaemonsApplication.php @@ -45,7 +45,6 @@ final class PhabricatorDaemonsApplication extends PhabricatorApplication { '' => 'PhabricatorDaemonLogListController', '(?P[1-9]\d*)/' => 'PhabricatorDaemonLogViewController', ), - 'event/(?P[1-9]\d*)/' => 'PhabricatorDaemonLogEventViewController', 'bulk/' => array( '(?:query/(?P[^/]+)/)?' => 'PhabricatorDaemonBulkJobListController', diff --git a/src/applications/daemon/controller/PhabricatorDaemonLogEventViewController.php b/src/applications/daemon/controller/PhabricatorDaemonLogEventViewController.php deleted file mode 100644 index 208a20b9a0..0000000000 --- a/src/applications/daemon/controller/PhabricatorDaemonLogEventViewController.php +++ /dev/null @@ -1,47 +0,0 @@ -getURIData('id'); - - $event = id(new PhabricatorDaemonLogEvent())->load($id); - if (!$event) { - return new Aphront404Response(); - } - - $event_view = id(new PhabricatorDaemonLogEventsView()) - ->setEvents(array($event)) - ->setUser($request->getUser()) - ->setCombinedLog(true) - ->setShowFullMessage(true); - - $log_panel = id(new PHUIObjectBoxView()) - ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) - ->appendChild($event_view); - - $daemon_id = $event->getLogID(); - - $crumbs = $this->buildApplicationCrumbs() - ->addTextCrumb( - pht('Daemon %s', $daemon_id), - $this->getApplicationURI("log/{$daemon_id}/")) - ->addTextCrumb(pht('Event %s', $event->getID())) - ->setBorder(true); - - $header = id(new PHUIHeaderView()) - ->setHeader(pht('Combined Log')) - ->setHeaderIcon('fa-file-text'); - - $view = id(new PHUITwoColumnView()) - ->setHeader($header) - ->setFooter($log_panel); - - return $this->newPage() - ->setTitle(pht('Combined Daemon Log')) - ->appendChild($view); - - } - -} diff --git a/src/applications/daemon/view/PhabricatorDaemonLogEventsView.php b/src/applications/daemon/view/PhabricatorDaemonLogEventsView.php deleted file mode 100644 index 039906e718..0000000000 --- a/src/applications/daemon/view/PhabricatorDaemonLogEventsView.php +++ /dev/null @@ -1,131 +0,0 @@ -showFullMessage = $show_full_message; - return $this; - } - - public function setEvents(array $events) { - assert_instances_of($events, 'PhabricatorDaemonLogEvent'); - $this->events = $events; - return $this; - } - - public function setCombinedLog($is_combined) { - $this->combinedLog = $is_combined; - return $this; - } - - public function render() { - $viewer = $this->getViewer(); - $rows = array(); - - foreach ($this->events as $event) { - - // Limit display log size. If a daemon gets stuck in an output loop this - // page can be like >100MB if we don't truncate stuff. Try to do cheap - // line-based truncation first, and fall back to expensive UTF-8 character - // truncation if that doesn't get things short enough. - - $message = $event->getMessage(); - $more = null; - - if (!$this->showFullMessage) { - $more_lines = null; - $more_chars = null; - $line_limit = 12; - if (substr_count($message, "\n") > $line_limit) { - $message = explode("\n", $message); - $more_lines = count($message) - $line_limit; - $message = array_slice($message, 0, $line_limit); - $message = implode("\n", $message); - } - - $char_limit = 8192; - if (strlen($message) > $char_limit) { - $message = phutil_utf8v($message); - $more_chars = count($message) - $char_limit; - $message = array_slice($message, 0, $char_limit); - $message = implode('', $message); - } - - if ($more_chars) { - $more = new PhutilNumber($more_chars); - $more = pht('Show %d more character(s)...', $more); - } else if ($more_lines) { - $more = new PhutilNumber($more_lines); - $more = pht('Show %d more line(s)...', $more); - } - - if ($more) { - $id = $event->getID(); - $more = array( - "\n...\n", - phutil_tag( - 'a', - array( - 'href' => "/daemon/event/{$id}/", - ), - $more), - ); - } - } - - $row = array( - $event->getLogType(), - phabricator_date($event->getEpoch(), $viewer), - phabricator_time($event->getEpoch(), $viewer), - array( - $message, - $more, - ), - ); - - if ($this->combinedLog) { - array_unshift( - $row, - phutil_tag( - 'a', - array( - 'href' => '/daemon/log/'.$event->getLogID().'/', - ), - pht('Daemon %s', $event->getLogID()))); - } - - $rows[] = $row; - } - - $classes = array( - '', - '', - 'right', - 'wide prewrap', - ); - - $headers = array( - 'Type', - 'Date', - 'Time', - 'Message', - ); - - if ($this->combinedLog) { - array_unshift($classes, 'pri'); - array_unshift($headers, 'Daemon'); - } - - $log_table = new AphrontTableView($rows); - $log_table->setHeaders($headers); - $log_table->setColumnClasses($classes); - - return $log_table->render(); - } - -} From a33409991c2762f78ddc025bb1aff9838f03bcd5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 19 Feb 2019 20:52:29 -0800 Subject: [PATCH 25/30] Remove an old Differential selection behavior Summary: Ref T12822. Ref PHI878. This is some leftover code from the old selection behavior that prevented visual selection of the left side of a diff if the user clicked on the right -- basically, a much simpler attack on what ultimately landed in D20191. I think the change from `th` to `td` "broke" it so it didn't interfere with the other behavior, which is why I didn't have to remove it earlier. It's no longer necessary, in any case. Test Plan: Grepped for behavior name, selected stuff on both sides of a diff. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12822 Differential Revision: https://secure.phabricator.com/D20196 --- resources/celerity/map.php | 16 +++------ resources/celerity/packages.php | 1 - .../DifferentialRevisionViewController.php | 2 -- .../css/application/differential/core.css | 8 ----- .../differential/behavior-user-select.js | 33 ------------------- 5 files changed, 4 insertions(+), 56 deletions(-) delete mode 100644 webroot/rsrc/js/application/differential/behavior-user-select.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 0fb2a50904..3a5ed56056 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,8 +11,8 @@ return array( 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => 'e3c1a8f2', 'core.pkg.js' => '2cda17a4', - 'differential.pkg.css' => '9f215e54', - 'differential.pkg.js' => '53f8d00c', + 'differential.pkg.css' => '97e13037', + 'differential.pkg.js' => '67e02996', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', 'maniphest.pkg.css' => '35995d6d', @@ -62,7 +62,7 @@ return array( 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', 'rsrc/css/application/differential/changeset-view.css' => 'de570228', - 'rsrc/css/application/differential/core.css' => 'bdb93065', + 'rsrc/css/application/differential/core.css' => '7300a73e', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', 'rsrc/css/application/differential/revision-history.css' => '8aa3eac5', @@ -380,7 +380,6 @@ return array( 'rsrc/js/application/diff/behavior-preview-link.js' => 'f51e9c17', 'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd', 'rsrc/js/application/differential/behavior-populate.js' => 'dfa1d313', - 'rsrc/js/application/differential/behavior-user-select.js' => 'e18685c0', 'rsrc/js/application/diffusion/DiffusionLocateFileSource.js' => '94243d89', 'rsrc/js/application/diffusion/behavior-audit-preview.js' => 'b7b73831', 'rsrc/js/application/diffusion/behavior-commit-branches.js' => '4b671572', @@ -542,7 +541,7 @@ return array( 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => 'd67475f5', 'differential-changeset-view-css' => 'de570228', - 'differential-core-view-css' => 'bdb93065', + 'differential-core-view-css' => '7300a73e', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', 'differential-revision-history-css' => '8aa3eac5', @@ -596,7 +595,6 @@ return array( 'javelin-behavior-diff-preview-link' => 'f51e9c17', 'javelin-behavior-differential-diff-radios' => '925fe8cd', 'javelin-behavior-differential-populate' => 'dfa1d313', - 'javelin-behavior-differential-user-select' => 'e18685c0', 'javelin-behavior-diffusion-commit-branches' => '4b671572', 'javelin-behavior-diffusion-commit-graph' => '1c88f154', 'javelin-behavior-diffusion-locate-file' => '87428eb2', @@ -2030,11 +2028,6 @@ return array( 'javelin-dom', 'javelin-history', ), - 'e18685c0' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - ), 'e562708c' => array( 'javelin-install', ), @@ -2339,7 +2332,6 @@ return array( 'javelin-behavior-aphront-drag-and-drop-textarea', 'javelin-behavior-phabricator-object-selector', 'javelin-behavior-repository-crossreference', - 'javelin-behavior-differential-user-select', 'javelin-behavior-aphront-more', 'phabricator-diff-inline', 'phabricator-diff-changeset', diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php index 4005e064ba..deef3633a8 100644 --- a/resources/celerity/packages.php +++ b/resources/celerity/packages.php @@ -199,7 +199,6 @@ return array( 'javelin-behavior-phabricator-object-selector', 'javelin-behavior-repository-crossreference', - 'javelin-behavior-differential-user-select', 'javelin-behavior-aphront-more', 'phabricator-diff-inline', diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index 2ac1d30eca..ba36b1da17 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -621,8 +621,6 @@ final class DifferentialRevisionViewController ->build($changesets); } - Javelin::initBehavior('differential-user-select'); - $view = id(new PHUITwoColumnView()) ->setHeader($header) ->setSubheader($subheader) diff --git a/webroot/rsrc/css/application/differential/core.css b/webroot/rsrc/css/application/differential/core.css index 2dcc02bb18..893cfb34a8 100644 --- a/webroot/rsrc/css/application/differential/core.css +++ b/webroot/rsrc/css/application/differential/core.css @@ -16,14 +16,6 @@ margin-bottom: 8px; } -.differential-unselectable tr td:nth-of-type(1) { - -moz-user-select: -moz-none; - -khtml-user-select: none; - -webkit-user-select: none; - -ms-user-select: none; - user-select: none; -} - .differential-content-hidden { margin: 0 0 24px 0; } diff --git a/webroot/rsrc/js/application/differential/behavior-user-select.js b/webroot/rsrc/js/application/differential/behavior-user-select.js deleted file mode 100644 index 8db48b704d..0000000000 --- a/webroot/rsrc/js/application/differential/behavior-user-select.js +++ /dev/null @@ -1,33 +0,0 @@ -/** - * @provides javelin-behavior-differential-user-select - * @requires javelin-behavior - * javelin-dom - * javelin-stratcom - */ - -JX.behavior('differential-user-select', function() { - - var unselectable; - - function isOnRight(node) { - return node.previousSibling && - node.parentNode.firstChild != node.previousSibling; - } - - JX.Stratcom.listen( - 'mousedown', - null, - function(e) { - var key = 'differential-unselectable'; - if (unselectable) { - JX.DOM.alterClass(unselectable, key, false); - } - var diff = e.getNode('differential-diff'); - var td = e.getNode('tag:td'); - if (diff && td && isOnRight(td)) { - unselectable = diff; - JX.DOM.alterClass(diff, key, true); - } - }); - -}); From f1a035d5c2c655a598a00dbb682d667f891bafe4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 20 Feb 2019 04:40:06 -0800 Subject: [PATCH 26/30] In Differential, give the "moved/copied from" gutter a more clear visual look Summary: Depends on D20196. See PHI985. When empty, the "moved/copied" gutter currently renders with the same background color as the rest of the line. This can be misleading because it makes code look more indented than it is, especially if you're unfamiliar with the tool: {F6225179} If we remove this misleading coloration, we get a white gap. This is more clear, but looks a little odd: {F6225181} Instead, give this gutter a subtle background fill in all casses, to make it more clear that it's a separate gutter region, not a part of the text diff: {F6225183} Test Plan: See screenshots. Copied text from a diff, added/removed inlines, etc. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20197 --- resources/celerity/map.php | 12 +++++------ .../DifferentialChangesetTwoUpRenderer.php | 5 ++--- .../PHUIDiffOneUpInlineCommentRowScaffold.php | 1 - .../PHUIDiffTwoUpInlineCommentRowScaffold.php | 4 ++-- .../differential/changeset-view.css | 20 ++++++++++++++----- 5 files changed, 25 insertions(+), 17 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 3a5ed56056..3188de0052 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -11,7 +11,7 @@ return array( 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => 'e3c1a8f2', 'core.pkg.js' => '2cda17a4', - 'differential.pkg.css' => '97e13037', + 'differential.pkg.css' => 'ab23bd75', 'differential.pkg.js' => '67e02996', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => '91192d85', @@ -61,7 +61,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '4267d6c6', 'rsrc/css/application/diff/inline-comment-summary.css' => '81eb368d', 'rsrc/css/application/differential/add-comment.css' => '7e5900d9', - 'rsrc/css/application/differential/changeset-view.css' => 'de570228', + 'rsrc/css/application/differential/changeset-view.css' => 'd92bed0d', 'rsrc/css/application/differential/core.css' => '7300a73e', 'rsrc/css/application/differential/phui-inline-comment.css' => '48acce5b', 'rsrc/css/application/differential/revision-comment.css' => '7dbc8d1d', @@ -540,7 +540,7 @@ return array( 'conpherence-thread-manager' => 'aec8e38c', 'conpherence-transaction-css' => '3a3f5e7e', 'd3' => 'd67475f5', - 'differential-changeset-view-css' => 'de570228', + 'differential-changeset-view-css' => 'd92bed0d', 'differential-core-view-css' => '7300a73e', 'differential-revision-add-comment-css' => '7e5900d9', 'differential-revision-comment-css' => '7dbc8d1d', @@ -1997,6 +1997,9 @@ return array( 'javelin-util', 'phabricator-shaped-request', ), + 'd92bed0d' => array( + 'phui-inline-comment-view-css', + ), 'da15d3dc' => array( 'phui-oi-list-view-css', ), @@ -2005,9 +2008,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - 'de570228' => array( - 'phui-inline-comment-view-css', - ), 'dfa1d313' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index 7c728c1ff7..c37655bb93 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -223,7 +223,7 @@ final class DifferentialChangesetTwoUpRenderer ($new_lines[$ii]['type'] == '\\'); if ($not_copied) { - $n_copy = phutil_tag('td', array('class' => "copy {$n_class}")); + $n_copy = phutil_tag('td', array('class' => 'copy')); } else { list($orig_file, $orig_line, $orig_type) = $copy_lines[$n_num]; $title = ($orig_type == '-' ? 'Moved' : 'Copied').' from '; @@ -243,8 +243,7 @@ final class DifferentialChangesetTwoUpRenderer 'msg' => $title, ), 'class' => 'copy '.$class, - ), - ''); + )); } } } diff --git a/src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php b/src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php index 1f8e05bc27..fe5cab8622 100644 --- a/src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php +++ b/src/infrastructure/diff/view/PHUIDiffOneUpInlineCommentRowScaffold.php @@ -18,7 +18,6 @@ final class PHUIDiffOneUpInlineCommentRowScaffold $attrs = array( 'colspan' => 3, - 'class' => 'right3', 'id' => $inline->getScaffoldCellID(), ); diff --git a/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php b/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php index 769ad84d1f..f9bde17bf3 100644 --- a/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php +++ b/src/infrastructure/diff/view/PHUIDiffTwoUpInlineCommentRowScaffold.php @@ -65,8 +65,7 @@ final class PHUIDiffTwoUpInlineCommentRowScaffold ); $right_attrs = array( - 'colspan' => 3, - 'class' => 'right3', + 'colspan' => 2, 'id' => ($right_side ? $right_side->getScaffoldCellID() : null), ); @@ -74,6 +73,7 @@ final class PHUIDiffTwoUpInlineCommentRowScaffold phutil_tag('td', array('class' => 'n'), $left_hidden), phutil_tag('td', $left_attrs, $left_side), phutil_tag('td', array('class' => 'n'), $right_hidden), + phutil_tag('td', array('class' => 'copy')), phutil_tag('td', $right_attrs, $right_side), ); diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 55cc5e8fd3..6ed939a2ee 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -144,6 +144,7 @@ min-width: 0.5%; width: 0.5%; padding: 0; + background: {$lightbluebackground}; } .differential-diff td.new-copy, @@ -178,6 +179,10 @@ should always have a boring grey background. */ overflow: hidden; } +.differential-diff td + td.n { + border-left: 1px solid {$thinblueborder}; +} + .differential-diff td.n::before { content: attr(data-n); } @@ -443,10 +448,6 @@ unselectable. */ -ms-user-select: none; -webkit-user-select: none; user-select: none; -} - -.differential-diff.copy-l > tbody > tr > td, -.differential-diff.copy-r > tbody > tr > td { opacity: 0.5; } @@ -463,7 +464,7 @@ unselectable. */ -ms-user-select: none; -webkit-user-select: none; user-select: none; - opacity: 0.25; + opacity: 0.5; } .differential-diff.copy-r > tbody > tr > td:nth-child(5) { @@ -473,3 +474,12 @@ unselectable. */ user-select: auto; opacity: 1; } + +.differential-diff.copy-l > tbody > tr.inline > td, +.differential-diff.copy-r > tbody > tr.inline > td { + -moz-user-select: none; + -ms-user-select: none; + -webkit-user-select: none; + user-select: none; + opacity: 0.5; +} From 1b832564211258526065aec985c5dd0570ed4df3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 20 Feb 2019 05:06:42 -0800 Subject: [PATCH 27/30] Don't enable the "ScopeEngine" or try to identify scope context for diffs without context Summary: Depends on D20197. Ref T13161. We currently try to build a "ScopeEngine" even for diffs with no context (e.g., `git diff` instead of `git diff -U9999`). Since we don't have any context, we won't really be able to figure out anything useful about scopes. Also, since ScopeEngine is pretty strict about what it accepts, we crash. In these cases, just don't build a ScopeEngine. Test Plan: Viewed a diff I copy/pasted with `git diff` instead of an `arc diff` / `git diff -U99999`, got a sensible diff with no context instead of a fatal. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13161 Differential Revision: https://secure.phabricator.com/D20198 --- .../render/DifferentialChangesetRenderer.php | 22 ++++++++++++++----- .../DifferentialChangesetTwoUpRenderer.php | 2 +- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/applications/differential/render/DifferentialChangesetRenderer.php b/src/applications/differential/render/DifferentialChangesetRenderer.php index b7f78b5b68..26de5cb53b 100644 --- a/src/applications/differential/render/DifferentialChangesetRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetRenderer.php @@ -33,7 +33,7 @@ abstract class DifferentialChangesetRenderer extends Phobject { private $canMarkDone; private $objectOwnerPHID; private $highlightingDisabled; - private $scopeEngine; + private $scopeEngine = false; private $depthOnlyLines; private $oldFile = false; @@ -677,13 +677,23 @@ abstract class DifferentialChangesetRenderer extends Phobject { return $views; } - final protected function getScopeEngine() { - if (!$this->scopeEngine) { - $line_map = $this->getNewLineTextMap(); + if ($this->scopeEngine === false) { + $hunk_starts = $this->getHunkStartLines(); - $scope_engine = id(new PhabricatorDiffScopeEngine()) - ->setLineTextMap($line_map); + // If this change is missing context, don't try to identify scopes, since + // we won't really be able to get anywhere. + $has_multiple_hunks = (count($hunk_starts) > 1); + $has_offset_hunks = (head_key($hunk_starts) != 1); + $missing_context = ($has_multiple_hunks || $has_offset_hunks); + + if ($missing_context) { + $scope_engine = null; + } else { + $line_map = $this->getNewLineTextMap(); + $scope_engine = id(new PhabricatorDiffScopeEngine()) + ->setLineTextMap($line_map); + } $this->scopeEngine = $scope_engine; } diff --git a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php index c37655bb93..7efd29519e 100644 --- a/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetTwoUpRenderer.php @@ -94,7 +94,7 @@ final class DifferentialChangesetTwoUpRenderer $context_text = null; $context_line = null; - if (!$is_last_block) { + if (!$is_last_block && $scope_engine) { $target_line = $new_lines[$ii + $len]['line']; $context_line = $scope_engine->getScopeStart($target_line); if ($context_line !== null) { From abc26aa96f4cfa31ecf6ffda83e4c3a948f21eb4 Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Wed, 20 Feb 2019 10:24:33 -0800 Subject: [PATCH 28/30] Track total time from task creation to task archival Summary: Ref T5401. Depends on D20201. Add timestamps to worker tasks to track task creation, and pass that through to archive tasks. This lets us measure the total time the task spent in the queue, not just the duration it was actually running. Also displays this information in the daemon status console; see screenshot: {F6225726} Test Plan: Stopped daemons, ran `bin/search index --all --background` to create lots of tasks, restarted daemons, observed expected values for `dateCreated` and `epochArchived` in the archive worker table. Also tested the changes to `unarchiveTask` by forcing a search task to permanently fail and then `bin/worker retry`ing it. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley, PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T5401 Differential Revision: https://secure.phabricator.com/D20200 --- .../20190220.daemon_worker.completed.01.sql | 2 ++ .../20190220.daemon_worker.completed.02.sql | 3 +++ .../PhabricatorDaemonConsoleController.php | 25 +++++++++++++++++-- .../storage/PhabricatorWorkerActiveTask.php | 5 ++-- .../storage/PhabricatorWorkerArchiveTask.php | 3 +++ 5 files changed, 34 insertions(+), 4 deletions(-) create mode 100644 resources/sql/autopatches/20190220.daemon_worker.completed.01.sql create mode 100644 resources/sql/autopatches/20190220.daemon_worker.completed.02.sql diff --git a/resources/sql/autopatches/20190220.daemon_worker.completed.01.sql b/resources/sql/autopatches/20190220.daemon_worker.completed.01.sql new file mode 100644 index 0000000000..37f5a89bba --- /dev/null +++ b/resources/sql/autopatches/20190220.daemon_worker.completed.01.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_worker.worker_archivetask + ADD archivedEpoch INT UNSIGNED NULL; diff --git a/resources/sql/autopatches/20190220.daemon_worker.completed.02.sql b/resources/sql/autopatches/20190220.daemon_worker.completed.02.sql new file mode 100644 index 0000000000..f0040576a9 --- /dev/null +++ b/resources/sql/autopatches/20190220.daemon_worker.completed.02.sql @@ -0,0 +1,3 @@ +ALTER TABLE {$NAMESPACE}_worker.worker_activetask + ADD dateCreated int unsigned NOT NULL, + ADD dateModified int unsigned NOT NULL; diff --git a/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php b/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php index a70cde04c4..421008082f 100644 --- a/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php +++ b/src/applications/daemon/controller/PhabricatorDaemonConsoleController.php @@ -31,6 +31,7 @@ final class PhabricatorDaemonConsoleController $completed_info[$class] = array( 'n' => 0, 'duration' => 0, + 'queueTime' => 0, ); } $completed_info[$class]['n']++; @@ -41,16 +42,33 @@ final class PhabricatorDaemonConsoleController // compute utilization. $usage_total += $lease_overhead + ($duration / 1000000); $usage_start = min($usage_start, $completed_task->getDateModified()); + + $date_archived = $completed_task->getArchivedEpoch(); + $queue_seconds = $date_archived - $completed_task->getDateCreated(); + + // Don't measure queue time for tasks that completed in the same + // epoch-second they were created in. + if ($queue_seconds > 0) { + $sec_in_us = phutil_units('1 second in microseconds'); + $queue_us = $queue_seconds * $sec_in_us; + $queue_exclusive_us = $queue_us - $duration; + $queue_exclusive_seconds = $queue_exclusive_us / $sec_in_us; + $rounded = floor($queue_exclusive_seconds); + $completed_info[$class]['queueTime'] += $rounded; + } } $completed_info = isort($completed_info, 'n'); $rows = array(); foreach ($completed_info as $class => $info) { + $duration_avg = new PhutilNumber((int)($info['duration'] / $info['n'])); + $queue_avg = new PhutilNumber((int)($info['queueTime'] / $info['n'])); $rows[] = array( $class, number_format($info['n']), - pht('%s us', new PhutilNumber((int)($info['duration'] / $info['n']))), + pht('%s us', $duration_avg), + pht('%s s', $queue_avg), ); } @@ -98,6 +116,7 @@ final class PhabricatorDaemonConsoleController phutil_tag('em', array(), pht('Queue Utilization (Approximate)')), sprintf('%.1f%%', 100 * $used_time), null, + null, ); } @@ -108,13 +127,15 @@ final class PhabricatorDaemonConsoleController array( pht('Class'), pht('Count'), - pht('Avg'), + pht('Average Duration'), + pht('Average Queue Time'), )); $completed_table->setColumnClasses( array( 'wide', 'n', 'n', + 'n', )); $completed_panel = id(new PHUIObjectBoxView()) diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php index b6bd462df7..7139e39ac3 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php @@ -12,7 +12,6 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask { $config = array( self::CONFIG_IDS => self::IDS_COUNTER, - self::CONFIG_TIMESTAMPS => false, self::CONFIG_KEY_SCHEMA => array( 'taskClass' => array( 'columns' => array('taskClass'), @@ -118,7 +117,9 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask { ->setPriority($this->getPriority()) ->setObjectPHID($this->getObjectPHID()) ->setResult($result) - ->setDuration($duration); + ->setDuration($duration) + ->setDateCreated($this->getDateCreated()) + ->setArchivedEpoch(PhabricatorTime::getNow()); // NOTE: This deletes the active task (this object)! $archive->save(); diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php index 0062d07a84..25a453b47b 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerArchiveTask.php @@ -8,6 +8,7 @@ final class PhabricatorWorkerArchiveTask extends PhabricatorWorkerTask { protected $duration; protected $result; + protected $archivedEpoch; protected function getConfiguration() { $parent = parent::getConfiguration(); @@ -22,6 +23,7 @@ final class PhabricatorWorkerArchiveTask extends PhabricatorWorkerTask { $config[self::CONFIG_COLUMN_SCHEMA] = array( 'result' => 'uint32', 'duration' => 'uint64', + 'archivedEpoch' => 'epoch?', ) + $config[self::CONFIG_COLUMN_SCHEMA]; $config[self::CONFIG_KEY_SCHEMA] = array( @@ -85,6 +87,7 @@ final class PhabricatorWorkerArchiveTask extends PhabricatorWorkerTask { ->setDataID($this->getDataID()) ->setPriority($this->getPriority()) ->setObjectPHID($this->getObjectPHID()) + ->setDateCreated($this->getDateCreated()) ->insert(); $this->setDataID(null); From 90064a350a027eb8b3e37323deb636db96664af6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 22 Feb 2019 14:54:13 -0800 Subject: [PATCH 29/30] Fix URI construction of typeahead browse "more" pager Summary: Ref T13251. See . Test Plan: - With more than 100 projects (or, set `$limit = 3`)... - Edit a task, then click the "Browse" magnifying glass icon next to the "Tags" typeahead. - Before change: fatal on 'q' being null. - After change: no fatal. Reviewers: amckinley, 20after4 Reviewed By: amckinley Maniphest Tasks: T13251 Differential Revision: https://secure.phabricator.com/D20204 --- ...ricatorTypeaheadModularDatasourceController.php | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php b/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php index efc9ea5f65..2d55c5f663 100644 --- a/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php +++ b/src/applications/typeahead/controller/PhabricatorTypeaheadModularDatasourceController.php @@ -127,10 +127,20 @@ final class PhabricatorTypeaheadModularDatasourceController if (($offset + (2 * $limit)) < $hard_limit) { $next_uri = id(new PhutilURI($request->getRequestURI())) ->replaceQueryParam('offset', $offset + $limit) - ->replaceQueryParam('q', $query) - ->replaceQueryParam('raw', $raw_query) ->replaceQueryParam('format', 'html'); + if ($query !== null) { + $next_uri->replaceQueryParam('q', $query); + } else { + $next_uri->removeQueryParam('q'); + } + + if ($raw_query !== null) { + $next_uri->replaceQueryParam('raw', $raw_query); + } else { + $next_uri->removeQueryParam('raw'); + } + $next_link = javelin_tag( 'a', array( From 701a9bc339b9d419326a62e85ef13666b08046cd Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 22 Feb 2019 16:28:43 -0800 Subject: [PATCH 30/30] Fix Facebook login on mobile violating CSP after form redirect Summary: Fixes T13254. See that task for details. Test Plan: Used iOS Simulator to do a login locally, didn't get blocked. Verified CSP includes "m.facebook.com". Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13254 Differential Revision: https://secure.phabricator.com/D20206 --- .../PhabricatorFacebookAuthProvider.php | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/applications/auth/provider/PhabricatorFacebookAuthProvider.php b/src/applications/auth/provider/PhabricatorFacebookAuthProvider.php index e3e1fb43e5..67840727e8 100644 --- a/src/applications/auth/provider/PhabricatorFacebookAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorFacebookAuthProvider.php @@ -47,6 +47,14 @@ final class PhabricatorFacebookAuthProvider return 'Facebook'; } + protected function getContentSecurityPolicyFormActions() { + return array( + // See T13254. After login with a mobile device, Facebook may redirect + // to the mobile site. + 'https://m.facebook.com/', + ); + } + public function readFormValuesFromProvider() { $require_secure = $this->getProviderConfig()->getProperty( self::KEY_REQUIRE_SECURE); @@ -114,15 +122,4 @@ final class PhabricatorFacebookAuthProvider return parent::renderConfigPropertyTransactionTitle($xaction); } - public static function getFacebookApplicationID() { - $providers = PhabricatorAuthProvider::getAllProviders(); - $fb_provider = idx($providers, 'facebook:facebook.com'); - if (!$fb_provider) { - return null; - } - - return $fb_provider->getProviderConfig()->getProperty( - self::PROPERTY_APP_ID); - } - }