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

Authored by epriestley on Mar 5 2015, 3:13 PM.

Details

Reviewers

btrahan

Maniphest Tasks

T2009: Refactor DifferentialChangesetParser to support 1-up views and sanity

Commits

Restricted Diffusion Commit
rPad3c94dd454f: 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.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley updated this revision to Diff 28828.Mar 5 2015, 3:13 PM

epriestley retitled this revision from to Make "Show Context" persist rendering, whitespace, encoding, etc.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: btrahan.

epriestley added a task: T2009: Refactor DifferentialChangesetParser to support 1-up views and sanity.

Herald added a subscriber: epriestley. · View Herald TranscriptMar 5 2015, 3:13 PM

This breaks Phriction, which is doing its own thing.

epriestley mentioned this in D11978: Always run "behavior-populate" before "behavior-show-more".Mar 5 2015, 3:38 PM

Fix Phriction and standalone diff views.
There are still some limitations in Diffusion/Phriction, but I believe this makes nothing worse than it was before.

Nothing important in the inlines.

webroot/rsrc/js/application/differential/ChangesetViewManager.js
221–224	I'm surprised these aren't null, but I'm guessing some JS abstraction makes '' the right value? (JX.Workflow.newFromForm(form, params) maybe?)
webroot/rsrc/js/application/differential/behavior-show-more.js
1	This file looks v. nice on the right hand side

This revision is now accepted and ready to land.Mar 5 2015, 8:54 PM

epriestley added inline comments.Mar 5 2015, 9:20 PM

webroot/rsrc/js/application/differential/ChangesetViewManager.js
221–224	They get used as URL parameters later, and `null` turns into literal "null" and `undefined` turns into literal "undefined" by default in JS, unlike in PHP. That is, in PHP: echo null."!"; ...echoes "!", but in JS: console.log(null + "!"); console.log(undefined + "!"); ..logs "null!" and "undefined!". Possibly `newFromForm()` or whatever the underlying abstraction is should interpret `null` to mean "omit the parameter", but it doesn't currently (or didn't at one point; it's vaguely possible this is no longer necessary), so this avoids PHP trying to apply "null" or "undefined", literally, as highlighting languages, for example.