Page MenuHomePhabricator

Refactor source mode conversion
Closed, ResolvedPublic

Description

Source mode conversion is done through the visual mode converter, but they have differences, such as whitespace preservation, that lead to issues such as T203114.

Currently:

  • In ve.init.Target.static.parseDocument, we have a special case for source mode, where the source text is split by line, and converted into an HTML document, with a <p> for each line in the source. This is then fed to the converter which generates linear model.
    • Creating an HTML document is probably unnecessary for source mode, we could go straight from source text to linear data (as we do in ve.dm.SourceSurfaceFragment.prototype.insertContent)
  • In ve.dm.Surface.prototype.getDom we bypass the converter completely, and use ve.dm.ElementLinearData.prototype.getSourceText to convert the document back to source text

Proposal:

  • Create a new "source converter" utility that converts directly from source text to linear data, and use it to replace all of the methods listed above.
  • It may need an "inline" mode, so that "a\nb" is converted to 'a',{type:/p},{type:p},'b' instead of {type:p},'a',{type:/p},{type:p},'b',{type:/p}. The former would be used by insertContent and the latter would be used when creating a new document.

This would also be a performance increase for converting source text to linear as we would remove the HTML DOM step.

Event Timeline

Esanders created this task.Aug 30 2018, 1:07 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 30 2018, 1:07 PM

Change 458490 had a related patch set uploaded (by Esanders; owner: Esanders):
[VisualEditor/VisualEditor@master] [BREAKING CHANGE] Implement a SourceConverter

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

Esanders claimed this task.Sep 6 2018, 12:41 PM
Esanders moved this task from Incoming to Code review on the VisualEditor (Current work) board.

Change 458490 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] [BREAKING CHANGE] Implement a SourceConverter

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

Change 458489 had a related patch set uploaded (by Bartosz Dziewoński; owner: Esanders):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (7420443fe)

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

Change 458489 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (7420443fe)

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

Ryasmeen added a subscriber: Ryasmeen.EditedSep 14 2018, 8:40 PM

@Esanders: Do you have some suggestions for me on testing the areas on the front-end side that might get effected by this refactoring?

It's pretty well covered by unit tests, but loading & saving VE source mode are the areas affected (or not, hopefully)

It's pretty well covered by unit tests, but loading & saving VE source mode are the areas affected (or not, hopefully)

Great! Did some quick check on those areas, nothing looks affected to me :)

Deskana closed this task as Resolved.Sep 28 2018, 12:20 PM
Deskana triaged this task as Normal priority.
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptSep 28 2018, 12:20 PM