Page MenuHomePhabricator

DM Selections should not cache document model data
Closed, ResolvedPublic

Description

Objects of class ve.dm.Selection currently have a documentModel property. This is misleading because the document model can change after the selection is created, breaking any correspondence between the selection and the document. Instead, the document model should be passed in where used, making the correspondence explicit to the caller.

It also makes it clearer that even though these selections are "immutable", there can be changes to the values returned by methods such as ve.dm.TableSelection#isFullCol .

(The same is also technically true for CE Selections, but that is less misleading, because there is generally less expectation that CE objects will remain valid when a transaction is applied)

Details

Show related patches Customize query in gerrit

Event Timeline

dchan created this task.Oct 29 2018, 2:54 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 29 2018, 2:54 PM

Change 470449 had a related patch set uploaded (by Divec; owner: Divec):
[VisualEditor/VisualEditor@master] Refactor ve.ce.*Selection#getDirectionality

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

Change 470456 had a related patch set uploaded (by Divec; owner: Divec):
[VisualEditor/VisualEditor@master] [BREAKING CHANGE] Do not cache document model data in DM selections

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

Change 470460 had a related patch set uploaded (by Divec; owner: Divec):
[mediawiki/extensions/VisualEditor@master] Use changed ve.dm.Selection interface

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

Change 470463 had a related patch set uploaded (by Divec; owner: Divec):
[mediawiki/extensions/Citoid@master] Use changed ve.dm.Selection interface

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

Change 470468 had a related patch set uploaded (by Divec; owner: Divec):
[mediawiki/extensions/ContentTranslation@master] Use changed ve.dm.Selection interface

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

Change 470449 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Refactor ve.ce.*Selection#getDirectionality

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

Change 470456 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] [BREAKING CHANGE] Do not cache document model data in DM selections

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

Change 470607 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (bef57a528)

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

I did an mwgrep for "new ve.dm.(Linear|Null|Table)Selection" and found one result on fr.wiki, fixed here: https://fr.wikipedia.org/w/index.php?title=MediaWiki%3AGadget-FlowDeluxe.js&type=revision&diff=153520356&oldid=151498895

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

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

Change 470460 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (02f5f0ca4)

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

Change 470463 merged by jenkins-bot:
[mediawiki/extensions/Citoid@master] Use changed ve.dm.Selection interface

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

Change 470644 had a related patch set uploaded (by Jforrester; owner: Divec):
[mediawiki/extensions/Citoid@wmf/1.33.0-wmf.2] Use changed ve.dm.Selection interface

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

Change 470468 merged by jenkins-bot:
[mediawiki/extensions/ContentTranslation@master] Use changed ve.dm.Selection interface

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

Change 470654 had a related patch set uploaded (by Jforrester; owner: Divec):
[mediawiki/extensions/ContentTranslation@wmf/1.33.0-wmf.2] Use changed ve.dm.Selection interface

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

Change 470644 merged by jenkins-bot:
[mediawiki/extensions/Citoid@wmf/1.33.0-wmf.2] Use changed ve.dm.Selection interface

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

Change 470654 abandoned by Jforrester:
Use changed ve.dm.Selection interface

Reason:
See T208366.

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

Change 470740 had a related patch set uploaded (by Divec; owner: Divec):
[VisualEditor/VisualEditor@master] WIP: [BREAKING CHANGE] Do not cache document model data in DM selections

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

Change 470747 had a related patch set uploaded (by Divec; owner: Divec):
[mediawiki/extensions/VisualEditor@master] Use changed ve.dm.Selection interface, if present

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

Change 470748 had a related patch set uploaded (by Divec; owner: Divec):
[mediawiki/extensions/ContentTranslation@master] Use changed ve.dm.Selection interface, if present

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

Change 470749 had a related patch set uploaded (by Divec; owner: Divec):
[mediawiki/extensions/Citoid@master] Use changed ve.dm.Selection interface, if present

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

Change 470741 had a related patch set uploaded (by Esanders; owner: Esanders):
[VisualEditor/VisualEditor@master] DesktopContext: Don't rely on selection#getDocument

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

Change 470741 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] DesktopContext: Don't rely on selection#getDocument

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

Change 470748 merged by jenkins-bot:
[mediawiki/extensions/ContentTranslation@master] Use changed ve.dm.Selection interface, if present

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

Change 470747 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Use changed ve.dm.Selection interface, if present

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

Change 470749 merged by jenkins-bot:
[mediawiki/extensions/Citoid@master] Use changed ve.dm.Selection interface, if present

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

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

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

Change 470740 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] [BREAKING CHANGE] Do not cache document model data in DM selections

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

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

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

This change should have no visible effect to the user.

Jdforrester-WMF added a project: Test-Coverage.
Jdforrester-WMF removed a project: Test-Coverage.

Wrong window, sorry.

Change 471802 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (866f48853)

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

Change 471802 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (866f48853)

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

Change 472179 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/Cite@master] Use changed ve.dm.Selection interface

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

Change 472179 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Use changed ve.dm.Selection interface

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

matmarex added a subscriber: matmarex.EditedNov 7 2018, 4:27 PM

I've searched all repos for new ve.dm.(Linear|Null|Table)Selection (same as Ed used T208228#4706292) and my Cite patch fixes the only place we seem to have missed (causing T208913).

marcella closed this task as Resolved.Nov 19 2018, 7:32 PM