Summary
- Rename DifferenceEngine to DiffPage and move DiffEngine to includes/libs.
- Add classes wrapping wikidiff2 and the PHP diff engine.
- Introduce a format concept as a generalization of the inline format switch.
Problems
- DifferenceEngineSlotDiffRenderer is hackish and was soft-deprecated since its initial commit, but is used by 5 extensions.
- TextSlotDiffRenderer::getTextDiffInternal() if/elseif block should be replaced by an engine class hierarchy, per existing TODO.
- Various things are calling DifferenceEngine::getEngine() and phpversion( 'wikidiff2' ) in order to decide whether to do wikidiff2-specific options and formatting. It seems like a layer boundary violation. There should be an engine layer which can answer the relevant questions.
- We need T336713 -- a user preference for diff format.
- When the visual (client-side) diff type is selected in preferences, the two-column diff is generated and delivered anyway, only to be replaced by JS after load.
- Most classes are not namespaced. There are classes from various layers in includes/diff.
- Recent work in wikidiff2 allows multiple formats (say inline and two-column) to be generated simultaneously (in a batch), but there is no support for this in MW.
- Too many things are called "engine". There are at least four distinct existing engine concepts: DifferenceEngine, DiffEngine, DifferenceEngine::getEngine() and TextSlotDiffRenderer::setEngine().
- The requirement for all diff renderers to return "one or more <tr> tags" is awkward, necessitating <td colspan="4"> around non-table formats.
Reorg
- DifferenceEngine can be renamed to MediaWiki\Diff\DiffPage. Everyone knows it is a page controller/view.
- SlotDiffRenderer and its subclasses can stay in includes/diff but should be moved to the MediaWiki\Diff namespace. Conceptually, they are components of the view.
- DiffEngine is a pure PHP library and can be moved to includes/libs along with its helper classes ComplexityException, Diff, DiffOp*, WordLevelDiff and WordAccumulator. It depends only on wfDebug().
- DiffFormatter and its subclasses can also go to includes/libs, with the slight complication that TableDiffFormatter uses the Html class.
- RangeDifference exists by accident and can be deleted.
DifferenceEngine subclass review
- NewsletterDiffEngine and MassMessageListDiffEngine have multipart content. They need to render multiple text diffs and join the outputs, adding headers.
- FlowBoardContentDiffView is just trying to turn off diffing of Flow content types.
- JCJsonDifferenceEngine preprocesses the content and then does a regular text diff on the preprocessed content.
- Wikibase's EntityContentDiffView is the most complex:
- There is a page controller-level override of the language, resulting in a few method overrides.
- getRevisionHeader apparently duplicates a lot of core code, changing some tool links.
- getParserOutput is overridden
- The deprecated generateContentDiffBody() is overridden in order to use Wikibase's EntityDiffVisualizer.
Engine and format concepts
Currently we have DifferenceEngine::getEngine(), which returns "php", "wikidiff2" or the path of an executable file. I would have a class hierarchy corresponding to this idea of an engine. There would be four engines: php, wikidiff2, external and VisualEditor.
We also have TextSlotDiffRenderer::setEngine() which can take e.g. wikidiff2 or wikidiff2inline as a parameter. This corresponds to my idea of a format. An engine can deliver multiple formats.
I don't see any good reason for requiring that there only be one global engine. If we allow both wikidiff2 and VisualEditor engines to exist, the VisualEditor engine can deliver a stub format for client-side replacement, if user preferences request that.
The engine factory would be responsible for collecting the available formats and deciding which should be exposed.
In ApiComparePages, the difftype parameter would always be available, and would allow the client to specify the format. The list of possible values would come from the engine factory.
Format here means text diff format. A non-text content type handler can make its own arrangements. Multipart content type handlers can use the preferred text diff format when they request text diffs. If a page has no text content, we should suppress the display of format selectors in the UI.
The Action API equivalent of suppression of UI format selectors could be a revision property, indicating whether text is present in the revision. Ignoring the difftype parameter to ApiComparePages when there is no text seems harmless. ApiComparePages can pass down the text diff format preference even when it is not used.
Engines have the following responsibilities:
- Render diff batch. Produce array of HTML strings keyed by format.
- Provide the expected HTML context for a given format, whether table or non-table.
- OutputPage modification, adding required ResourceLoader modules.
- Cache keys, invalidation.
- Localisation, i.e. DifferenceEngine::localiseDiff()
- Table prefixes
- Get supported formats
- Get preferred format batch, given input format
These engines are tied to the diff page view, they are not independent libraries like wikidiff2 and the PHP DiffEngine.