Page MenuHomePhabricator

[Technical debt payoff] Remove InlineDiffFormatter and InlineDifferenceEngine from MobileFrontend
Closed, ResolvedPublic3 Estimated Story Points

Description

The DifferenceEngine class in core can now handle inline diffs (provided wikidiff2 is installed). We will drop InlineDiffFormatter and InlineDifferenceEngine from MobileFrontend which cater for 3rd parties. The deprecation notice was added a while back and we've been running this code in production for several months now. All we need to do is remove it.

Acceptance criteria

  • The deprecation notice has been pushed out.
  • InlineDifferenceEngine is gone from MobileFrontend
  • InlineDiffFormatter is gone from MobileFrontend
  • No change in display before and after to Special:MobileDiff.

Developer notes

When testing note you'll need wikidiff2 installed. If you don't after the code removal you will realise that you've been testing code locally that is never used in production and will need to set it up :-).

Note: The removal of MobileDiff will happen later. We are not ready to do that just yet!

Related Objects

StatusSubtypeAssignedTask
Declineddchen
OpenNone
OpenNone
DuplicateNone
OpenNone
OpenNone
OpenNone
StalledNone
DuplicateNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedppelberg
ResolvedKrinkle
OpenNone
OpenNone
OpenNone
ResolvedHalfak
OpenNone
OpenNone
StalledNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
ResolvedPetrb
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedphuedx

Event Timeline

Change 556811 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drop InlineDiffFormatter and InlineDifferenceEngine

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

Tgr added a comment.Dec 12 2019, 10:23 PM

Shouldn't they be moved to core instead, to support inline diffs when wikidiff2 is not present?

@Tgr I assume you mean the PHP version of the diff code? When writing https://gerrit.wikimedia.org/r/547057 I suggested that we don't support the PHP inline mode. Sounds like you disagree?

We could of course add ENGINE_PHP_INLINE, but I haven't got the bandwidth (or support for code review/from product?) to do that right now. I also have no idea if people are using MobileFrontend without wikidiff2 I'm not sure if there is a need to even do this.

If we did upstream that code, I'm not quite sure where that code would even live (on the Diff class? Inside TableDiffFormatter?)

Change 558677 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Deprecate InlineDiffFormatter and InlineDifferenceEngine

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

Tgr added a comment.Dec 17 2019, 11:01 PM

@Tgr I assume you mean the PHP version of the diff code? When writing https://gerrit.wikimedia.org/r/547057 I suggested that we don't support the PHP inline mode. Sounds like you disagree?

Yes (although not doing that in the same commit was certainly reasonable). If there is no PHP implementation, that means it won't work on a vanilla MediaWiki install, which means all code making use of the inline diff feature in some way has to assume it might not be available (and there needs to be a way to detect if it is available, which currently is not the case). Just moving over the PHP code is probably way less hassle.

We could of course add ENGINE_PHP_INLINE, but I haven't got the bandwidth (or support for code review/from product?) to do that right now.

I can review, if that helps. I don't think a non-Wikimedia polyfill needs product support per se.

I also have no idea if people are using MobileFrontend without wikidiff2 I'm not sure if there is a need to even do this.

Everyone on shared hosting? MobileFrontend is pretty much the only way to get a decent reader-oriented mobile experience these days, I'm sure many small wikis want that.

If we did upstream that code, I'm not quite sure where that code would even live (on the Diff class? Inside TableDiffFormatter?)

