Page MenuHomePhabricator

Legend should not show for mediainfo only diffs
Closed, ResolvedPublic3 Estimated Story PointsBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What happens?:

  • We are able to see the inline legend but the two-column format is being displayed.

What should have happened instead?:

  • The legend should not show

Event Timeline

KSiebert set the point value for this task to 3.Jun 20 2023, 11:48 AM

I updated the link in the description to one that actually shows the issue.

I reproduced the issue locally. The main slot in the two revisions is identical, with an identical content_id. However, there is no filtering for this, and the main slot still appears in the return value of DifferenceEngine::getSlotDiffRenderers(). So an empty inline diff is shown, with <tr><td colspan="4"></td></tr> appearing in the output, and the legend is added corresponding to this empty diff.

Options:

  • Filter out identical slots from getSlotDiffRenderers(). It may return an empty array and some callers may have to be updated to account for that.
  • Make sure SlotDiffRenderer returns an empty string to indicate identical content (currently it doesn't always). Store the list of empty diffs in DifferenceEngine and suppress the subsequent call to getTablePrefix() for empty diffs.
  • Both/hybrid: have getSlotDiffRenderers() filter out content with matching content_id, possibly without even loading the full text. But rely on SlotDiffRenderer to compare the actual text and return an empty string if it matches.

Note that the diff-empty message "(No difference)" is currently implemented by checking if the aggregated diff returned by DifferenceEngine::getDiffBody() is an empty string.

I think it's going to be option 1 for now, because it seems inelegant to add more side-effects to getDiffBody(). There's no nice way to propagate information about identical slots from SlotDiffRenderer::getDiff() to the getTablePrefix() call through 3 stack levels (getDiffBody -> getDiff -> showDiff -> showDiffPage), all with stability constraints.

getSlotDiffRenderers() has the loaded content, and there is Content::equals(), so we can do a deep comparison without asking for an actual diff.

Change 931702 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Do not include generate diffs for slots with identical content

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

Change 931702 merged by jenkins-bot:

[mediawiki/core@master] Do not generate diffs for slots with identical content

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

Checked out a few mediainfo diffs on different browsers and skins and the legend does not show. Moving to Done. Thanks!

T338670_BetterDiffsLegend_MediaFile.png (871×3 px, 299 KB)