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.

Details

Related Gerrit Patches:

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

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?

Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptOct 15 2019, 5:18 PM
eprodromou added a subscriber: eprodromou.

@Jdlrobson sounds like it's a good change, and since @tstarling thinks it's a good idea, I think we can do some review and support from our Clinic Duty. I'm untagging us for now, but when you have something for CPT to review, tag us back in again, and we'll get the code review and other help you need.

Change 547056 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Permissioning refactor of DifferenceEngine

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

Thanks @eprodromou . The above (hopefully minor) refactor helps lay down some groundwork for doing this. Please review when you can!

@Jdlrobson Why did you remove Patch-For-Review project tag? Just wondering.

OK! I've moved this into our Clinic Duty board; we'll have someone from CPT look at it. Hopefully @tstarling has some time.

Change 547594 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Separate header generation from diff generation

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

Change 547056 merged by jenkins-bot:
[mediawiki/core@master] Permissioning refactor of DifferenceEngine

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

Change 547057 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] WIP: Inline diffs in core

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

Change 547766 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Style the diff page to look like Special:MobileDiff

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

Change 547795 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Namespace InlineDifferenceEngine

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

Looking for review on https://gerrit.wikimedia.org/r/547057:

copying what I wrote there:

I'm pausing to get more feedback. Once we've worked out the whats and wheres (im not sure the constants are in the most appropriate place I will pull out the control to switch diffs into a separate patch to get it some design review.

Jdlrobson moved this task from Inbox to Blocked on the User-Jdlrobson board.

Change 548936 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Allow end users to switch between available diffs

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

Just trying to channel help here, @tstarling said he could help review a couple years ago, and @Tgr knows this code. Would either of you be able to help now?

Change 547057 abandoned by Jdlrobson:
WIP: Inline diffs upstreamed from MobileFrontend to core.

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

Change 547057 restored by Jdlrobson:
WIP: Inline diffs upstreamed from MobileFrontend to core.

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

Change 547795 abandoned by Jdlrobson:
Namespace InlineDifferenceEngine

Reason:
Different approach coming..

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