Change 454056 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] Track newlines between paragraphs separately

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

Please doublecheck the behavior if one of the conflicting parties added, changed or removed newline characters. We do want to see all differences between the two conflicting versions, otherwise changes might be omitted without anybody deciding to do so.

Please document explicitely which scenarios you tested with, so we know the edge cases are all covered.
Ideally having old and new wikitext blobs that can be tried out (similar to our wikidiff2 testing)

Change 454512 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/TwoColConflict@master] Rewrite empty line handling in LineBasedUnifiedDiffFormatter

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

Change 454056 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Track newlines between paragraphs separately

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

TODOs in WMDE-QWERTY-Sprint-2018-08-29:

• Review and merge https://gerrit.wikimedia.org/r/454512
• Document edge cases (here as a comment)
• Check if the relevant edge cases are actual PHPUnit tests in the code, and add them

I set my local system back to how it was before the patch https://gerrit.wikimedia.org/r/454056 and tried to come up with test cases that could demonstrate the broken behavior the patch already fixed. I just couldn't. There was not really any algorithm in place that would willfully do anything with newlines. The results are effectively random: Sometimes random empty lines got added or removed without a conflict even being close. Sometimes a paragraph moves down a line, leaving two empty lines above and none below. Sometimes an entire paragraph gets actually lost (yes, really, the reason can be seen in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/TwoColConflict/+/454056/8/includes/SplitTwoColConflict/HtmlSplitConflictView.php in lines 59 and 79). I just don't know how to condense this down to a few test cases.

Something I tried is having a trivial text like this:

First line.

Second line.

Then I did an edit like this:

First line.
x
Second line.

And pressed the "simulate conflict" button. On the conflict resolution page the "x" would sometimes show up on it's own, but sometimes together with the second paragraph (this happens if the simulated conflict is in the second paragraph). If this happens and the user chooses the right side with the "x", the second paragraph gets lost.

Other tests I did work like this:

First line.
0 empty lines above.


2 empty lines above.

Then I can either add or remove empty lines and see if the result I get matches my expectation. Essentially: Whatever side I pick, I expect the result to contain as many empty lines as the side I picked originally contained. I do not expect to be able to edit these newlines when they are at the end of a textarea, because this is very hard, if not impossible to predict.

@thiemowmde I might also play with this a bit more to find fitting test cases.

One thing I just discovered bugs me a bit though and I am still not entirely sure what happens there, so I just share my observation:

I am not really sure what I like more. I think the latter could be improved a bit and "really empty lines with nothing in them" could be hidden. Also I think, that the general merge view layout has a problem with things that are present on one side and not the other. Probably this should more behave like in the table-diff view. If there's a inserted block on the one side that's not on the other side, then there should be just nothing and not an "empty" block.

This issues becomes more prominent with the famous no-js view having editors everywhere:

But as said, this could probably be improved with having nothing at places where we really have nothing ;-)

In T202402#4548865, @WMDE-Fisch wrote:

In the diff/merge view this whole change is now treated in a single block that is compared.

One thing to note here: In the example it still looks sane, even if several lines are now part of one block. But I assume this behavior would also be the same when the textlines have more characters. So imagine the textlines being actually really long article parts. Then we have several longer textblocks all in one merge comparison block :-/

In this screenshot you are essentially running into the "moved paragraph" issue: It shows paragraph 2 to be in conflict with paragraph 3, which is not really what happened as you did no edit to these paragraphs, only to the empty space between them. I honestly don't know how I am supposed to merge this. I'm even able to pick one of the paragraphs 2 and 3 two times, one time on the right, and one time on the left. Or I can choose the two empty blocks, which results in one of the paragraphs getting lost, even if no edit was made to any of them.

So I clearly prefer the other diff that does not have this problem, but shows the change as a single textarea.

There needs to be a discussion within the team. Are tests still necessary?

I just looked at the last "cleanup" patch for this and will merge it so the implementation can be tested further. One remark though:

With the new technique we omit unchanged and therefor - for solving the conflict - irrelevant empty lines. We still show relevant / unilateral changed empty lines to make them visible and editable to the user. Right now an example would look like this:

We might think about a way to make the changed empty lines a bit more prominent here.

Hmm I just noticed, that merging the https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/TwoColConflict/+/454512/ cleanup would remove the visualization for empty lines on the old interface. It looks a bit strange. - Normally I would comment this on gerrit but there I can't upload pics so easily :-). @thiemowmde

Before:

Change 454512 merged by jenkins-bot:
[mediawiki/extensions/TwoColConflict@master] Rewrite empty line handling in LineBasedUnifiedDiffFormatter

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

In T202402#4614780, @Lea_WMDE wrote:

There needs to be a discussion within the team. Are tests still necessary?

With the last change on this I have tested the newline handling quit much. Currently I am good with the implementation and it behaves consistently. I would still vote for some thoughts on empty line visibility T202402#4625082 and left a minor ticket for a minor editing issue (T205845).

I will move this to Demo now. Some minor issues are documented but in general it should work fine from our side.