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)
  • 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
OpenFeatureNone
OpenBUG REPORTNone
OpenNone
StalledNone
OpenFeatureNone
DuplicateNone
ResolvedNone
OpenNone
OpenNone
OpenFeatureNone
OpenNone
ResolvedNone
ResolvedNone
ResolvedHalfak
OpenNone
OpenNone
OpenFeatureNone
StalledNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
ResolvedPetrb
OpenNone
OpenNone
DeclinedNone
OpenBUG REPORTNone
OpenNone
Resolvedovasileva
StalledNone
ResolvedAmmarpad
OpenNone
StalledNone
OpenNone
Resolvedphuedx
OpenNone
Resolvedovasileva
Resolved marcella
Resolvedmatmarex
ResolvedBUG REPORTEsanders
Resolvedovasileva
Resolvedovasileva
ResolvedJdlrobson
OpenNone
Resolved jkroll
StalledFeatureNone
OpenNone
StalledNone

Event Timeline

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

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 :-)

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

Does this still need code review from Platform Engineering?

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.

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.

Screenshot_20200121-090348.png (480×960 px, 97 KB)

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.
Screenshot_20200121-090309.png (480×960 px, 45 KB)

Change 547766 abandoned by Jdlrobson:
[mediawiki/skins/MinervaNeue@master] Style the desktop diff page of Minerva to look like Special:MobileDiff

Reason:
Not much bandwidth for this right now, but happy to provide code review for efforts pushing us in that direction.

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

LGoto moved this task from Q2 2020-2021 to Nice to Have on the Desktop Improvements board.
LGoto added a subscriber: LGoto.

Sorry moved this on accident, please disregard.