diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -836,7 +836,17 @@ unset($submit_fields[$key]); continue; } + } + + // Before we read the submitted values, store a copy of what we would + // use if the form was empty so we can figure out which transactions are + // just setting things to their default values for the current form. + $defaults = array(); + foreach ($submit_fields as $key => $field) { + $defaults[$key] = $field->getValueForTransaction(); + } + foreach ($submit_fields as $key => $field) { $field->setIsSubmittedForm(true); if (!$field->shouldReadValueFromSubmit()) { @@ -847,14 +857,22 @@ } $xactions = array(); - foreach ($submit_fields as $field) { + foreach ($submit_fields as $key => $field) { + $field_value = $field->getValueForTransaction(); + $type_xactions = $field->generateTransactions( clone $template, array( - 'value' => $field->getValueForTransaction(), + 'value' => $field_value, )); foreach ($type_xactions as $type_xaction) { + $default = $defaults[$key]; + + if ($default === $field->getValueForTransaction()) { + $type_xaction->setIsDefaultTransaction(true); + } + $xactions[] = $type_xaction; } } diff --git a/src/applications/transactions/editfield/PhabricatorEditField.php b/src/applications/transactions/editfield/PhabricatorEditField.php --- a/src/applications/transactions/editfield/PhabricatorEditField.php +++ b/src/applications/transactions/editfield/PhabricatorEditField.php @@ -755,8 +755,4 @@ return $edit_type->generateTransactions($template, $spec); } - - - - } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -823,6 +823,12 @@ throw $ex; } + foreach ($xactions as $xaction) { + if ($this->getIsNewObject()) { + $xaction->setIsCreateTransaction(true); + } + } + // Now that we've merged, filtered, and combined transactions, check for // required capabilities. foreach ($xactions as $xaction) { diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -142,6 +142,22 @@ return $this->comment; } + public function setIsCreateTransaction($create) { + return $this->setMetadataValue('core.create', $create); + } + + public function getIsCreateTransaction() { + return (bool)$this->getMetadataValue('core.create', false); + } + + public function setIsDefaultTransaction($default) { + return $this->setMetadataValue('core.default', $default); + } + + public function getIsDefaultTransaction() { + return (bool)$this->getMetadataValue('core.default', false); + } + public function attachComment( PhabricatorApplicationTransactionComment $comment) { $this->comment = $comment; @@ -456,6 +472,39 @@ } public function shouldHide() { + // Never hide comments. + if ($this->hasComment()) { + return false; + } + + // Hide creation transactions if the old value is empty. These are + // transactions like "alice set the task tile to: ...", which are + // essentially never interesting. + if ($this->getIsCreateTransaction()) { + $old = $this->getOldValue(); + + if (is_array($old) && !$old) { + return true; + } + + if (!strlen($old)) { + return true; + } + } + + // Hide creation transactions setting values to defaults, even if + // the old value is not empty. For example, tasks may have a global + // default view policy of "All Users", but a particular form sets the + // policy to "Administrators". The transaction corresponding to this + // change is not interesting, since it is the default behavior of the + // form. + + if ($this->getIsCreateTransaction()) { + if ($this->getIsDefaultTransaction()) { + return true; + } + } + switch ($this->getTransactionType()) { case PhabricatorTransactions::TYPE_VIEW_POLICY: case PhabricatorTransactions::TYPE_EDIT_POLICY: