Page MenuHomePhabricator

[EPIC] Core should provide inline diffs as well as side by side (Move InlineDifferenceEngine into core / remove MobileDiff)
Open, NormalPublic

Description

Background

MobileFrontend provides an InlineDifferenceEngine which rather than displaying the diff in a 2 column layout displays it in a single column (e.g. unified). This seems like it would be useful for core as most websites these days offer unified as well as 2 column layout diffs and the former is extremely useful on mobile.

The Mobilefrontend diff has been a nuisance point in recent changes to revisions (e.g. Multi content revisions) and it would be helpful to make unified diffs a first class citizen in some form. This inconsistency causes confusion due to the completely different implementations: (see T123413)

Proposal

  1. Put the MobileDiff code in core with any necessary modifications (The code for includes/diff/DifferenceEngine.php is pretty messy and is likely to need heavy refactoring to achieve this).
  1. Update MobileFrontend to use this link and retire Special:MobileDiff page
  1. Update core special page to allow switching between the 2 types of diff and surface this in the desktop UI.
  1. Consider hiding or surfacing side by side diffs in mobile

Questions

  • What other solutions provide inline diffs? Is MobileFrontend implementation the best one?
  • What refactors are necessary in core / to mobile code to support such a change if the mobile implementation is favoured.

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
OpenNone
OpenNone

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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 renamed this task from Move InlineDifferenceEngine into core / remove MobileDiff to [EPIC] Move InlineDifferenceEngine into core / remove MobileDiff.Mar 20 2019, 11:37 PM
Jdlrobson raised the priority of this task from Low to Normal.
Jdlrobson added a subscriber: Fjalapeno.
Jdlrobson added a project: TechCom.

I'd be interested in Techcom's opinions on using the unified diff in core and desktop by moving the mobile code into core. Most sites displaying diffs tend to offer both side by side and unified (with unified on mobile devices) so I think we are behind here. The alternative would be to build something from scratch.

Izno added a subscriber: Izno.Jul 8 2019, 9:56 PM

On the point of unified diffs, WikEdDiff provides unified view and I almost exclusively read diffs using it rather than 2 column.

Jdlrobson updated the task description. (Show Details)Jul 8 2019, 11:02 PM

@Izno interesting! Which extension provides that and can you provide an example link showing it in action? We should probably consolidate the two of these things. It doesn't seem like a good idea to support 2.

Jdlrobson renamed this task from [EPIC] Move InlineDifferenceEngine into core / remove MobileDiff to [EPIC] Core should provide inline diffs as well as side by side (Move InlineDifferenceEngine into core / remove MobileDiff).Jul 8 2019, 11:03 PM

@Jdlrobson Not an extension; wikEdDiff is a user script on Wikipedia. =)

Izno added a comment.EditedJul 9 2019, 12:03 AM

@Jdlrobson Not an extension; wikEdDiff is a user script on Wikipedia. =)

And supported as a Gadget on en.WP. This diff displays like:

The little triangle in the middle is the toggle to display unified--it doesn't supplant side by side as I try to show toward the bottom of the screenshot. WikEdDiff does some other stuff like attempting to make the "new paragraph change" problem less noticeable, and uses older style "highlight everything", but that's more of an aside. I'd stop using the gadget if unified were available in core since the other features aren't all that necessary for me... I think. :)

I would prefer if I can swap from unified to side-by-side and vice versa on the diff page. Not a requirement for MVP for me but a strong feature. Sometimes views do have a slightly better representation in side-by-side.

The gadget also remembers (cookie? not sure) what the last opened tab was where you changed whether the unified view was displayed (the functionality is a little wonky so I won't try to elaborate). This would be a good feature but not an MVP requirement. Alternatively, a preference would work as well for the default diff view.

My preference for unified may be driven strongly by the percentage of talk pages rather than content pages on my watchlist.

TheDJ added a subscriber: TheDJ.Jul 9 2019, 9:57 AM

i've long since said we should just provide both and allow users to switch between them, like we now do for normal diff and VE diff.

I think this should be done. The code looks fine. I see it is in wikidiff2 already, so from a production standpoint this is just the addition of a few lines of wrapper code. It seems like a no-brainer. Why has it been stalled for so long, do you need help with the implementation?

D3r1ck01 added a subscriber: D3r1ck01.
Krinkle moved this task from Inbox to Watching on the TechCom board.Jul 24 2019, 8:46 PM
Florian removed a subscriber: Florian.Aug 11 2019, 10:45 AM

@tstarling It just hasn't been prioritised. Right now my motivation is purely technical, but without a product goal (e.g. we need side by side diffs on desktop and it's really important!) it's hard to drive this.

If I could find someone to help with code review, I'd be happy to do this in 10% time. Would TechComm be able to be involved in code review if that happened? Is this something the core platform team would be interested in doing with support from myself and others that know the code?