1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-18 21:02:41 +01:00

Make "Show Context" persist rendering, whitespace, encoding, etc

Summary:
Ref T2009. Currently, we do not persist view parameters when making context rendering requests.

The big one is the renderer (1up vs 2up). This makes context on unified diffs come in with too many columns.

However, it impacts other parameters too. For example, at HEAD, if you change highlighting to "rainbow" and then load more context, the context uses the original highlighter instead of the rainbow highlighter.

This moves context loads into ChangesetViewManager, which maintains view parameters and can provide them correctly.

  - This removes "ref"; it is no longer required, as the ChangesetViewManager tracks it.
  - This removes URI management from `behavior-show-more`; it is no longer required, since the ChangesetViewManager knows how to render.
  - This removes "whitespace" since this is handled properly by the view manager.

Test Plan:
  - Used "Show Top" / "Show All" / "Show Bottom" in 1-up and 2-up views.
  - Changed file highlighting to rainbow, loaded stuff, saw rainbow stick.
  - Used "Show Entire File" in 1-up and 2-up views.
    - Saw loading chrome.
    - No loading chrome normally.
  - Made inlines, verified `copyRows()` code runs.
  - Poked around Diffusion -- it is missing some parameter handling, but works OK.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2009

Differential Revision: https://secure.phabricator.com/D11977
This commit is contained in:
epriestley 2015-03-05 14:03:00 -08:00
parent df661eca35
commit ad3c94dd45
17 changed files with 252 additions and 159 deletions

View file

