Page MenuHomePhabricator

Use ContentHandler to obtain DifferenceEngine in MobileFrontend
Closed, ResolvedPublic


DifferenceEngine will be split into two parts for MCR: the page-level part that deals with e.g. generating the navigational links on top of the diff, and the slot-level part which actually renders the diff for a single Content pair. DifferenceEngine will retain the page-level functionality and the slot-level logic will move to a new SlotDiffRenderer class.

Extensions can customize diff rendering by subclassing DifferenceEngine and making MediaWiki use that subclass by overriding ContentHandler::createDifferenceEngine or implementing the GetDifferenceEngine hook. Since such subclassing might be intended to change page-level rendering, slot-level rendering or both, the new code tries to handle it on both levels: on the page level, things work as before, and on the slot level, ContentHandler::getSlotDiffRenderer (which is responsible for creating SlotDiffRenderers) calls ContentHandler::createDifferenceEngine in certain cases and if a custom DifferenceEngine is used, uses that for rendering the slot diff.

Instead of using one of those mechanisms, Special:MobileDiff creates an InlineDifferenceEngine directly. There isn't any way to intercept that and use it for slot diffs so those are rendered as non-inline diffs and things break. In the longer term, some serious refactoring/rethinking will be needed to fix that (how do we show a mobile diff that has multiple slots and one of them is a content type that has its own diff renderer? we can't use both InlineDifferenceEngine and the content type's custom subclass for that slot). In the short term, just change diff engine creation to happen in a way that's easy to intercept.

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Event Timeline

This is not really an MCR problem (trying to show mobile diffs of Wikidata edits has the same issue) but will occur in many more places due to MCR. It's not a phase 1 blocker (we can preserve the current behavior with a simple patch as long as only main slots exist). It might be a phase 2 blocker if we want to show diffs of file page edits on mobile (which we do currently) but can be worked around by writing a custom InlineEntityContentDiffView engine for the Wikibase + mobile combination. Dealing with it in general is more complex. We might want to handle inline / non-inline at the DifferenceEngine::getEngine() level.

Change 452413 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/extensions/MobileFrontend@master] Use GetDifferenceEngine to inject InlineDifferenceEngine

Patch is up. In the future either InlineDifferenceEngine::getSlotHeader or InlineDiffFormatter should be improved so that slot headers look sanely, but that's not an immediate blocker, it's blocked on the main diff patch and slot headers need to be changed anyway when SlotRoleHandler lands, so that can wait.

Jdlrobson added subscribers: pmiazga, Jdlrobson.

The reading web team are not expecting to be needed to review this code as part of their process. If that's not the case, please get in touch! cc @pmiazga

phuedx triaged this task as High priority.Aug 15 2018, 10:05 AM
phuedx added subscribers: ovasileva, phuedx.

Quoting @Tgr from IRC:

13:06:26 <tgr> hi Sam, Corey asked me to ping you about this patch:
13:06:57 <tgr> it's needed to prevent an upcoming MCR change from breaking mobile diffs

With that in mind, I'll bring task back into Readers Web's backlog.

10:45:25 <phuedx> when's the breaking mcr going to land?
10:46:07 <tgr> ideally, last week :)
10:46:23 <tgr> the MobileFrontent patch is added as a dependency in gerrit
10:46:47 <tgr> so it shouldn't be able to break anything

AIUI blocks further work on parts of MCR and therefore this is a high priority (/cc @ovasileva) for Readers Web. Given the current state of #readers-web-kanbanana-board (2 tasks in Needs Code Review as of 11 AM BST), I'll also move this onto the board and into Needs Code Review.

I've +2ed the change, but it depends on a core change ( which needs to be reviewed still. Will keep on board in case we need to put this through QA or do any further review.

Change 452413 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Use GetDifferenceEngine to inject InlineDifferenceEngine