1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-09 16:32:39 +01:00

Make "Highlight As..." sticky across reloads in Diffusion and Differential

Summary:
Ref T13455. Add container-level storage for persistent view state, and persist "Highlight As..." inside it.

The storage generates a "PhabricatorChangesetViewState" configuration object as an output.

When preferences are expressed on a diff and that diff is later attached to a revision, we attempt to copy the preferences.

The internal storage tracks per-changeset settings, but currently always uses "last update wins" to apply the settings in the UI.

Test Plan:
  - Viewed revisions, changed highlighting, reloaded. Saw highlighting stick in revision view and standalone view.
  - Viewed commits, changed highlighting, reloaded. Saw highlighting stick.
  - Created a diff, changed highlighting, turned it into a revision, saw highlighting persist.

Subscribers: jmeador, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13455

Differential Revision: https://secure.phabricator.com/D21137
This commit is contained in:
epriestley 2020-04-17 11:34:36 -07:00
parent 3adf082002
commit 8aac55cc57
13 changed files with 469 additions and 45 deletions

View file

@ -0,0 +1,9 @@
CREATE TABLE {$NAMESPACE}_differential.differential_viewstate (
id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
viewerPHID VARBINARY(64) NOT NULL,
objectPHID VARBINARY(64) NOT NULL,
viewState LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT},
dateCreated INT UNSIGNED NOT NULL,
dateModified INT UNSIGNED NOT NULL,
UNIQUE KEY `key_viewer` (viewerPHID, objectPHID)
) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT};

View file