@ -11,7 +11,7 @@ return array(
'core.pkg.js' => 'a77025a1',
'darkconsole.pkg.js' => '8ab24e01',
'differential.pkg.css' => 'd8866ed8',
'differential.pkg.js' => '7b5a4aa4',
'differential.pkg.js' => '9e55f9f5',
'diffusion.pkg.css' => '591664fa',
'diffusion.pkg.js' => 'bfc0737b',
'maniphest.pkg.css' => '68d4dd3d',
@ -360,18 +360,18 @@ return array(
'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '82439934',
'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
'rsrc/js/application/differential/ChangesetViewManager.js' => 'c024db3d',
'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => 'f2441746',
'rsrc/js/application/differential/ChangesetViewManager.js' => 'fce415a0',
'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => '6a049cf7',
'rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js' => 'e10f8e18',
'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d',
'rsrc/js/application/differential/behavior-comment-preview.js' => '6932def3',
'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1',
'rsrc/js/application/differential/behavior-dropdown-menus.js' => 'e33d4bc5',
'rsrc/js/application/differential/behavior-dropdown-menus.js' => '2035b9cb',
'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '65936067',
'rsrc/js/application/differential/behavior-keyboard-nav.js' => '2c426492',
'rsrc/js/application/differential/behavior-populate.js' => 'bdb3e4d0',
'rsrc/js/application/differential/behavior-show-field-details.js' => 'bba9eedf',
'rsrc/js/application/differential/behavior-show-more.js' => '954d2de0',
'rsrc/js/application/differential/behavior-show-more.js' => 'c662904a',
'rsrc/js/application/differential/behavior-toggle-files.js' => 'ca3f91eb',
'rsrc/js/application/differential/behavior-user-select.js' => 'a8d8459d',
'rsrc/js/application/diffusion/DiffusionLocateFileSource.js' => 'b42eddc7',
@ -510,7 +510,7 @@ return array(
'aphront-two-column-view-css' => '16ab3ad2',
'aphront-typeahead-control-css' => '0e403212',
'auth-css' => '1e655982',
'changeset-view-manager' => 'c024db3d',
'changeset-view-manager' => 'fce415a0',
'config-options-css' => '7fedf08b',
'config-welcome-css' => '6abd79be',
'conpherence-durable-column-view' => '3b836442',
@ -521,7 +521,7 @@ return array(
'conpherence-widget-pane-css' => '3d575438',
'differential-changeset-view-css' => 'b600950c',
'differential-core-view-css' => '7ac3cabc',
'differential-inline-comment-editor' => 'f2441746',
'differential-inline-comment-editor' => '6a049cf7',
'differential-results-table-css' => '181aa9d9',
'differential-revision-add-comment-css' => 'c478bcaa',
'differential-revision-comment-css' => '48186045',
@ -569,13 +569,13 @@ return array(
'javelin-behavior-differential-add-reviewers-and-ccs' => 'e10f8e18',
'javelin-behavior-differential-comment-jump' => '4fdb476d',
'javelin-behavior-differential-diff-radios' => 'e1ff79b1',
'javelin-behavior-differential-dropdown-menus' => 'e33d4bc5',
'javelin-behavior-differential-dropdown-menus' => '2035b9cb',
'javelin-behavior-differential-edit-inline-comments' => '65936067',
'javelin-behavior-differential-feedback-preview' => '6932def3',
'javelin-behavior-differential-keyboard-navigation' => '2c426492',
'javelin-behavior-differential-populate' => 'bdb3e4d0',
'javelin-behavior-differential-show-field-details' => 'bba9eedf',
'javelin-behavior-differential-show-more' => '954d2de0',
'javelin-behavior-differential-show-more' => 'c662904a',
'javelin-behavior-differential-toggle-files' => 'ca3f91eb',
'javelin-behavior-differential-user-select' => 'a8d8459d',
'javelin-behavior-diffusion-commit-branches' => 'bdaf4d04',
@ -944,6 +944,18 @@ return array(
'javelin-dom',
'javelin-reactor-dom',
),
'2035b9cb' => array(
'javelin-behavior',
'javelin-dom',
'javelin-util',
'javelin-stratcom',
'javelin-workflow',
'phuix-dropdown-menu',
'phuix-action-list-view',
'phuix-action-view',
'phabricator-phtize',
'changeset-view-manager',
),
'2290aeef' => array(
'javelin-install',
'javelin-dom',
@ -1236,6 +1248,14 @@ return array(
'69adf288' => array(
'javelin-install',
),
'6a049cf7' => array(
'javelin-dom',
'javelin-util',
'javelin-stratcom',
'javelin-install',
'javelin-request',
'javelin-workflow',
),
'6c2b09a2' => array(
'javelin-install',
'javelin-util',
@ -1534,13 +1554,6 @@ return array(
'javelin-resource',
'javelin-routable',
),
'954d2de0' => array(
'javelin-behavior',
'javelin-dom',
'javelin-workflow',
'javelin-util',
'javelin-stratcom',
),
'988040b4' => array(
'javelin-install',
'javelin-dom',
@ -1704,16 +1717,6 @@ return array(
'javelin-util',
'phabricator-shaped-request',
),
'c024db3d' => array(
'javelin-dom',
'javelin-util',
'javelin-stratcom',
'javelin-install',
'javelin-workflow',
'javelin-router',
'javelin-behavior-device',
'javelin-vector',
),
'c1700f6f' => array(
'javelin-install',
'javelin-util',
@ -1728,6 +1731,14 @@ return array(
'javelin-stratcom',
'javelin-vector',
),
'c662904a' => array(
'javelin-behavior',
'javelin-dom',
'javelin-workflow',
'javelin-util',
'javelin-stratcom',
'changeset-view-manager',
),
'c90a04fc' => array(
'javelin-dom',
'javelin-dynval',
@ -1827,18 +1838,6 @@ return array(
'javelin-workflow',
'javelin-vector',
),
'e33d4bc5' => array(
'javelin-behavior',
'javelin-dom',
'javelin-util',
'javelin-stratcom',
'javelin-workflow',
'phuix-dropdown-menu',
'phuix-action-list-view',
'phuix-action-view',
'phabricator-phtize',
'changeset-view-manager',
),
'e379b58e' => array(
'javelin-behavior',
'javelin-stratcom',
@ -1893,14 +1892,6 @@ return array(
'javelin-install',
'javelin-util',
),
'f2441746' => array(
'javelin-dom',
'javelin-util',
'javelin-stratcom',
'javelin-install',
'javelin-request',
'javelin-workflow',
),
'f24f3253' => array(
'javelin-behavior',
'javelin-dom',
@ -1991,6 +1982,16 @@ return array(
'javelin-dom',
'phortune-credit-card-form',
),
'fce415a0' => array(
'javelin-dom',
'javelin-util',
'javelin-stratcom',
'javelin-install',
'javelin-workflow',
'javelin-router',
'javelin-behavior-device',
'javelin-vector',
),
'fe287620' => array(
'javelin-install',
'javelin-dom',

View file

@ -154,13 +154,8 @@ final class DifferentialChangesetViewController extends DifferentialController {
$parser->setRenderCacheKey($render_cache_key);
$parser->setRightSideCommentMapping($right_source, $right_new);
$parser->setLeftSideCommentMapping($left_source, $left_new);
$parser->setWhitespaceMode($request->getStr('whitespace'));
$parser->setCharacterEncoding($request->getStr('encoding'));
$parser->setHighlightAs($request->getStr('highlight'));
if ($request->getStr('renderer') == '1up') {
$parser->setRenderer(new DifferentialChangesetOneUpRenderer());
}
$parser->readParametersFromRequest($request);
if ($left && $right) {
$parser->setOriginals($left, $right);
@ -235,6 +230,9 @@ final class DifferentialChangesetViewController extends DifferentialController {
$detail = id(new DifferentialChangesetDetailView())
->setUser($this->getViewer())
->setChangeset($changeset)
->setRenderingRef($rendering_reference)
->setRenderURI('/differential/changeset/')
->setRenderer($parser->getRenderer()->getRendererKey())
->appendChild($output)
->setVsChangesetID($left_source);
@ -242,11 +240,7 @@ final class DifferentialChangesetViewController extends DifferentialController {
'changesetViewIDs' => array($detail->getID()),
));
Javelin::initBehavior('differential-show-more', array(
'uri' => '/differential/changeset/',
'whitespace' => $request->getStr('whitespace'),
));
Javelin::initBehavior('differential-show-more');
Javelin::initBehavior('differential-comment-jump', array());
$panel = new DifferentialPrimaryPaneView();

View file

@ -92,6 +92,44 @@ final class DifferentialChangesetParser {
return $this->disableCache;
}
public static function getDefaultRendererForViewer(PhabricatorUser $viewer) {
$prefs = $viewer->loadPreferences();
$pref_unified = PhabricatorUserPreferences::PREFERENCE_DIFF_UNIFIED;
if ($prefs->getPreference($pref_unified) == 'unified') {
return '1up';
}
return null;
}
public function readParametersFromRequest(AphrontRequest $request) {
$this->setWhitespaceMode($request->getStr('whitespace'));
$this->setCharacterEncoding($request->getStr('encoding'));
$this->setHighlightAs($request->getStr('highlight'));
$renderer = null;
// If the viewer prefers unified diffs, always set the renderer to unified.
// Otherwise, we leave it unspecified and the client will choose a
// renderer based on the screen size.
if ($request->getStr('renderer')) {
$renderer = $request->getStr('renderer');
} else {
$renderer = self::getDefaultRendererForViewer($request->getViewer());
}
switch ($renderer) {
case '1up':
$this->setRenderer(new DifferentialChangesetOneUpRenderer());
break;
default:
$this->setRenderer(new DifferentialChangesetTwoUpRenderer());
break;
}
return $this;
}
const CACHE_VERSION = 11;
const CACHE_MAX_SIZE = 8e6;

View file

@ -519,7 +519,6 @@ abstract class DifferentialChangesetHTMLRenderer
'sigil' => 'show-more',
'meta' => array(
'type' => ($is_all ? 'all' : null),
'ref' => $reference,
'range' => $range,
),
),

View file

@ -11,6 +11,10 @@ final class DifferentialChangesetOneUpRenderer
return 'diff-1up';
}
public function getRendererKey() {
return '1up';
}
protected function renderColgroup() {
return phutil_tag('colgroup', array(), array(
phutil_tag('col', array('class' => 'num')),

View file

@ -7,4 +7,8 @@ final class DifferentialChangesetOneUpTestRenderer
return true;
}
public function getRendererKey() {
return '1up-test';
}
}

View file

@ -34,6 +34,8 @@ abstract class DifferentialChangesetRenderer {
private $oldFile = false;
private $newFile = false;
abstract public function getRendererKey();
public function setShowEditAndReplyLinks($bool) {
$this->showEditAndReplyLinks = $bool;
return $this;

View file

@ -11,6 +11,10 @@ final class DifferentialChangesetTwoUpRenderer
return 'diff-2up';
}
public function getRendererKey() {
return '2up';
}
protected function renderColgroup() {
return phutil_tag('colgroup', array(), array(
phutil_tag('col', array('class' => 'num')),

View file

@ -7,4 +7,8 @@ final class DifferentialChangesetTwoUpTestRenderer
return false;
}
public function getRendererKey() {
return '2up-test';
}
}

View file

@ -12,6 +12,7 @@ final class DifferentialChangesetDetailView extends AphrontView {
private $whitespace;
private $renderingRef;
private $autoload;
private $renderer;
public function setAutoload($autoload) {
$this->autoload = $autoload;
@ -69,6 +70,15 @@ final class DifferentialChangesetDetailView extends AphrontView {
return $this;
}
public function setRenderer($renderer) {
$this->renderer = $renderer;
return $this;
}
public function getRenderer() {
return $this->renderer;
}
public function getID() {
if (!$this->id) {
$this->id = celerity_generate_unique_node_id();
@ -188,19 +198,6 @@ final class DifferentialChangesetDetailView extends AphrontView {
$icon = id(new PHUIIconView())
->setIconFont($display_icon);
$renderer = null;
// If the viewer prefers unified diffs, always set the renderer to unified.
// Otherwise, we leave it unspecified and the client will choose a
// renderer based on the screen size.
$viewer = $this->getUser();
$prefs = $viewer->loadPreferences();
$pref_unified = PhabricatorUserPreferences::PREFERENCE_DIFF_UNIFIED;
if ($prefs->getPreference($pref_unified) == 'unified') {
$renderer = '1up';
}
return javelin_tag(
'div',
array(
@ -213,7 +210,7 @@ final class DifferentialChangesetDetailView extends AphrontView {
'renderURI' => $this->getRenderURI(),
'whitespace' => $this->getWhitespace(),
'highlight' => null,
'renderer' => $renderer,
'renderer' => $this->getRenderer(),
'ref' => $this->getRenderingRef(),
'autoload' => $this->getAutoload(),
),

View file

@ -136,6 +136,9 @@ final class DifferentialChangesetListView extends AphrontView {
),
));
$renderer = DifferentialChangesetParser::getDefaultRendererForViewer(
$this->getUser());
$output = array();
$ids = array();
foreach ($changesets as $key => $changeset) {
@ -169,6 +172,7 @@ final class DifferentialChangesetListView extends AphrontView {
$detail->setRenderURI($this->renderURI);
$detail->setWhitespace($this->whitespace);
$detail->setRenderer($renderer);
if (isset($this->visibleChangesets[$key])) {
$load = 'Loading...';
@ -205,11 +209,7 @@ final class DifferentialChangesetListView extends AphrontView {
'changesetViewIDs' => $ids,
));
$this->initBehavior('differential-show-more', array(
'uri' => $this->renderURI,
'whitespace' => $this->whitespace,
));
$this->initBehavior('differential-show-more');
$this->initBehavior('differential-comment-jump', array());
if ($this->inlineURI) {

View file

@ -97,6 +97,8 @@ final class PhrictionDiffController extends PhrictionController {
$output = id(new DifferentialChangesetDetailView())
->setUser($this->getViewer())
->setChangeset($changeset)
->setRenderingRef("{$l},{$r}")
->setRenderURI('/phriction/diff/'.$document->getID().'/')
->appendChild($output);
require_celerity_resource('differential-changeset-view-css');
@ -106,11 +108,7 @@ final class PhrictionDiffController extends PhrictionController {
Javelin::initBehavior('differential-populate', array(
'changesetViewIDs' => array($output->getID()),
));
Javelin::initBehavior('differential-show-more', array(
'uri' => '/phriction/diff/'.$document->getID().'/',
'whitespace' => $whitespace_mode,
));
Javelin::initBehavior('differential-show-more');
$slug = $document->getSlug();

View file

@ -948,8 +948,8 @@ final class PhabricatorUSEnglishTranslation
),
"\xE2\x96\xB2 Show %d Line(s)" => array(
"\xE2\x96\xB2 Show %d Line(s)",
"\xE2\x96\xB2 Show %d Line(s)",
"\xE2\x96\xB2 Show Line",
"\xE2\x96\xB2 Show %d Lines",
),
'Show All %d Line(s)' => array(

View file

@ -118,25 +118,12 @@ JX.install('ChangesetViewManager', {
this._loaded = true;
this._sequence++;
var params = {
ref: this._ref,
whitespace: this._whitespace || '',
renderer: this.getRenderer() || '',
highlight: this._highlight || '',
encoding: this._encoding || ''
};
var params = this._getViewParameters();
var workflow = new JX.Workflow(this._renderURI, params)
.setHandler(JX.bind(this, this._onresponse, this._sequence));
var routable = workflow.getRoutable();
routable
.setPriority(500)
.setType('content')
.setKey(this._getRoutableKey());
JX.Router.getInstance().queue(routable);
this._startContentWorkflow(workflow);
JX.DOM.setContent(
this._getContentFrame(),
@ -148,6 +135,95 @@ JX.install('ChangesetViewManager', {
return this;
},
/**
* Load missing context in a changeset.
*
* We do this when the user clicks "Show X Lines". We also expand all of
* the missing context when they "Show Entire File".
*
* @param string Line range specification, like "0-40/0-20".
* @param node Row where the context should be rendered after loading.
* @param bool True if this is a bulk load of multiple context blocks.
* @return this
*/
loadContext: function(range, target, bulk) {
var params = this._getViewParameters();
params.range = range;
var container = JX.DOM.scry(target, 'td')[0];
// TODO: pht()
JX.DOM.setContent(container, 'Loading...');
JX.DOM.alterClass(target, 'differential-show-more-loading', true);
var workflow = new JX.Workflow(this._renderURI, params)
.setHandler(JX.bind(this, this._oncontext, target));
if (bulk) {
// If we're loading a bunch of these because the viewer clicked
// "Show Entire File Content" or similar, use lower-priority requests
// and draw a progress bar.
this._startContentWorkflow(workflow);
} else {
// If this is a single click on a context link, use a higher priority
// load without a chrome change.
workflow.start();
}
return this;
},
_startContentWorkflow: function(workflow) {
var routable = workflow.getRoutable();
routable
.setPriority(500)
.setType('content')
.setKey(this._getRoutableKey());
JX.Router.getInstance().queue(routable);
},
/**
* Receive a response to a context request.
*/
_oncontext: function(target, response) {
var table = JX.$H(response.changeset).getNode();
var root = target.parentNode;
this._moveRows(table, root, target);
root.removeChild(target);
},
_moveRows: function(src, dst, before) {
var rows = JX.DOM.scry(src, 'tr');
for (var ii = 0; ii < rows.length; ii++) {
// Find the table this <tr /> belongs to. If it's a sub-table, like a
// table in an inline comment, don't copy it.
if (JX.DOM.findAbove(rows[ii], 'table') !== src) {
continue;
}
if (before) {
dst.insertBefore(rows[ii], before);
} else {
dst.appendChild(rows[ii]);
}
}
},
/**
* Get parameters which define the current rendering options.
*/
_getViewParameters: function() {
return {
ref: this._ref,
whitespace: this._whitespace || '',
renderer: this.getRenderer() || '',
highlight: this._highlight || '',
encoding: this._encoding || ''
};
},
/**
* Get the active @{class:JX.Routable} for this changeset.
@ -176,9 +252,7 @@ JX.install('ChangesetViewManager', {
// a different one we don't re-render the diffs, because it's a
// complicated mess and you could lose inline comments, cursor positions,
// etc.
var renderer = (JX.Device.getDevice() == 'desktop') ? '2up' : '1up';
return renderer;
return (JX.Device.getDevice() == 'desktop') ? '2up' : '1up';
},
setEncoding: function(encoding) {

View file

@ -43,6 +43,25 @@ JX.install('DifferentialInlineCommentEditor', {
var table = this.getTable();
var target = exact_row ? row : this._skipOverInlineCommentRows(row);
function copyRows(dst, src, before) {
var rows = JX.DOM.scry(src, 'tr');
for (var ii = 0; ii < rows.length; ii++) {
// Find the table this <tr /> belongs to. If it's a sub-table, like a
// table in an inline comment, don't copy it.
if (JX.DOM.findAbove(rows[ii], 'table') !== src) {
continue;
}
if (before) {
dst.insertBefore(rows[ii], before);
} else {
dst.appendChild(rows[ii]);
}
}
return rows;
}
return copyRows(table, content, target);
},
_removeUndoLink : function() {

View file

@ -16,18 +16,17 @@ JX.behavior('differential-dropdown-menus', function(config) {
var pht = JX.phtize(config.pht);
function show_more(container) {
var view = JX.ChangesetViewManager.getForNode(container);
var nodes = JX.DOM.scry(container, 'tr', 'context-target');
for (var ii = 0; ii < nodes.length; ii++) {
var show = JX.DOM.scry(nodes[ii], 'a', 'show-more');
for (var jj = 0; jj < show.length; jj++) {
if (JX.Stratcom.getData(show[jj]).type != 'all') {
var data = JX.Stratcom.getData(show[jj]);
if (data.type != 'all') {
continue;
}
var event_data = {
context : nodes[ii],
show : show[jj]
};
JX.Stratcom.invoke('differential-reveal-context', null, event_data);
view.loadContext(data.range, nodes[ii], true);
}
}
}

View file

@ -5,67 +5,23 @@
* javelin-workflow
* javelin-util
* javelin-stratcom
* changeset-view-manager
*/
JX.behavior('differential-show-more', function(config) {
function onresponse(context, response) {
var table = JX.$H(response.changeset).getNode();
var root = context.parentNode;
copyRows(root, table, context);
root.removeChild(context);
}
JX.behavior('differential-show-more', function() {
JX.Stratcom.listen(
'click',
'show-more',
function(e) {
var event_data = {
context : e.getNodes()['context-target'],
show : e.getNodes()['show-more']
};
JX.Stratcom.invoke('differential-reveal-context', null, event_data);
e.kill();
});
JX.Stratcom.listen(
'differential-reveal-context',
null,
function(e) {
var context = e.getData().context;
var data = JX.Stratcom.getData(e.getData().show);
var changeset = e.getNode('differential-changeset');
var view = JX.ChangesetViewManager.getForNode(changeset);
var data = e.getNodeData('show-more');
var target = e.getNode('context-target');
var container = JX.DOM.scry(context, 'td')[0];
JX.DOM.setContent(container, 'Loading...');
JX.DOM.alterClass(context, 'differential-show-more-loading', true);
if (!data.whitespace) {
data.whitespace = config.whitespace;
}
new JX.Workflow(config.uri, data)
.setHandler(JX.bind(null, onresponse, context))
.start();
view.loadContext(data.range, target);
});
});
function copyRows(dst, src, before) {
var rows = JX.DOM.scry(src, 'tr');
for (var ii = 0; ii < rows.length; ii++) {
// Find the table this <tr /> belongs to. If it's a sub-table, like a
// table in an inline comment, don't copy it.
if (JX.DOM.findAbove(rows[ii], 'table') !== src) {
continue;
}
if (before) {
dst.insertBefore(rows[ii], before);
} else {
dst.appendChild(rows[ii]);
}
}
return rows;
}