Page MenuHomePhabricator

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

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

  • 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).
  • Update core diff page to allow switching to the inline diff and surface this in the desktop UI. (T240608)
  • Remove the code in MobileFrontend that has been upstreamed to core. (T240622)
  • Update MobileFrontend to use this link and retire Special:MobileDiff page
  • 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

StatusSubtypeAssignedTask
Declineddchen
OpenNone
OpenNone
DuplicateNone
OpenNone
OpenNone
OpenNone
OpenNone
DuplicateNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedppelberg
ResolvedKrinkle
OpenNone
OpenNone
OpenNone
OpenHalfak
OpenNone
OpenNone
StalledNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
ResolvedPetrb
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedovasileva
OpenNone
ResolvedAmmarpad
OpenNone
OpenNone
OpenNone
Openovasileva
OpenNone
Resolvedovasileva
Resolvedmarcella
OpenNone
OpenBUG REPORTNone
Resolvedovasileva
Resolvedovasileva

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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

There's a -1 from @daniel and a +1 from @tstarling on https://gerrit.wikimedia.org/r/#/c/547057/ and I'm not quite sure what to do here.

@daniel writes:

This is a breaking change that violates the documented contract of this method. It's no better than just removing the method. Do we know that it does not break any subclass of ContentHandler?

Note that, since ContentHandler is designed to be subclassed by extensions, any protected methods are part of the stable public interface.

You can just do:

return $this->getSlotDiffRendererWithOptions( $context, [] );

I am on vacation till the 9th but will check in then... and write some unit tests.

Change 547057 merged by jenkins-bot:
[mediawiki/core@master] Inline diffs upstreamed from MobileFrontend to core.

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

Masumrezarock100 added a subscriber: Johan.

Announcing it in the next Tech news might be a good idea since you are removing MobileDiff and all.

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

Nothing is being removed yet. This is one change in a long line of changes so advertising this would be very confusing right now.
I can confirm the diff is available (example) but right now there is no UI to access it.

Next up:

  1. Making it possible to switch to inline diff on desktop e.g. Vector:

https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/548936/5

  1. Styling diff pages for Minerva to look like Special:MobileDiff

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

  1. Removing MobileFrontend code in favor of upstreamed code

Later:

  1. Removing Special:MobileDiff

I will be working on this during the course of the coming week :-)

Schnark added a subscriber: Schnark.Dec 9 2019, 9:18 AM

This uses diff-type as URL parameter, while VisualDiff uses diffmode. I think it makes sense to decide on one common interface for different diffs -- both for URL parameters and for UI to switch between different diff styles -- before graduating any of them out of beta.

Just to clarify, what I could imagine as UI for diff switching, that would probably work well with VisualDiffs, is this:

In core, on server side: Add a button group (like the current one in VisualDiff) for switching, that simply links the different diff modes.

In core, on client side: Enhance that button group via JS to load the diffs via AJAX and show them without reloading the page.

For VisualDiff, on client side: Add a third button to the button group that will show the VisualDiff.

This would also solve T171437.

@Schnark yep that makes sense. We'll also want to standardise the control for switching in core so the VisualDiff extension can hook into adding the view.Would you be the person to collaborate with on VisualEditor-VisualDiffs or is there someone else I should speak to?

My only involvement with VisualDiffs is my user script https://de.wikipedia.org/wiki/Benutzer:Schnark/js/diff which hooks into the UI of VisualDiffs. I don't know who in the VE team currently feels responsible for the diffs, if there is anyone at all.

Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Dec 12 2019, 9:15 PM
Jdlrobson moved this task from Doing to Next up on the User-Jdlrobson board.

Change 547594 abandoned by Jdlrobson:
Separate header generation from diff generation

Reason:
not working on this for now.

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

daniel added a comment.Jan 6 2020, 2:09 PM

Does this still need code review from Core Platform Team?

Does this still need code review from Core Platform Team?

Not right now but input (guidance) is welcomed from TechCom (and core platform) on T240608

I am seeing some regressions in this change. I have created a subtask.

Moushira removed a subscriber: Moushira.Jan 9 2020, 11:35 AM
Masumrezarock100 added a comment.EditedJan 20 2020, 6:18 PM

Another regression:

Structured data edit diffs are still being served side by side rather than inline if diff-type=inline is set

Steps to reproduce

Actual results

  • Structured data edit diffs are still being served side by side even if diff-type=inline is set.

Expected results

  • We see a unified diff.

View https://commons.wikimedia.org

Note we don't support the new diff type officially on desktop. That's blocked on T240608.

I think this is an issue with mobile however?
https://commons.m.wikimedia.org/wiki/Special:MobileDiff/388741403

Thanks for the bug report I've opened T243235 to explore.

As I explained in,
T117279#5817692, I am seeing something like this on desktop mode. The diff is not shown unified even if I set diff-type=inline in the URL. The below screenshot is of https://commons.wikimedia.beta.wmflabs.org/w/index.php?title=File:Photo_on_14-01-2020_at_11.33.jpg&diff=216482&diff-type=inline&diffmode=source.


Similar problem on mobile. On mobile a structured data diff should be unified to be consistent with the normal wikitext diff. But I see side by side diffs. The below screenshot is of https://commons.m.wikimedia.beta.wmflabs.org/wiki/Special:MobileDiff/216482.