Page MenuHomePhabricator

[EPIC] Move InlineDifferenceEngine into core / remove MobileDiff
Open, NormalPublic

Description

MobileFrontend provides an InlineDifferenceEngine which rather than displaying the diff in a 2 column layout displays it in a single column. This seems like it would be useful for core.

Expected:
https://en.wikipedia.org/w/index.php?title=San_Francisco&type=revision&diff=687927333&oldid=687926997&diffonly=1&useskin=minerva&mode=inline

renders a single column diff and replicates the functionality of https://en.m.wikipedia.org/wiki/Special:MobileDiff/687926997

The code for includes/diff/DifferenceEngine.php is pretty messy and is likely to need heavy refactoring to achive this.

This inconsistency causes confusion due to the completely different implementations: (see T123413)

Related Objects

StatusAssignedTask
Declineddchen
OpenNone
OpenNone
DuplicateNone
OpenNone
ResolvedAbit
OpenNone
OpenNone
OpenNone
OpenNone
DuplicateNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedppelberg
ResolvedKrinkle
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
StalledNone
ResolvedPetrb
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedovasileva
OpenNone

Event Timeline

Jdlrobson updated the task description. (Show Details)
Jdlrobson raised the priority of this task from to Low.
Jdlrobson added subscribers: jm3, Niharika, Jhernandez and 14 others.

I had a play around with this and it's harder then it seems.

This is as far as I got - the diff code does not allow you to easily slot in new diff engines :-/
https://gerrit.wikimedia.org/r/250081

Dupe of https://www.mediawiki.org/wiki/Requests_for_comment/Inline_diffs ? I didn't see a phab task for that though. T90948 is also related...

kaldari removed a subscriber: kaldari.Nov 2 2015, 9:02 PM

See also T56328 which works on implementing a DiffFormatter subclass that supports intraline diffs. That one specifically has the API as consumer in mind, but it can of course be consumed internally as well for HTML formatting.

Niharika removed a subscriber: Niharika.Nov 2 2015, 11:12 PM
jm3 awarded a token.Dec 22 2015, 10:37 PM
jm3 removed a subscriber: jm3.
Danny_B moved this task from Unsorted to Move on the Technical-Debt board.Jan 23 2016, 12:17 AM
Jdlrobson moved this task from Tasks to Tech debt on the MobileFrontend board.Feb 18 2016, 6:44 PM
Krinkle removed a subscriber: Krinkle.Mar 31 2016, 3:10 AM

Change 300106 had a related patch set uploaded (by Jdlrobson):
Mobile diffs should use OneColumnDifferenceEngine in core

https://gerrit.wikimedia.org/r/300106

I just manually rebased this patch again.

@Jdforrester-WMF any interest in helping me get some code review for this? It's been stalled since the hackathon in Israel and I'm keen to get this work wrapped up as I think it is useful and it would be a shame to have to abandon this.

Sorry, I'd like to but I'm no phab coder.

Change 300106 abandoned by Jdlrobson:
Mobile diffs should use OneColumnDifferenceEngine in core

https://gerrit.wikimedia.org/r/300106

@Jdforrester-WMF I'm guessing that's a no :)

@Jdforrester-WMF I'm guessing that's a no :)

Yes. Still. But incredible major changes like this to incredibly important areas of the interface are slow, and to expect otherwise is to give yourself false hope.

Someone should feel free to resurrect my patch at any time.

Not me. As I said, I'm no developer and have no idea what any of this is or
what it does,.

Mark

I really want to be able to get InlineDiffEngine diff html from the API. For newcomers, it's much easier to understand than our traditional diffs, and I'm planning to build a diff viewer that would use that kind of diff into dashboard.wikiedu.org.

Code review is the current hold-up for this? Hmm... guess it's time to start sending out bribes.

@Ragesoss yup it's always been code review... :-/ I really want it too.
I recently explored an alternative approach - a new RESTBase api - given I can write JS but not so well PHP.

Tgr added a comment.Feb 4 2017, 9:10 PM

As a first step just make the diff formatter injectable in DifferenceEngine, move InlineDiffFormatter into core, expose the ability to change diff formatters to the API, keep everything else?

Eventually the horrible mess of DifferenceEngine should be decomposed into UI code (DiffView or something?) and model code.

https://gerrit.wikimedia.org/r/#/c/280904/ gives a sense of the complexity here
but pulling out the UI code would make sense to avoid the introduction of more than one engine like I did here: https://gerrit.wikimedia.org/r/#/c/280904/33/includes/diff/OneColumnDifferenceEngine.php

But yes, essentially most of the pain I encountered here was the mixing of UI and model code...

Jdlrobson moved this task from Backlog to Later on the Readers-Web-Backlog (Tracking) board.
Jdlrobson moved this task from Later to Blocked on the Readers-Web-Backlog (Tracking) board.

This is blocking some work I want to do on wikilabels so count me in if you want any help.

@Ladsgroup is this something that Wikimedia DE could spearhead as part of multi-content revisions?
Any kind of code in core that helps do the single column diff would simplify things in mobile greatly!

No, I want it as part of T105518 for Scoring platform team

Tgr added a comment.Apr 24 2018, 8:14 AM

Probably fits into MCR too, although in that case it wouldn't be worked on by WMDE as they are handling the storage side of things.

D3r1ck01 updated the task description. (Show Details)Oct 18 2018, 11:45 AM
Jdlrobson raised the priority of this task from Low to Normal.Mar 20 2019, 11:37 PM
Jdlrobson renamed this task from Move InlineDifferenceEngine into core / remove MobileDiff to [EPIC] Move InlineDifferenceEngine into core / remove MobileDiff.
Jdlrobson added a subscriber: Fjalapeno.