Page MenuHomePhabricator

Refactor DifferentialChangesetParser to support 1-up views and sanity
Closed, ResolvedPublic

Description

DifferentialChangesetParser is the probably the worst piece of code in the codebase. It has far far too many responsibilities and is generally twisted and evil. Unfortunately, it's also difficult to understand/separate, and has little-to-no test coverage.

It should be separated into a number of smaller pieces which each have less responsibility. In particular:

Rendering

  • All rendering should be separated out of the main class, into rendering classes with different targets.
  • Today we should support these targets:
    • 2-up target for normal diffs.
    • Plain-text target for unit testing.
  • In the future we should support these targets:
    • 1-up target for mobile/alternate stuff.
  • And maybe these targets:
    • Email+Text target for the stuff currently controlled by metamta.differential.unified-comment-context.
    • Email+HTML target to possibly restore the "!showdiff" email reply action.

Parsing

Currently we do manual hunk parsing in several places, notably DifferentialChangeset::makeContextDiff() (which support only metamta.differential.unified-comment-context), DifferentialChangesetParser itself (in multiple places), DifferentialDiff::detectCopiedCode(), and probably some others. Most of these can probably be served by creating a more pure changeset parser (which is originally what DifferentialChangesetParser was) which accepts DifferentialHunk objects, parses them, and then exposes a general useful interface for accessing the data they contain that all of the manual parsing implementations can use.

Glue

Ideally this should leave DifferentialChangesetParser as a small amount of glue code covering:

  • Whitespace options.
  • Cache.
  • Inline comments.
  • "Shields" ("This file was not changed, click to view contents.", etc).
  • JS hooks?
  • Reading inputs from Parsers.
  • Sending outputs to Renderers.

That's still kind of a lot of stuff so maybe there are options to further split it apart.

Some discussion at the bottom in T972: Enable differential diff view to span entire width of the screen (and not be fixed width).

Revisions and Commits

rP Phabricator
D12032
D12028
D12027
D12026
D12024
D12019
D12018
D12015
D12012
D12013
D12011
D12010
D12009
D12003
D11997
D12000
D11998
D11996
D11994
D11988
D11987
D11983
D11981
D11979
D11977
D11978
D11976
D11975
D11974
D11973
D11972

Related Objects

StatusAssignedTask
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley
ResolvedNone
Resolvedepriestley
Resolvedepriestley
Wontfixcsilvers
Wontfixepriestley
Resolvedepriestley
Duplicateepriestley
Resolvedepriestley
Resolvedepriestley

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
epriestley added a subscriber: Unknown Object (MLST).Sep 3 2013, 11:39 PM

Planned UI changes near the Differential revision list.

Rough patch that made this work usably-well for one install:

{P940}

epriestley closed subtask Restricted Maniphest Task as Duplicate.Jul 9 2014, 1:02 AM
epriestley added subscribers: chad, joshuaspence.

◀ Merged tasks: T5576.

epriestley changed the visibility from "All Users" to "Public (No Login Required)".Jul 28 2014, 2:31 PM

I'm going to push all that stuff into HEAD. Unified diffs are still significantly buggy, but I believe they've come far enough that the mobile experience is in a better place overall, and anticipate fixing most of the remaining issues shortly.

This probably still has some bugs, but I don't know what they are yet. The code is now relatively cohesive and meaningfully testable.