Page MenuHomePhabricator

VisualEditorEdit paction=diff gives bad results on MCR pages
Closed, ResolvedPublicBUG REPORT

Description

If a page with slots is edited in VE or wikitext-in-VE, the diff shows every slot being deleted. This is presumably because VisualEditorEdit only passes the wikitext to the Compare API, without restricting what slots should be compared.

This turned up when checking the fixes for T351026, because the diffs shown in VE looked bad.

Event Timeline

To see this go to e.g. https://commons.wikimedia.org/wiki/File:Lord_Bishnu-Shesh_Narayan.JPG and make any minor change in VE or in VE's source mode, and review your changes (in wikitext mode; the visual diff will look fine). You'll see something like this:

image.png (2×1 px, 327 KB)

Based on some sandbox browsing, it seems that you can pass fromslots=mediainfo to the action=compare API request (in ApiVisualEditorEdit::diffWikitext), without passing fromtext-mediainfo, to override the slot to nothing, and thus get no diff with our text (which also does not pass totext-mediainfo). Like this: https://commons.wikimedia.org/wiki/Special:ApiSandbox#action=compare&format=json&fromtitle=File%3ALord%20Bishnu-Shesh%20Narayan.JPG&fromslots=mediainfo&totext=test&formatversion=2

It's a bit icky to hardcode mediainfo there though. There's probably some way to get a list of slots that exist in the original revision and pass them all to be ignored (except the main slot).

https://commons.wikimedia.org/wiki/Special:ApiSandbox seems like it has pretty good documentation.

I think VE just needs to set toslots to main to indicate that it is only changing the one slot, not providing new values for all the slots.

You can also set slots to main to *only get a diff from the main slot* but that seems a little less good -- in the future when VE (say) gets the ability to edit categories in a separate "categories" slot, it seems better just to expand toslots to say that VE is now providing content for both main and categories and leave slots alone.

toslots being main doesn't stop the other slots being considered for the diff. It seems to just be a mechanism to explicitly tell the API which slot given content is being provided for -- the API is going to return all the slots in its output unless we tell it not to. e.g. https://commons.wikimedia.org/wiki/Special:ApiSandbox#action=compare&format=json&fromtitle=File%3ALord%20Bishnu-Shesh%20Narayan.JPG&toslots=main&totext-main=test&formatversion=2

As such, I think we should be setting slots to main to tell it that's the only one we care about. We might as well also explicitly pass the wikitext in as totext-main for the sake of clarity. e.g. https://commons.wikimedia.org/wiki/Special:ApiSandbox#action=compare&format=json&fromtitle=File%3ALord%20Bishnu-Shesh%20Narayan.JPG&toslots=main&slots=main&totext-main=test&formatversion=2

Any later change to have categories be stored in slots would need to touch this code anyway to deliberately pass it in and display the diff, so nothing we do here is going to be futureproofed in that regard.

Change 974622 had a related patch set uploaded (by DLynch; author: DLynch):

[mediawiki/extensions/VisualEditor@master] VisualEditorEdit diffs should only compare the main slot

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

Change 974622 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] VisualEditorEdit diffs should only compare the main slot

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