@ -713,6 +713,8 @@ phutil_register_library_map(array(
'DifferentialUnitStatus' => 'applications/differential/constants/DifferentialUnitStatus.php',
'DifferentialUnitTestResult' => 'applications/differential/constants/DifferentialUnitTestResult.php',
'DifferentialUpdateRevisionConduitAPIMethod' => 'applications/differential/conduit/DifferentialUpdateRevisionConduitAPIMethod.php',
'DifferentialViewState' => 'applications/differential/storage/DifferentialViewState.php',
'DifferentialViewStateQuery' => 'applications/differential/query/DifferentialViewStateQuery.php',
'DiffusionAuditorDatasource' => 'applications/diffusion/typeahead/DiffusionAuditorDatasource.php',
'DiffusionAuditorFunctionDatasource' => 'applications/diffusion/typeahead/DiffusionAuditorFunctionDatasource.php',
'DiffusionAuditorsAddAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddAuditorsHeraldAction.php',
@ -2735,6 +2737,8 @@ phutil_register_library_map(array(
'PhabricatorChangePasswordUserLogType' => 'applications/people/userlog/PhabricatorChangePasswordUserLogType.php',
'PhabricatorChangesetCachePurger' => 'applications/cache/purger/PhabricatorChangesetCachePurger.php',
'PhabricatorChangesetResponse' => 'infrastructure/diff/PhabricatorChangesetResponse.php',
'PhabricatorChangesetViewState' => 'infrastructure/diff/viewstate/PhabricatorChangesetViewState.php',
'PhabricatorChangesetViewStateEngine' => 'infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php',
'PhabricatorChartAxis' => 'applications/fact/chart/PhabricatorChartAxis.php',
'PhabricatorChartDataQuery' => 'applications/fact/chart/PhabricatorChartDataQuery.php',
'PhabricatorChartDataset' => 'applications/fact/chart/PhabricatorChartDataset.php',
@ -6783,6 +6787,11 @@ phutil_register_library_map(array(
'DifferentialUnitStatus' => 'Phobject',
'DifferentialUnitTestResult' => 'Phobject',
'DifferentialUpdateRevisionConduitAPIMethod' => 'DifferentialConduitAPIMethod',
'DifferentialViewState' => array(
'DifferentialDAO',
'PhabricatorPolicyInterface',
),
'DifferentialViewStateQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
'DiffusionAuditorDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
'DiffusionAuditorFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource',
'DiffusionAuditorsAddAuditorsHeraldAction' => 'DiffusionAuditorsHeraldAction',
@ -9130,6 +9139,8 @@ phutil_register_library_map(array(
'PhabricatorChangePasswordUserLogType' => 'PhabricatorUserLogType',
'PhabricatorChangesetCachePurger' => 'PhabricatorCachePurger',
'PhabricatorChangesetResponse' => 'AphrontProxyResponse',
'PhabricatorChangesetViewState' => 'Phobject',
'PhabricatorChangesetViewStateEngine' => 'Phobject',
'PhabricatorChartAxis' => 'Phobject',
'PhabricatorChartDataQuery' => 'Phobject',
'PhabricatorChartDataset' => 'Phobject',

View file

@ -89,13 +89,16 @@ final class DifferentialParseRenderTestCase extends PhabricatorTestCase {
$engine = new PhabricatorMarkupEngine();
$engine->setViewer(new PhabricatorUser());
$viewstate = new PhabricatorChangesetViewState();
$parsers = array();
foreach ($changesets as $changeset) {
$cparser = new DifferentialChangesetParser();
$cparser->setUser(new PhabricatorUser());
$cparser->setDisableCache(true);
$cparser->setChangeset($changeset);
$cparser->setMarkupEngine($engine);
$cparser = id(new DifferentialChangesetParser())
->setViewer(new PhabricatorUser())
->setDisableCache(true)
->setChangeset($changeset)
->setMarkupEngine($engine)
->setViewState($viewstate);
if ($type == 'one') {
$cparser->setRenderer(new DifferentialChangesetOneUpTestRenderer());

View file

@ -165,21 +165,6 @@ final class DifferentialChangesetViewController extends DifferentialController {
list($range_s, $range_e, $mask) =
DifferentialChangesetParser::parseRangeSpecification($spec);
$parser = id(new DifferentialChangesetParser())
->setViewer($viewer)
->setCoverage($coverage)
->setChangeset($changeset)
->setRenderingReference($rendering_reference)
->setRenderCacheKey($render_cache_key)
->setRightSideCommentMapping($right_source, $right_new)
->setLeftSideCommentMapping($left_source, $left_new);
$parser->readParametersFromRequest($request);
if ($left && $right) {
$parser->setOriginals($left, $right);
}
$diff = $changeset->getDiff();
$revision_id = $diff->getRevisionID();
@ -197,6 +182,35 @@ final class DifferentialChangesetViewController extends DifferentialController {
}
}
if ($revision) {
$container_phid = $revision->getPHID();
} else {
$container_phid = $diff->getPHID();
}
$viewstate_engine = id(new PhabricatorChangesetViewStateEngine())
->setViewer($viewer)
->setObjectPHID($container_phid)
->setChangeset($changeset);
$viewstate = $viewstate_engine->newViewStateFromRequest($request);
$parser = id(new DifferentialChangesetParser())
->setViewer($viewer)
->setViewState($viewstate)
->setCoverage($coverage)
->setChangeset($changeset)
->setRenderingReference($rendering_reference)
->setRenderCacheKey($render_cache_key)
->setRightSideCommentMapping($right_source, $right_new)
->setLeftSideCommentMapping($left_source, $left_new);
$parser->readParametersFromRequest($request);
if ($left && $right) {
$parser->setOriginals($left, $right);
}
// Load both left-side and right-side inline comments.
if ($revision) {
$query = id(new DifferentialInlineCommentQuery())
@ -249,7 +263,7 @@ final class DifferentialChangesetViewController extends DifferentialController {
$engine->process();
$parser
->setUser($viewer)
->setViewer($viewer)
->setMarkupEngine($engine)
->setShowEditAndReplyLinks(true)
->setCanMarkDone($can_mark)

View file

@ -45,7 +45,6 @@ final class DifferentialChangesetParser extends Phobject {
private $disableCache;
private $renderer;
private $characterEncoding;
private $highlightAs;
private $highlightingDisabled;
private $showEditAndReplyLinks = true;
private $canMarkDone;
@ -61,6 +60,8 @@ final class DifferentialChangesetParser extends Phobject {
private $viewer;
private $documentEngineKey;
private $viewState;
public function setRange($start, $end) {
$this->rangeStart = $start;
$this->rangeEnd = $end;
@ -85,13 +86,13 @@ final class DifferentialChangesetParser extends Phobject {
return $this->showEditAndReplyLinks;
}
public function setHighlightAs($highlight_as) {
$this->highlightAs = $highlight_as;
public function setViewState(PhabricatorChangesetViewState $view_state) {
$this->viewState = $view_state;
return $this;
}
public function getHighlightAs() {
return $this->highlightAs;
public function getViewState() {
return $this->viewState;
}
public function setCharacterEncoding($character_encoding) {
@ -183,7 +184,6 @@ final class DifferentialChangesetParser extends Phobject {
public function readParametersFromRequest(AphrontRequest $request) {
$this->setCharacterEncoding($request->getStr('encoding'));
$this->setHighlightAs($request->getStr('highlight'));
$this->setDocumentEngineKey($request->getStr('engine'));
$renderer = null;
@ -378,15 +378,6 @@ final class DifferentialChangesetParser extends Phobject {
return $this;
}
public function setUser(PhabricatorUser $user) {
$this->user = $user;
return $this;
}
public function getUser() {
return $this->user;
}
public function setCoverage($coverage) {
$this->coverage = $coverage;
return $this;
@ -604,7 +595,7 @@ final class DifferentialChangesetParser extends Phobject {
}
private function getHighlightFuture($corpus) {
$language = $this->highlightAs;
$language = $this->getViewState()->getHighlightLanguage();
if (!$language) {
$language = $this->highlightEngine->getLanguageFromFilename(
@ -634,6 +625,8 @@ final class DifferentialChangesetParser extends Phobject {
}
private function tryCacheStuff() {
$viewstate = $this->getViewState();
$skip_cache = false;
if ($this->disableCache) {
@ -644,7 +637,8 @@ final class DifferentialChangesetParser extends Phobject {
$skip_cache = true;
}
if ($this->highlightAs) {
$highlight_language = $viewstate->getHighlightLanguage();
if ($highlight_language !== null) {
$skip_cache = true;
}
@ -844,7 +838,7 @@ final class DifferentialChangesetParser extends Phobject {
count($this->new));
$renderer = $this->getRenderer()
->setUser($this->getUser())
->setUser($this->getViewer())
->setChangeset($this->changeset)
->setRenderPropertyChangeHeader($render_pch)
->setIsTopLevel($this->isTopLevel)

View file

@ -0,0 +1,64 @@
<?php
final class DifferentialViewStateQuery
extends PhabricatorCursorPagedPolicyAwareQuery {
private $ids;
private $viewerPHIDs;
private $objectPHIDs;
public function withIDs(array $ids) {
$this->ids = $ids;
return $this;
}
public function withViewerPHIDs(array $phids) {
$this->viewerPHIDs = $phids;
return $this;
}
public function withObjectPHIDs(array $phids) {
$this->objectPHIDs = $phids;
return $this;
}
public function newResultObject() {
return new DifferentialViewState();
}
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->viewerPHIDs !== null) {
$where[] = qsprintf(
$conn,
'viewerPHID IN (%Ls)',
$this->viewerPHIDs);
}
if ($this->objectPHIDs !== null) {
$where[] = qsprintf(
$conn,
'objectPHID IN (%Ls)',
$this->objectPHIDs);
}
return $where;
}
public function getQueryApplicationClass() {
return 'PhabricatorDifferentialApplication';
}
}

View file

@ -716,6 +716,8 @@ final class DifferentialDiff
public function destroyObjectPermanently(
PhabricatorDestructionEngine $engine) {
$viewer = $engine->getViewer();
$this->openTransaction();
$this->delete();
@ -730,6 +732,13 @@ final class DifferentialDiff
$prop->delete();
}
$viewstates = id(new DifferentialViewStateQuery())
->setViewer($viewer)
->withObjectPHIDs(array($this->getPHID()));
foreach ($viewstates as $viewstate) {
$viewstate->delete();
}
$this->saveTransaction();
}

View file

@ -1002,9 +1002,11 @@ final class DifferentialRevision extends DifferentialDAO
public function destroyObjectPermanently(
PhabricatorDestructionEngine $engine) {
$viewer = $engine->getViewer();
$this->openTransaction();
$diffs = id(new DifferentialDiffQuery())
->setViewer($engine->getViewer())
->setViewer($viewer)
->withRevisionIDs(array($this->getID()))
->execute();
foreach ($diffs as $diff) {
@ -1022,6 +1024,13 @@ final class DifferentialRevision extends DifferentialDAO
$dummy_path->getTableName(),
$this->getID());
$viewstates = id(new DifferentialViewStateQuery())
->setViewer($viewer)
->withObjectPHIDs(array($this->getPHID()));
foreach ($viewstates as $viewstate) {
$viewstate->delete();
}
$this->delete();
$this->saveTransaction();
}

View file

@ -0,0 +1,132 @@
<?php
final class DifferentialViewState
extends DifferentialDAO
implements PhabricatorPolicyInterface {
protected $viewerPHID;
protected $objectPHID;
protected $viewState = array();
private $hasModifications;
protected function getConfiguration() {
return array(
self::CONFIG_SERIALIZATION => array(
'viewState' => self::SERIALIZATION_JSON,
),
self::CONFIG_KEY_SCHEMA => array(
'key_viewer' => array(
'columns' => array('viewerPHID', 'objectPHID'),
'unique' => true,
),
'key_object' => array(
'columns' => array('objectPHID'),
),
),
) + parent::getConfiguration();
}
public function setChangesetProperty(
DifferentialChangeset $changeset,
$key,
$value) {
if ($this->getChangesetProperty($changeset, $key) === $value) {
return;
}
$properties = array(
'value' => $value,
'epoch' => PhabricatorTime::getNow(),
);
$diff_id = $changeset->getDiffID();
if ($diff_id !== null) {
$properties['diffID'] = (int)$diff_id;
}
$path_hash = $this->getChangesetPathHash($changeset);
$changeset_phid = $this->getChangesetKey($changeset);
$this->hasModifications = true;
$this->viewState['changesets'][$path_hash][$key][$changeset_phid] =
$properties;
}
public function getChangesetProperty(
DifferentialChangeset $changeset,
$key,
$default = null) {
$path_hash = $this->getChangesetPathHash($changeset);
$entries = idxv($this->viewState, array('changesets', $path_hash, $key));
if (!is_array($entries)) {
$entries = array();
}
$entries = isort($entries, 'epoch');
$entry = last($entries);
if (!is_array($entry)) {
$entry = array();
}
return idx($entry, 'value', $default);
}
public function getHasModifications() {
return $this->hasModifications;
}
private function getChangesetPathHash(DifferentialChangeset $changeset) {
$path = $changeset->getFilename();
return PhabricatorHash::digestForIndex($path);
}
private function getChangesetKey(DifferentialChangeset $changeset) {
$key = $changeset->getID();
if ($key === null) {
return '*';
}
return (string)$key;
}
public static function copyViewStatesToObject($src_phid, $dst_phid) {
$table = new self();
$conn = $table->establishConnection('w');
queryfx(
$conn,
'INSERT IGNORE INTO %R
(viewerPHID, objectPHID, viewState, dateCreated, dateModified)
SELECT viewerPHID, %s, viewState, dateCreated, dateModified
FROM %R WHERE objectPHID = %s',
$table,
$dst_phid,
$table,
$src_phid);
}
/* -( PhabricatorPolicyInterface )----------------------------------------- */
public function getCapabilities() {
return array(
PhabricatorPolicyCapability::CAN_VIEW,
);
}
public function getPolicy($capability) {
return PhabricatorPolicies::POLICY_NOONE;
}
public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
return ($viewer->getPHID() === $this->getViewerPHID());
}
}

View file

@ -100,6 +100,14 @@ final class DifferentialRevisionUpdateTransaction
HarbormasterMessageType::BUILDABLE_CONTAINER,
true);
}
// See T13455. If users have set view properites on a diff and the diff
// is then attached to a revision, attempt to copy their view preferences
// to the revision.
DifferentialViewState::copyViewStatesToObject(
$diff->getPHID(),
$object->getPHID());
}
public function getColor() {

View file

@ -60,9 +60,20 @@ final class DiffusionDiffController extends DiffusionController {
return new Aphront404Response();
}
$parser = new DifferentialChangesetParser();
$parser->setUser($viewer);
$parser->setChangeset($changeset);
$commit = $drequest->loadCommit();
$viewstate_engine = id(new PhabricatorChangesetViewStateEngine())
->setViewer($viewer)
->setObjectPHID($commit->getPHID())
->setChangeset($changeset);
$viewstate = $viewstate_engine->newViewStateFromRequest($request);
$parser = id(new DifferentialChangesetParser())
->setViewer($viewer)
->setChangeset($changeset)
->setViewState($viewstate);
$parser->setRenderingReference($drequest->generateURI(
array(
'action' => 'rendering-ref',
@ -75,8 +86,6 @@ final class DiffusionDiffController extends DiffusionController {
$parser->setCoverage($coverage);
}
$commit = $drequest->loadCommit();
$pquery = new DiffusionPathIDQuery(array($changeset->getFilename()));
$ids = $pquery->loadPathIDs();
$path_id = $ids[$changeset->getFilename()];

View file

@ -0,0 +1,17 @@
<?php
final class PhabricatorChangesetViewState
extends Phobject {
private $highlightLanguage;
public function setHighlightLanguage($highlight_language) {
$this->highlightLanguage = $highlight_language;
return $this;
}
public function getHighlightLanguage() {
return $this->highlightLanguage;
}
}

View file

@ -0,0 +1,145 @@
<?php
final class PhabricatorChangesetViewStateEngine
extends Phobject {
private $viewer;
private $objectPHID;
private $changeset;
private $storage;
public function setViewer(PhabricatorUser $viewer) {
$this->viewer = $viewer;
return $this;
}
public function getViewer() {
return $this->viewer;
}
public function setObjectPHID($object_phid) {
$this->objectPHID = $object_phid;
return $this;
}
public function getObjectPHID() {
return $this->objectPHID;
}
public function setChangeset(DifferentialChangeset $changeset) {
$this->changeset = $changeset;
return $this;
}
public function getChangeset() {
return $this->changeset;
}
public function newViewStateFromRequest(AphrontRequest $request) {
$storage = $this->loadViewStateStorage();
$this->setStorage($storage);
$highlight = $request->getStr('highlight');
if ($highlight !== null && strlen($highlight)) {
$this->setChangesetProperty('highlight', $highlight);
}
$this->saveViewStateStorage();
$state = new PhabricatorChangesetViewState();
$highlight_language = $this->getChangesetProperty('highlight');
$state->setHighlightLanguage($highlight_language);
return $state;
}
private function setStorage(DifferentialViewState $storage) {
$this->storage = $storage;
return $this;
}
private function getStorage() {
return $this->storage;
}
private function setChangesetProperty(
$key,
$value) {
$storage = $this->getStorage();
$changeset = $this->getChangeset();
$storage->setChangesetProperty($changeset, $key, $value);
}
private function getChangesetProperty(
$key,
$default = null) {
$storage = $this->getStorage();
$changeset = $this->getChangeset();
return $storage->getChangesetProperty($changeset, $key, $default);
}
private function loadViewStateStorage() {
$viewer = $this->getViewer();
$object_phid = $this->getObjectPHID();
$viewer_phid = $viewer->getPHID();
$storage = null;
if ($viewer_phid !== null) {
$storage = id(new DifferentialViewStateQuery())
->setViewer($viewer)
->withViewerPHIDs(array($viewer_phid))
->withObjectPHIDs(array($object_phid))
->executeOne();
}
if ($storage === null) {
$storage = id(new DifferentialViewState())
->setObjectPHID($object_phid);
if ($viewer_phid !== null) {
$storage->setViewerPHID($viewer_phid);
} else {
$storage->makeEphemeral();
}
}
return $storage;
}
private function saveViewStateStorage() {
if (PhabricatorEnv::isReadOnly()) {
return;
}
$storage = $this->getStorage();
$viewer_phid = $storage->getViewerPHID();
if ($viewer_phid === null) {
return;
}
if (!$storage->getHasModifications()) {
return;
}
$unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
try {
$storage->save();
} catch (AphrontDuplicateKeyQueryException $ex) {
// We may race another process to save view state. For now, just discard
// our state if we do.
}
unset($unguarded);
}
}