Page MenuHomePhabricator

[Technical debt payoff] Remove InlineDiffFormatter and InlineDifferenceEngine from MobileFrontend
Open, HighPublic

Description

The DifferenceEngine class in core can now handle inline diffs (provided wikidiff2 is installed)

Let's drop InlineDiffFormatter and InlineDifferenceEngine from MobileFrontend

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

Acceptance criteria

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

Details

Related Gerrit Patches:
mediawiki/extensions/MobileFrontend : masterBreaking: Drop InlineDiffFormatter and InlineDifferenceEngine
mediawiki/extensions/MobileFrontend : masterDeprecate InlineDiffFormatter and InlineDifferenceEngine

Related Objects

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

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.Mon, Feb 17, 2:11 PM
Jdlrobson raised the priority of this task from Medium to High.Fri, Feb 21, 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.