From 257b3f381647fcd4739f7d3bacdb5d07eed301d2 Mon Sep 17 00:00:00 2001 From: Adam Wight Date: Wed, 24 Jul 2024 17:12:10 +0200 Subject: [PATCH] Switch reuse dialog to use shared numbering mechanism This patch gives us the same number as will appear in the document, even when subrefs are present. Tests could be improved using sinon to check some call assertions but should be fine for now. Removed the test for placeholders, because these should be filtered in MWDocumentReferences.getGroupRefsByParents() Bug: T370874 Change-Id: I7543a6593308c529bcfbeb0835a7c0882cbf8621 --- .../ve-cite/ve.ui.MWReferenceSearchWidget.js | 59 +++++++++---------- .../ve.ui.MWReferenceSearchWidget.test.js | 56 +++++++++--------- 2 files changed, 56 insertions(+), 59 deletions(-) diff --git a/modules/ve-cite/ve.ui.MWReferenceSearchWidget.js b/modules/ve-cite/ve.ui.MWReferenceSearchWidget.js index 3b978a3e9..a7a4ad19b 100644 --- a/modules/ve-cite/ve.ui.MWReferenceSearchWidget.js +++ b/modules/ve-cite/ve.ui.MWReferenceSearchWidget.js @@ -140,54 +140,48 @@ ve.ui.MWReferenceSearchWidget.prototype.buildIndex = function () { * @return {Object[]} */ ve.ui.MWReferenceSearchWidget.prototype.buildSearchIndex = function () { + const docRefs = ve.dm.MWDocumentReferences.static.refsForDoc( this.internalList.getDocument() ); const groups = this.internalList.getNodeGroups(); - const index = []; const groupNames = Object.keys( groups ).sort(); // FIXME: Temporary hack, to be removed soon // eslint-disable-next-line no-jquery/no-class-state const filterExtends = this.$element.hasClass( 've-ui-citoidInspector-extends' ); + let index = []; for ( let i = 0; i < groupNames.length; i++ ) { const groupName = groupNames[ i ]; if ( groupName.indexOf( 'mwReference/' ) !== 0 ) { + // FIXME: Should be impossible to reach continue; } - const group = groups[ groupName ]; - const firstNodes = group.firstNodes; - const indexOrder = group.indexOrder; + 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' ) ] || [] ); + } ); + } - let n = 0; - for ( let j = 0; j < indexOrder.length; j++ ) { - const refNode = firstNodes[ indexOrder[ j ] ]; - // Exclude placeholder references - if ( !refNode || refNode.getAttribute( 'placeholder' ) ) { - continue; - } - // FIXME: This might miss subrefs that are reused without repeating the extends attribute - if ( filterExtends && refNode.getAttribute( 'extendsRef' ) ) { - continue; - } - // Only increment counter for real references - n++; - const refModel = ve.dm.MWReferenceModel.static.newFromReferenceNode( refNode ); - const itemNode = this.internalList.getItemNode( refModel.getListIndex() ); + index = index.concat( flatNodes.map( ( node ) => { + const listKey = node.getAttribute( 'listKey' ); + // remove `mwReference/` prefix + const group = groupName.slice( 12 ); + const footnoteNumber = docRefs.getIndexNumber( group, listKey ); + const citation = ( group ? group + ' ' : '' ) + footnoteNumber; - const refGroup = refModel.getGroup(); - const citation = ( refGroup ? refGroup + ' ' : '' ) + n; // Use [\s\S]* instead of .* to catch esoteric whitespace (T263698) - const matches = refModel.getListKey().match( /^literal\/([\s\S]*)$/ ); + const matches = listKey.match( /^literal\/([\s\S]*)$/ ); const name = matches && matches[ 1 ] || ''; - // TODO: At some point we need to make sure this text is updated in - // case the view node is still rendering. This shouldn't happen because - // all references are supposed to be in the store and therefore are - // immediately rendered, but we shouldn't trust that on principle to - // account for edge cases. - let $element; // Make visible text, citation and reference name searchable let text = ( citation + ' ' + name ).toLowerCase(); + const itemNode = this.internalList.getItemNode( node.getAttribute( 'listIndex' ) ); if ( itemNode.length ) { $element = new ve.ui.MWPreviewElement( itemNode, { useView: true } ).$element; text = $element.text().toLowerCase() + ' ' + text; @@ -201,14 +195,15 @@ ve.ui.MWReferenceSearchWidget.prototype.buildSearchIndex = function () { .text( ve.msg( 'cite-ve-referenceslist-missingref-in-list' ) ); } - index.push( { + return { $element: $element, text: text, - reference: refModel, + // TODO: return a simple node + reference: ve.dm.MWReferenceModel.static.newFromReferenceNode( node ), citation: citation, name: name - } ); - } + }; + } ) ); } return index; diff --git a/tests/qunit/ve-cite/ve.ui.MWReferenceSearchWidget.test.js b/tests/qunit/ve-cite/ve.ui.MWReferenceSearchWidget.test.js index a3659f061..7b594e72e 100644 --- a/tests/qunit/ve-cite/ve.ui.MWReferenceSearchWidget.test.js +++ b/tests/qunit/ve-cite/ve.ui.MWReferenceSearchWidget.test.js @@ -2,11 +2,35 @@ QUnit.module( 've.ui.MWReferenceSearchWidget (Cite)', ve.test.utils.newMwEnvironment() ); +function getInternalListMock( groups, mockWithNode ) { + const node = mockWithNode ? { + getAttribute: () => ( 'literal/foo' ), + getAttributes: () => ( {} ) + } : {}; + const refDocMock = { + getGroupRefsByParents: () => ( { '': [ node ] } ), + getIndexNumber: () => ( 1 ), + getItemNode: () => ( node ) + }; + const docMock = { + getStorage: () => ( refDocMock ) + }; + const mockInternalList = { + getDocument: () => ( docMock ), + getNodeGroups: () => ( groups || {} ), + getItemNode: () => ( node ) + }; + docMock.getInternalList = () => ( mockInternalList ); + node.getDocument = () => ( docMock ); + + return mockInternalList; +} + QUnit.test( 'buildIndex', ( assert ) => { const widget = new ve.ui.MWReferenceSearchWidget(); - widget.internalList = { getNodeGroups: () => ( {} ) }; - assert.strictEqual( widget.index, null ); + widget.internalList = getInternalListMock(); + assert.strictEqual( widget.index, null ); widget.buildIndex(); assert.deepEqual( widget.index, [] ); @@ -22,18 +46,7 @@ QUnit.test( 'buildIndex', ( assert ) => { QUnit.test( 'buildSearchIndex when empty', ( assert ) => { const widget = new ve.ui.MWReferenceSearchWidget(); - widget.internalList = { getNodeGroups: () => ( {} ) }; - - const index = widget.buildSearchIndex(); - assert.deepEqual( index, [] ); -} ); - -QUnit.test( 'buildSearchIndex with a placeholder', ( assert ) => { - const widget = new ve.ui.MWReferenceSearchWidget(); - const placeholder = true; - const node = { getAttribute: () => placeholder }; - const groups = { 'mwReference/': { indexOrder: [ 0 ], firstNodes: [ node ] } }; - widget.internalList = { getNodeGroups: () => groups, getItemNode: () => [] }; + widget.internalList = getInternalListMock(); const index = widget.buildSearchIndex(); assert.deepEqual( index, [] ); @@ -41,19 +54,8 @@ QUnit.test( 'buildSearchIndex with a placeholder', ( assert ) => { QUnit.test( 'buildSearchIndex', ( assert ) => { const widget = new ve.ui.MWReferenceSearchWidget(); - - // XXX: This is a regression test with a fragile setup. Please feel free to delete this test - // when you feel like it doesn't make sense to update it. - const placeholder = false; - const node = { - getDocument: () => ( { - getInternalList: () => null - } ), - getAttributes: () => ( { listKey: 'literal/foo' } ), - getAttribute: () => placeholder - }; - const groups = { 'mwReference/': { indexOrder: [ 0 ], firstNodes: [ node ] } }; - widget.internalList = { getNodeGroups: () => groups, getItemNode: () => [] }; + const groups = { 'mwReference/': {} }; + widget.internalList = getInternalListMock( groups, true ); const index = widget.buildSearchIndex(); assert.deepEqual( index.length, 1 );