@RHo I think this is the wrong parent task for T280846: [New component] Carousel: Add Carousel component to Codex.

If you add a normal link and look at the visual diff, there is a small "Link added: XXX" annotation on the right side. (Is that a new feature? I don't recall seeing it in the past.) Looks like we'd need to implement the describeChange or describeAdded/describeRemoved methods on the annotation data model for that. Maybe that would solve this issue as well.

In T280980#7031970, @Tgr wrote:

@RHo I think this is the wrong parent task for T280846: [New component] Carousel: Add Carousel component to Codex.

Yes sorry, removed!

In terms of effort to lines of code written ratio, this was pretty extreme. The process for generating a visual diff is something like:

• the recommended link annotations are applied to the DOM via their Annotation.toDomElements method (which wraps the link text in an <a> tag for accepted annotations, and is a no-op otherwise)
• ve.dm.Converter.getModelFromDom converts the DOM to the linear model, via ve.dm.Converter.getDataFromDomSubtree which very roughly does a preorder DOM tree traversal, turns DOM nodes to model objects (based on model properties like matchTagNames, matchRdfaTypes or matchFunction, and then ve.dm.Model.toDataElement as a second pass), and then turns model objects to object literals with ve.dm.Model.toDataElement.
• ve.dm.Document.buildNodeTree converts the linear data back to the data model (a ve.dm.Node tree)
• that node tree and the node tree representing the original, unedited document are compared via ve.dm.VisualDiff.diffList which finds which are the matching nodes between the original and the new document, and identifies changed nodes by comparing their linear data via ve.dm.ElementLinearData.compareElements, which relies on diffing the plain objects given by ve.dm.Annotation.getComparableObject for all the annotations on the text.

That last step is where things went wrong. Because we use the Parsoid load hook to include the initial annotations in the document, VE considered it part of the original revision, so it all came down to comparing the getComparableObject() output of an undecided and an accepted/rejected annotation, and the acceptance state of course was not represented in that object.

One very painful gotcha that was that the visual diff code has a timer and if it decides diffing goes slowly (which of course is always the case when stepping through it in a debugger), it switches to a dumber alternative algorithm which more or less just compares top-level nodes of the node tree, and in that comparison annotations are completely ignored (so the patch seemingly has no effect when viewed in a debugger).

@RHo some notes:

• Only added links are shown in the diff, not rejected ones - I figured those aren't really changes to the article, and don't show up in the wikitext diff either, so it would be less confusing this way. Easy to change though.
• There's a sidebar with a message "Link accepted", much like how the visual diff would show "Link added" etc. for other, non-addlink types of changes. If it's not considered useful (given that there is only really one kind of change - although that won't be the case once we implement editing the link target), we can just skip returning the message and the sidebar won't show up. (Also let me know if you prefer a different text.)
• The visual diff uses green for additions, red for deletions and blue for changes. For reasons to do with internals of the diff logic, link suggestions count as changes, even if conceptually it would be more correct to say we are adding something to the document. I don't think there's a clean way to change that. If it's considered a big problem, we could probably hack around it with custom CSS rules.

Change 687783 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/GrowthExperiments@master] Add Link: Fix visual diff

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

Thanks for the notes @Tgr - replies inline.

In T280980#7072979, @Tgr wrote:

@RHo some notes:

• Only added links are shown in the diff, not rejected ones - I figured those aren't really changes to the article, and don't show up in the wikitext diff either, so it would be less confusing this way. Easy to change though.

Agree that rejections and skips should not be shown. This corresponds to the logic for the scenario where no links are added, where we do not show the "review your changes" button at all:

• There's a sidebar with a message "Link accepted", much like how the visual diff would show "Link added" etc. for other, non-addlink types of changes. If it's not considered useful (given that there is only really one kind of change - although that won't be the case once we implement editing the link target), we can just skip returning the message and the sidebar won't show up. (Also let me know if you prefer a different text.)

I'd prefer to not show the message or sidebar at all please.

• The visual diff uses green for additions, red for deletions and blue for changes. For reasons to do with internals of the diff logic, link suggestions count as changes, even if conceptually it would be more correct to say we are adding something to the document. I don't think there's a clean way to change that. If it's considered a big problem, we could probably hack around it with custom CSS rules.

This is fine as is.

Change 687783 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Add Link: Fix visual diff

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

All looks and works according to the specs (no issues) - also I ran a comparison check with VE diffs for when normal links are added.

Moving for @RHo for design review.
Noitce:

• the edit summary fo Add link is automatically populated with the info on how many links are marked with 'yes', 'no' and how many
• (different from VE) for Add link Review your changes dialog doesn't have options for marking edits as minor and adds automatically the edited page to a user's Watchlist.

VE links added	Suggested links added