From 6e03d2cafa67ede47df133f5023515b1aa4fcddd Mon Sep 17 00:00:00 2001 From: Adam Wight Date: Fri, 2 Aug 2024 11:57:08 +0200 Subject: [PATCH] Moving ref group knowledge into a dedicated data structure Pushes per-group knowledge down into a structured object and give it an interface, separated from the singleton cache across all groups. Also changes the behavior of orphaned subrefs so that they're still rendered as subrefs, with an error placeholder where the parent should be. Bug: T372871 Change-Id: I84e679a8365f3fbfabaf344d99f56f6d069c0776 --- extension.json | 1 + modules/ve-cite/ve.ce.MWReferencesListNode.js | 61 +++++---- modules/ve-cite/ve.dm.MWDocumentReferences.js | 83 +++--------- modules/ve-cite/ve.dm.MWGroupReferences.js | 128 ++++++++++++++++++ .../ve-cite/ve.ui.MWReferenceSearchWidget.js | 14 +- .../ve.dm.MWDocumentReferences.test.js | 9 +- tests/qunit/ve-cite/ve.dm.citeExample.js | 2 +- .../ve.ui.MWReferenceSearchWidget.test.js | 11 +- 8 files changed, 192 insertions(+), 117 deletions(-) create mode 100644 modules/ve-cite/ve.dm.MWGroupReferences.js diff --git a/extension.json b/extension.json index cb8e83598..133501690 100644 --- a/extension.json +++ b/extension.json @@ -72,6 +72,7 @@ "remoteExtPath": "Cite/modules/ve-cite", "scripts": [ "ve.dm.MWDocumentReferences.js", + "ve.dm.MWGroupReferences.js", "ve.dm.MWReferenceModel.js", "ve.dm.MWReferencesListNode.js", "ve.dm.MWReferenceNode.js", diff --git a/modules/ve-cite/ve.ce.MWReferencesListNode.js b/modules/ve-cite/ve.ce.MWReferencesListNode.js index aa67c5dc0..f3755b778 100644 --- a/modules/ve-cite/ve.ce.MWReferencesListNode.js +++ b/modules/ve-cite/ve.ce.MWReferencesListNode.js @@ -240,12 +240,15 @@ ve.ce.MWReferencesListNode.prototype.update = function () { this.$refmsg.text( emptyText ); this.$element.append( this.$refmsg ); } else { - const groupedByParent = this.docRefs.getGroupRefsByParents( listGroup ); - const topLevelNodes = groupedByParent[ '' ] || []; + const groupRefs = this.docRefs.getGroupRefs( listGroup ); this.$reflist.append( - topLevelNodes.map( ( node ) => this.renderListItem( - nodes, internalList, groupedByParent, refGroup, node - ) ) + // FIXME: Clean up access functions. + Object.keys( groupRefs.footnoteNumberLookup ) + .filter( ( listKey ) => groupRefs.footnoteNumberLookup[ listKey ][ 1 ] === -1 ) + .sort( ( aKey, bKey ) => groupRefs.footnoteNumberLookup[ aKey ][ 0 ] - groupRefs.footnoteNumberLookup[ bKey ][ 0 ] ) + .map( ( listKey ) => this.renderListItem( + nodes, internalList, groupRefs, refGroup, listKey + ) ) ); this.updateClasses(); @@ -259,24 +262,23 @@ ve.ce.MWReferencesListNode.prototype.update = function () { * @private * @param {Object} nodes Node group object, containing nodes and key order array * @param {ve.dm.InternalList} internalList Internal list - * @param {Object.} groupedByParent Mapping - * from parent ref name (or '' for top-level) to refs + * @param {ve.dm.MWGroupReferences} groupRefs object holding calculated information about all group refs * @param {string} refGroup Reference group - * @param {ve.dm.MWReferenceNode} node Reference node to render as a footnote body + * @param {string} key top-level reference key, doesn't necessarily exist * @return {jQuery} Rendered list item */ -ve.ce.MWReferencesListNode.prototype.renderListItem = function ( nodes, internalList, groupedByParent, refGroup, node ) { - const listIndex = node.getAttribute( 'listIndex' ); - const key = internalList.keys[ listIndex ]; - const keyedNodes = ( nodes.keyedNodes[ key ] || [] ) - .filter( - // Exclude placeholders and references defined inside the references list node - ( backRefNode ) => !backRefNode.getAttribute( 'placeholder' ) && !backRefNode.findParent( ve.dm.MWReferencesListNode ) - ); +ve.ce.MWReferencesListNode.prototype.renderListItem = function ( nodes, internalList, groupRefs, refGroup, key ) { + const keyedNodes = nodes.keyedNodes[ key ] || []; + const node = keyedNodes ? keyedNodes[ 0 ] : null; + const listIndex = node ? node.getAttribute( 'listIndex' ) : null; + const backlinkNodes = keyedNodes.filter( + // Exclude placeholders and references defined inside the references list node + ( backRefNode ) => !backRefNode.getAttribute( 'placeholder' ) && !backRefNode.findParent( ve.dm.MWReferencesListNode ) + ); const $li = $( '
  • ' ) - .css( '--footnote-number', `"${ this.docRefs.getIndexLabel( refGroup, key ) }."` ) - .append( this.renderBacklinks( keyedNodes, refGroup ), ' ' ); + .css( '--footnote-number', `"${ groupRefs.getIndexLabel( key ) }."` ) + .append( this.renderBacklinks( backlinkNodes, refGroup ), ' ' ); // Generate reference HTML from first item in key const modelNode = internalList.getItemNode( listIndex ); @@ -320,18 +322,8 @@ ve.ce.MWReferencesListNode.prototype.renderListItem = function ( nodes, internal e.preventDefault(); } ); } - const listKey = node.getAttribute( 'listKey' ); - const subrefs = groupedByParent[ listKey ] || []; - if ( subrefs.length ) { - $li.append( - $( '
      ' ).append( - subrefs.map( ( subNode ) => this.renderListItem( - nodes, internalList, groupedByParent, refGroup, subNode - ) ) - ) - ); - } } else { + // TODO: Special rendering for missing parent of orphaned subrefs? $li.append( $( '' ) .addClass( 've-ce-mwReferencesListNode-muted' ) @@ -339,6 +331,17 @@ ve.ce.MWReferencesListNode.prototype.renderListItem = function ( nodes, internal ).addClass( 've-ce-mwReferencesListNode-missingRef' ); } + const subrefs = groupRefs.getSubrefs( key ); + if ( subrefs.length ) { + $li.append( + $( '
        ' ).append( + subrefs.map( ( subNode ) => this.renderListItem( + nodes, internalList, groupRefs, refGroup, subNode.getAttribute( 'listKey' ) + ) ) + ) + ); + } + return $li; }; diff --git a/modules/ve-cite/ve.dm.MWDocumentReferences.js b/modules/ve-cite/ve.dm.MWDocumentReferences.js index b5cd4de83..6a0269b52 100644 --- a/modules/ve-cite/ve.dm.MWDocumentReferences.js +++ b/modules/ve-cite/ve.dm.MWDocumentReferences.js @@ -19,6 +19,11 @@ ve.dm.MWDocumentReferences = function VeDmMWDocumentReferences( doc ) { // Properties this.doc = doc; + /** + * Holds the information calculated for each group. + * + * @member {Object.} + */ this.cachedByGroup = {}; doc.getInternalList().connect( this, { update: 'updateGroups' } ); @@ -68,26 +73,19 @@ ve.dm.MWDocumentReferences.prototype.updateGroups = function ( groupsChanged ) { /** * @private * @param {string[]} groupName Name of the reference group which needs to be - * updated + * updated, with prefix */ ve.dm.MWDocumentReferences.prototype.updateGroup = function ( groupName ) { - const refsByParent = this.getGroupRefsByParents( groupName ); - const topLevelNodes = refsByParent[ '' ] || []; + const nodeGroup = this.doc.getInternalList().getNodeGroup( groupName ); + this.cachedByGroup[ groupName ] = ve.dm.MWGroupReferences.static.makeGroupRefs( nodeGroup ); +}; - const indexNumberLookup = {}; - for ( let i = 0; i < topLevelNodes.length; i++ ) { - const topLevelNode = topLevelNodes[ i ]; - const topLevelKey = topLevelNode.getAttribute( 'listKey' ); - indexNumberLookup[ topLevelKey ] = ve.dm.MWDocumentReferences.static.contentLangDigits( i + 1 ); - const subrefs = ( refsByParent[ topLevelKey ] || [] ); - for ( let j = 0; j < subrefs.length; j++ ) { - const subrefNode = subrefs[ j ]; - const subrefKey = subrefNode.getAttribute( 'listKey' ); - // FIXME: RTL, and customization of the separator like with mw:referencedBy - indexNumberLookup[ subrefKey ] = `${ ve.dm.MWDocumentReferences.static.contentLangDigits( i + 1 ) }.${ ve.dm.MWDocumentReferences.static.contentLangDigits( j + 1 ) }`; - } - } - this.cachedByGroup[ groupName ] = indexNumberLookup; +/** + * @param {string} groupName with or without prefix + * @return {ve.dm.MWGroupReferences} + */ +ve.dm.MWDocumentReferences.prototype.getGroupRefs = function ( groupName ) { + return this.cachedByGroup[ groupName.startsWith( 'mwReference/' ) ? groupName : 'mwReference/' + groupName ]; }; ve.dm.MWDocumentReferences.prototype.getAllGroupNames = function () { @@ -122,54 +120,5 @@ ve.dm.MWDocumentReferences.static.contentLangDigits = function ( num ) { * marker or reflist item number. */ ve.dm.MWDocumentReferences.prototype.getIndexLabel = function ( groupName, listKey ) { - return ( this.cachedByGroup[ 'mwReference/' + groupName ] || {} )[ listKey ]; -}; - -/** - * Get all refs for a group, organized by parent ref - * - * This is appropriate when rendering a reflist organized hierarchically by - * subrefs using the `extends` feature. - * - * @param {string} groupName Filter by this group. - * @return {Object.} Mapping from parent ref - * name to a list of its subrefs. Note that the top-level refs are under the - * `null` value. - */ -ve.dm.MWDocumentReferences.prototype.getGroupRefsByParents = function ( groupName ) { - const nodeGroup = this.doc.getInternalList().getNodeGroup( groupName ); - const indexOrder = ( nodeGroup ? nodeGroup.indexOrder : [] ); - // Compile a list of all top-level node names so that we can handle orphans - // while keeping them in document order. - const seenTopLevelNames = new Set( - indexOrder - .map( ( index ) => nodeGroup.firstNodes[ index ] ) - .filter( ( node ) => node && !node.element.attributes.extendsRef && !node.element.attributes.placeholder ) - .map( ( node ) => node.element.attributes.listKey ) - .filter( ( listKey ) => listKey ) - ); - - // Group nodes by parent ref, while iterating in order of document appearance. - return indexOrder.reduce( ( acc, index ) => { - const node = nodeGroup.firstNodes[ index ]; - if ( !node || node.element.attributes.placeholder ) { - return acc; - } - - let extendsRef = node.element.attributes.extendsRef || ''; - - if ( !seenTopLevelNames.has( extendsRef ) ) { - // Promote orphaned subrefs to become top-level refs. - // TODO: Ideally this would be handled by creating placeholder error - // nodes as is done by the renderer. - extendsRef = ''; - } - - if ( acc[ extendsRef ] === undefined ) { - acc[ extendsRef ] = []; - } - acc[ extendsRef ].push( node ); - - return acc; - }, {} ); + return this.getGroupRefs( groupName ).getIndexLabel( listKey ); }; diff --git a/modules/ve-cite/ve.dm.MWGroupReferences.js b/modules/ve-cite/ve.dm.MWGroupReferences.js new file mode 100644 index 000000000..198ff4258 --- /dev/null +++ b/modules/ve-cite/ve.dm.MWGroupReferences.js @@ -0,0 +1,128 @@ +'use strict'; + +/*! + * @copyright 2024 VisualEditor Team's Cite sub-team and others; see AUTHORS.txt + * @license MIT + */ + +/** + * Holds information about the refs from a single Cite group. + * + * This structure is persisted in memory until a document change affects a ref + * tag from this group, at which point it will be fully recalculated. + * + * @private + * @constructor + */ +ve.dm.MWGroupReferences = function VeDmMWGroupReferences() { + // Mixin constructors + OO.EventEmitter.call( this ); + + // Properties + this.footnoteNumberLookup = {}; + // FIXME: push labeling to presentation code and drop from here. + this.footnoteLabelLookup = {}; + this.subRefsByParent = {}; + + /** @private */ + this.topLevelCounter = 1; + this.nodeGroup = null; +}; + +/* Inheritance */ + +OO.initClass( ve.dm.MWGroupReferences ); + +/* Static Methods */ + +/** + * Rebuild information about this group of references. + * + * @param {Object} nodeGroup InternalList group object containing refs. + * @return {ve.dm.MWGroupReferences} + */ +ve.dm.MWGroupReferences.static.makeGroupRefs = function ( nodeGroup ) { + const result = new ve.dm.MWGroupReferences(); + result.nodeGroup = nodeGroup; + + ( nodeGroup ? nodeGroup.indexOrder : [] ) + .map( ( index ) => nodeGroup.firstNodes[ index ] ) + // FIXME: debug null nodes + .filter( ( node ) => node && !node.getAttribute( 'placeholder' ) ) + .forEach( ( node ) => { + const listKey = node.getAttribute( 'listKey' ); + const extendsRef = node.getAttribute( 'extendsRef' ); + + if ( !extendsRef ) { + result.getOrAllocateTopLevelIndex( listKey ); + } else { + result.addSubref( extendsRef, listKey, node ); + } + } ); + + return result; +}; + +/* Methods */ + +/** + * @private + * @param {string} listKey Full key for the top-level ref + * @return {number[]} Allocated topLevelIndex + */ +ve.dm.MWGroupReferences.prototype.getOrAllocateTopLevelIndex = function ( listKey ) { + if ( this.footnoteNumberLookup[ listKey ] === undefined ) { + const number = this.topLevelCounter++; + this.footnoteNumberLookup[ listKey ] = [ number, -1 ]; + this.footnoteLabelLookup[ listKey ] = ve.dm.MWDocumentReferences.static.contentLangDigits( number ); + } + return this.footnoteNumberLookup[ listKey ][ 0 ]; +}; + +/** + * @private + * @param {string} parentKey Full key of the parent reference + * @param {string} listKey Full key of the subreference + * @param {ve.dm.MWReferenceNode} subrefNode Subref to add to internal tracking + */ +ve.dm.MWGroupReferences.prototype.addSubref = function ( parentKey, listKey, subrefNode ) { + if ( this.subRefsByParent[ parentKey ] === undefined ) { + this.subRefsByParent[ parentKey ] = []; + } + this.subRefsByParent[ parentKey ].push( subrefNode ); + const subrefIndex = this.subRefsByParent[ parentKey ].length; + + const topLevelIndex = this.getOrAllocateTopLevelIndex( parentKey ); + this.footnoteNumberLookup[ listKey ] = [ topLevelIndex, subrefIndex ]; + this.footnoteLabelLookup[ listKey ] = ve.dm.MWDocumentReferences.static.contentLangDigits( topLevelIndex ) + + // FIXME: RTL, and customization of the separator like with mw:referencedBy + '.' + ve.dm.MWDocumentReferences.static.contentLangDigits( subrefIndex ); +}; + +/** + * @return {ve.dm.MWReferenceNode[]} + */ +ve.dm.MWGroupReferences.prototype.getAllRefsInDocumentOrder = function () { + return Object.keys( this.footnoteNumberLookup ) + .sort( ( aKey, bKey ) => this.footnoteNumberLookup[ aKey ][ 0 ] - this.footnoteNumberLookup[ bKey ][ 0 ] ) + .map( ( listKey ) => this.nodeGroup.keyedNodes[ listKey ] ) + .filter( ( nodes ) => !!nodes ) + .map( ( nodes ) => nodes[ 0 ] ); +}; + +/** + * @param {string} parentKey parent ref key + * @return {ve.dm.MWReferenceNode[]} List of subrefs for this parent + */ +ve.dm.MWGroupReferences.prototype.getSubrefs = function ( parentKey ) { + return this.subRefsByParent[ parentKey ] || []; +}; + +/** + * @deprecated TODO: push to presentation + * @param {string} listKey full ref key + * @return {string} rendered number label + */ +ve.dm.MWGroupReferences.prototype.getIndexLabel = function ( listKey ) { + return this.footnoteLabelLookup[ listKey ]; +}; diff --git a/modules/ve-cite/ve.ui.MWReferenceSearchWidget.js b/modules/ve-cite/ve.ui.MWReferenceSearchWidget.js index 4b4fbc363..600d12425 100644 --- a/modules/ve-cite/ve.ui.MWReferenceSearchWidget.js +++ b/modules/ve-cite/ve.ui.MWReferenceSearchWidget.js @@ -154,17 +154,9 @@ ve.ui.MWReferenceSearchWidget.prototype.buildSearchIndex = function () { // FIXME: Should be impossible to reach continue; } - const groupedByParent = docRefs.getGroupRefsByParents( groupName ); - let flatNodes = []; - if ( filterExtends ) { - flatNodes = ( groupedByParent[ '' ] || [] ); - } else { - // flatMap - ( groupedByParent[ '' ] || [] ).forEach( ( parentNode ) => { - flatNodes.push( parentNode ); - flatNodes = flatNodes.concat( groupedByParent[ parentNode.getAttribute( 'listKey' ) ] || [] ); - } ); - } + const groupRefs = docRefs.getGroupRefs( groupName ); + const flatNodes = groupRefs.getAllRefsInDocumentOrder() + .filter( ( node ) => !filterExtends || !node.getAttribute( 'extendsRef' ) ); index = index.concat( flatNodes.map( ( node ) => { const listKey = node.getAttribute( 'listKey' ); diff --git a/tests/qunit/ve-cite/ve.dm.MWDocumentReferences.test.js b/tests/qunit/ve-cite/ve.dm.MWDocumentReferences.test.js index a03e12cc1..9a4830c4f 100644 --- a/tests/qunit/ve-cite/ve.dm.MWDocumentReferences.test.js +++ b/tests/qunit/ve-cite/ve.dm.MWDocumentReferences.test.js @@ -17,9 +17,8 @@ QUnit.test( 'extends test', ( assert ) => { const doc = ve.dm.citeExample.createExampleDocument( 'extends' ); const docRefs = ve.dm.MWDocumentReferences.static.refsForDoc( doc ); - // FIXME: Shows that the class doesn't handle orphans correctly - assert.strictEqual( docRefs.getIndexLabel( '', 'auto/0' ), '3.1' ); - assert.strictEqual( docRefs.getIndexLabel( '', 'auto/1' ), '1' ); - assert.strictEqual( docRefs.getIndexLabel( '', 'literal/orphaned' ), '2' ); - assert.strictEqual( docRefs.getIndexLabel( '', 'literal/ldr' ), '3' ); + assert.strictEqual( docRefs.getIndexLabel( '', 'auto/0' ), '1.1' ); + assert.strictEqual( docRefs.getIndexLabel( '', 'auto/1' ), '2' ); + assert.strictEqual( docRefs.getIndexLabel( '', 'literal/orphaned' ), '3.1' ); + assert.strictEqual( docRefs.getIndexLabel( '', 'literal/ldr' ), '1' ); } ); diff --git a/tests/qunit/ve-cite/ve.dm.citeExample.js b/tests/qunit/ve-cite/ve.dm.citeExample.js index 55af32e37..a248a375a 100644 --- a/tests/qunit/ve-cite/ve.dm.citeExample.js +++ b/tests/qunit/ve-cite/ve.dm.citeExample.js @@ -728,7 +728,7 @@ ve.dm.citeExample.domToDataCases = { data-mw='{"name":"ref","body":{"html":"Bar"},"attrs":{"extends":"foo"}}' class="mw-ref reference"> - [1] + [1.1]

        diff --git a/tests/qunit/ve-cite/ve.ui.MWReferenceSearchWidget.test.js b/tests/qunit/ve-cite/ve.ui.MWReferenceSearchWidget.test.js index e56534fb4..414f64fab 100644 --- a/tests/qunit/ve-cite/ve.ui.MWReferenceSearchWidget.test.js +++ b/tests/qunit/ve-cite/ve.ui.MWReferenceSearchWidget.test.js @@ -15,14 +15,16 @@ function getInternalListMock( hasNode ) { } : {}; const groups = hasNode ? { 'mwReference/': { - indexOrder: [ 0 ] + indexOrder: [ 0 ], + firstNodes: [ node ], + keyedNodes: { [ listKey ]: [ node ] } } } : {}; const docRefsMock = { getAllGroupNames: () => ( Object.keys( groups ) ), - getGroupRefsByParents: () => ( { '': [ node ] } ), getIndexLabel: () => ( '1' ), - getItemNode: () => ( node ) + getItemNode: () => ( node ), + getGroupRefs: ( groupName ) => ( ve.dm.MWGroupReferences.static.makeGroupRefs( groups[ groupName ] ) ) }; const docMock = { getStorage: () => ( docRefsMock ), @@ -31,7 +33,8 @@ function getInternalListMock( hasNode ) { const mockInternalList = { getDocument: () => ( docMock ), getNodeGroups: () => ( groups ), - getItemNode: () => ( node ) + getItemNode: () => ( node ), + getNodeGroup: ( groupName ) => ( groups[ groupName ] ) }; docMock.getInternalList = () => ( mockInternalList ); node.getDocument = () => ( docMock );