Page MenuHomePhabricator

Talk page special case detection misses some in-between responses
Closed, ResolvedPublic8 Estimated Story Points


There are edge-cases where an edit on a talk page that should be detected as a valid "talk page special case" (i.e. both users did nothing but adding a response at the same position) is not detected as such.


After the 1st users edit
Conflicting edit by the 2nd user
:Conflicting response


The reason is that the diff algorithm we use detects the first edit as copy–change–copy. But we are looking for two copy–add–copy changes, so we don't detect this. The first user's addition is not detected as an addition but as a "change" from an empty line into the user's response. That's even correct as it is exactly what the user did.

It might be possible to add this edge-case to our detection algorithm. Specifically: We could check if the left side of a "change" is empty, and act as if it an addition.

This is split from T248668: Edge case: newlines can break talk page use case.

Acceptance criteria:

  • Add this edge case to the detection algorithm.
  • Make sure the view is able to render the result of this detection.
  • Check if the merge works as expected (see below).
  • Should we also create a browser test for this?
Expected merge result
:Conflicting response


Event Timeline

Restricted Application added a project: archived--TCB-Team. · View Herald TranscriptApr 28 2020, 12:01 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 592926 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] [WIP] Detect talk-page cases that are changes instead of additions

What I learned to far while working on the patch

I tried to add the detection of the described copy–change–copy situations to our AnnotatedHtmlDiffFormatter, but noticed this is not possible: To fully process this situation we need both sides, but the formatter only has one. So the detection must be done in the ResolutionSuggester, or some new class.

Why do we need both? Let's say we do have the text A\n\nB. The 1st user changes it to A\n:1\nB, and the 2nd user changes it to A\n:2\n\nB.

  • Diff for the 1st user:
    • Copy "A"
    • Change "" (1 empty line) to ":1"
    • Copy "B"
  • Diff for the 2nd user:
    • Copy "A"
    • Add ":2"
    • Copy "\nB"

Note that the empty line between A and B is edited in the first diff, but part of an unchanged copy block in the second diff. Note this makes the last copy block not identical. What to do about this? Drop the newline? It would get lost. Always add it? That would cause an unexpected amount of newlines in the merge result.

The view needs this to be able to resolve this cleanly:

  • Copy "A"
  • Add ":1"
  • Add ":2\n"
  • Copy "B"

Problem: It's impossible to derive this only from the 2nd diff, because there is nothing wrong with the 2nd diff. We need to take the 1st diff into account to fix-up the 2nd.

Or the other way around, because there are at least 4 edge-cases like this (empty lines in either edit, on either side of the added response).

Should we move this into the sprint?

Lena_WMDE set the point value for this task to 13.May 11 2020, 11:10 AM
Lena_WMDE changed the point value for this task from 13 to 8.May 20 2020, 8:21 AM

Change 597519 had a related patch set uploaded (by WMDE-Fisch; owner: WMDE-Fisch):
[mediawiki/extensions/TwoColConflict@master] Add browser test for talk page conflicts adding new lines

Change 592926 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Detect talk-page cases that are changes instead of additions

Change 597519 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Add browser test for talk page conflicts adding new lines

thiemowmde closed this task as Resolved.May 26 2020, 11:49 AM
thiemowmde claimed this task.
thiemowmde moved this task from Demo to Done on the WMDE-QWERTY-Sprint-2020-05-13 board.