Without having looked at the code, a separate class per format type seems reasonable (so it would live in InlineDiffFormatter, although it probably wouldn't be quite the same class).

Yes (although not doing that in the same commit was certainly reasonable).

Have split out the patch into 2 - one with a deprecation notice and one removing it.

If there is no PHP implementation, that means it won't work on a vanilla MediaWiki install

It will fall back to the side by side diff that's packaged in core. So diffs will be usable just not optimum. Do you think it's unreasonable to require wikidiff2 for inline diffs?

I can review, if that helps. I don't think a non-Wikimedia polyfill needs product support per se.

If we do this let's capture this in a new task. I often write patches that sit in gerrit for months so I'd want to make sure there's support for adding it. The deprecation notice I've added in the new patchset gives us a whole release to do this.

Tgr added a comment.Dec 18 2019, 12:29 AM

It will fall back to the side by side diff that's packaged in core. So diffs will be usable just not optimum.

That's fair, the old diff is ugly but still functional on a mobile screen.

Change 558677 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Deprecate InlineDiffFormatter and InlineDifferenceEngine

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

Jdlrobson updated the task description. (Show Details)
Jdlrobson changed the task status from Open to Stalled.Dec 20 2019, 3:53 PM

Blocked until next mediawiki release 1.35 which will be a few months since 1.34 only just came out.
Then https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/556811 Breaking: Drop InlineDiffFormatter and InlineDifferenceEngine can land

Jdlrobson changed the task status from Stalled to Open.Jan 7 2020, 5:30 PM

Talking to Gergo I don't think we need to block this until the release. I'll leave a few weeks as grace but we can remove this more promptly.

Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.
Moushira removed a subscriber: Moushira.Jan 8 2020, 1:11 PM
ovasileva triaged this task as Medium priority.Feb 17 2020, 2:11 PM
Jdlrobson raised the priority of this task from Medium to High.Feb 21 2020, 4:41 PM

@ovasileva there is a new branch coming shortly (7 April 2020). This should be done before that date. At this point it should be low risk as we are simply removing unused code. If we could schedule this some time in March that would be splendid. I've moved to needs prioritization so that this doesn't get lost.

@ovasileva there is a new branch coming shortly (7 April 2020). This should be done before that date. At this point it should be low risk as we are simply removing unused code. If we could schedule this some time in March that would be splendid. I've moved to needs prioritization so that this doesn't get lost.

@Jdlrobson - noted. Keeping it and eye on it here for the next week or so, but will move to kanban board once we have some more of the tasks there estimated.

Thanks Olga! Sounds good to me!

Jdlrobson moved this task from Blocked to Next up on the User-Jdlrobson board.Mar 2 2020, 10:41 PM

@Tgr I've got a question about whether I can deprecate this now - would it be possible for you to give your perspective in this discussion? Thanks in advance.

Thanks to @Tgr I understand this better. For third parties we should wait until after the next release is cut.

Change 596186 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/vagrant@master] Add wikidiff2 as a mobilefrontend dependency

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

Change 596186 merged by jenkins-bot:
[mediawiki/vagrant@master] Add wikidiff2 as a mobilefrontend dependency

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

Jdlrobson changed the task status from Open to Stalled.Jun 25 2020, 9:22 PM

Stalled until next release.

Jdlrobson changed the task status from Stalled to Open.Jul 14 2020, 7:56 PM
ovasileva lowered the priority of this task from High to Medium.Aug 5 2020, 5:20 PM
ovasileva set the point value for this task to 3.Aug 5 2020, 5:22 PM
Jdlrobson removed ovasileva as the assignee of this task.Aug 17 2020, 6:30 PM
Jdlrobson added a subscriber: ovasileva.

Change 556811 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Breaking: Drop InlineDiffFormatter and InlineDifferenceEngine

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

phuedx claimed this task.Aug 26 2020, 5:08 PM
TheDJ added a comment.Aug 31 2020, 8:57 AM

It seems that the inline diff is now in the diff table, but this causes it to overflow ?
Example: https://en.m.wikipedia.org/wiki/Special:MobileDiff/975914133?diffmode=source

Is that related to this work ?

Jdlrobson added a comment.EditedAug 31 2020, 4:38 PM

@TheDJ thanks for flagging this the partial revert in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/623372 will take care of this.

I'll open a new ticket to describe the problem that bad CSS was trying to fix.

phuedx closed this task as Resolved.Sep 7 2020, 10:20 AM

FTR https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/623372 was merged. Being bold and resolving this task.

I'll open a new ticket to describe the problem that bad CSS was trying to fix.

Was this task created?

While cramped diffs don't look great they are usuable,

e.g. Desktop Minerva diffs:
https://en.wikipedia.org/w/index.php?title=Lancashire&diff=977049817&diffmode=source&useskin=minerva

I figure we should see if any 3rd parties/Minerva desktop users raise bugs around this feature. It' not clear how well we need to support this right now so I skipped the task creation for now.

Jdlrobson updated the task description. (Show Details)Sep 8 2020, 10:21 PM