Esanders has uploaded a new change for review.
https://gerrit.wikimedia.org/r/60973
Change subject: Fix range translation for surface fragments
......................................................................
Fix range translation for surface fragments
Remove all manual changes to SF ranges as these are not
undoable. Instead change translate range to default to
outer expand and build functionality around that behaviour
never changing.
As translate range is always outer I don't think we need to
check for start and end crossing over?
Added more undo tests to assert these selections are maintained
properly, and added the test case to 'update' for when and undo
point is overwritten.
Insert content now results in a selection over the inserted
content. Most usages were expecting this anyway and were
followed up with an adjustRange(-length,0) which is no longer
necessary.
Noticed that the link inspector case was never being triggered
as word boundary was always expanding to at least one char (mainly
for Hanzi selection). This doesn't make much sense as single
spaces get auto selected so removed this functionality.
Added flag to collapseRange to allow collapsing to end as this
may be required to get the old behaviour (range moves to end
after insert).
Change-Id: I3dc0b4d00d37bad1ca3076a69b41c5f0b3fa0570
---
M modules/ve/ce/ve.ce.Surface.js
M modules/ve/dm/lineardata/ve.dm.ElementLinearData.js
M modules/ve/dm/ve.dm.SurfaceFragment.js
M modules/ve/dm/ve.dm.Transaction.js
M modules/ve/test/dm/lineardata/ve.dm.ElementLinearData.test.js
M modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
M modules/ve/test/dm/ve.dm.Transaction.test.js
M modules/ve/ui/inspectors/ve.ui.LinkInspector.js
8 files changed, 119 insertions(+), 103 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/VisualEditor
refs/changes/73/60973/1
diff --git a/modules/ve/ce/ve.ce.Surface.js b/modules/ve/ce/ve.ce.Surface.js
index ff7403c..783eb8c 100644
--- a/modules/ve/ce/ve.ce.Surface.js
+++ b/modules/ve/ce/ve.ce.Surface.js
@@ -327,8 +327,7 @@
originFragment.removeContent();
// Re-insert node at new location and re-select it
- targetFragment.insertContent( nodeData );
- targetFragment.adjustRange( -nodeData.length, 0
).select();
+ targetFragment.insertContent( nodeData ).select();
}, this ) );
}
diff --git a/modules/ve/dm/lineardata/ve.dm.ElementLinearData.js
b/modules/ve/dm/lineardata/ve.dm.ElementLinearData.js
index 865cef7..72ca409 100644
--- a/modules/ve/dm/lineardata/ve.dm.ElementLinearData.js
+++ b/modules/ve/dm/lineardata/ve.dm.ElementLinearData.js
@@ -685,13 +685,7 @@
} else if ( !unicodeJS.wordbreak.isBreak( dataString, offset -
1 ) ) {
offset--;
} else {
- // just return one character to the right, unless we
are at the end
- // of the text, in which case the character to the left
- if ( dataString.read( offset ) !== null ) {
- return new ve.Range( offset, offset + 1 );
- } else {
- return new ve.Range( offset - 1, offset );
- }
+ return new ve.Range( offset );
}
}
diff --git a/modules/ve/dm/ve.dm.SurfaceFragment.js
b/modules/ve/dm/ve.dm.SurfaceFragment.js
index 6b094c6..d42addb 100644
--- a/modules/ve/dm/ve.dm.SurfaceFragment.js
+++ b/modules/ve/dm/ve.dm.SurfaceFragment.js
@@ -22,7 +22,7 @@
// Properties
this.surface = surface;
- this.setRange( range && range instanceof ve.Range ? range :
surface.getSelection() );
+ this.range = range && range instanceof ve.Range ? range :
surface.getSelection();
// Short-circuit for invalid range null fragment
if ( !this.range ) {
return this;
@@ -30,14 +30,14 @@
this.document = surface.getDocument();
this.noAutoSelect = !!noAutoSelect;
-
// Initialization
var length = this.document.data.getLength();
- this.setRange( new ve.Range(
+ this.range = new ve.Range(
// Clamp range to valid document offsets
Math.min( Math.max( this.range.from, 0 ), length ),
Math.min( Math.max( this.range.to, 0 ), length )
- ) );
+ );
+ this.historyPointer = this.getSurface().getCompleteHistoryLength();
};
/* Static Properties */
@@ -103,17 +103,6 @@
};
/**
- * Update the range of the transaction and reset the history pointer.
- *
- * @method
- * @param {ve.Range} range New range
- */
-ve.dm.SurfaceFragment.prototype.setRange = function ( range ) {
- this.historyPointer = this.getSurface().getCompleteHistoryLength();
- this.range = range;
-};
-
-/**
* Check if the fragment is null.
*
* @method
@@ -163,18 +152,19 @@
};
/**
- * Get a new fragment with a zero-length selection at the start offset.
+ * Get a new fragment with a zero-length selection at the start or end offset.
*
* @method
+ * @param {boolean} [collapseToEnd=false] Collapses fragment to end
* @returns {ve.dm.SurfaceFragment} Collapsed fragment
*/
-ve.dm.SurfaceFragment.prototype.collapseRange = function () {
+ve.dm.SurfaceFragment.prototype.collapseRange = function ( collapseToEnd ) {
// Handle null fragment
if ( !this.surface ) {
return this;
}
return new ve.dm.SurfaceFragment(
- this.surface, new ve.Range( this.getRange( true ).start ),
this.noAutoSelect
+ this.surface, new ve.Range( collapseToEnd ? this.getRange( true
).end : this.getRange( true ).start ), this.noAutoSelect
);
};
@@ -535,17 +525,6 @@
if ( this.getRange( true ).getLength() ) {
tx = ve.dm.Transaction.newFromRemoval( this.document,
this.getRange() );
this.surface.change( tx, !this.noAutoSelect &&
tx.translateRange( this.getRange() ) );
- // Check if the range didn't get collapsed automatically - this
will occur when removing
- // content across un-mergable nodes because the delete only
strips out content leaving
- // structure at the beginning and end of the range in place
- if ( this.getRange( true ).getLength() ) {
- // Collapse the range manually
- this.range = new ve.Range( this.getRange( true ).start
);
- if ( !this.noAutoSelect ) {
- // Update the surface selection
- this.surface.change( null, this.getRange() );
- }
- }
}
return this;
};
@@ -596,10 +575,8 @@
if ( !ve.isArray( wrapper ) ) {
wrapper = [wrapper];
}
- var tx = ve.dm.Transaction.newFromWrap( this.document, this.getRange(),
[], [], [], wrapper ),
- newRange = new ve.Range( this.getRange( true ).start,
this.getRange( true ).end + tx.getLengthDifference() );
- this.surface.change( tx, !this.noAutoSelect && newRange );
- this.setRange( newRange );
+ var tx = ve.dm.Transaction.newFromWrap( this.document, this.getRange(),
[], [], [], wrapper );
+ this.surface.change( tx, !this.noAutoSelect && tx.translateRange(
this.getRange() ) );
return this;
};
@@ -621,7 +598,7 @@
if ( !this.surface ) {
return this;
}
- var i, tx, newRange, innerUnwrapper = [], outerUnwrapper = [];
+ var i, tx, innerUnwrapper = [], outerUnwrapper = [];
if ( this.getRange( true ).end - this.getRange( true ).start <
innerDepth * 2 ) {
throw new Error( 'cannot unwrap by greater depth than maximum
theoretical depth of selection' );
@@ -635,10 +612,7 @@
}
tx = ve.dm.Transaction.newFromWrap( this.document, this.getRange(),
outerUnwrapper, [], innerUnwrapper, [] );
- newRange = new ve.Range( this.getRange( true ).start - outerDepth,
this.getRange( true ).end + outerDepth + tx.getLengthDifference() );
- this.surface.change( tx, !this.noAutoSelect && newRange );
-
- this.setRange( newRange );
+ this.surface.change( tx, !this.noAutoSelect && tx.translateRange(
this.getRange() ) );
return this;
};
@@ -669,7 +643,7 @@
if ( !this.surface ) {
return this;
}
- var i, tx, newRange, unwrapper = [];
+ var i, tx, unwrapper = [];
if ( !ve.isArray( wrapper ) ) {
wrapper = [wrapper];
@@ -684,10 +658,7 @@
}
tx = ve.dm.Transaction.newFromWrap( this.document, this.getRange(), [],
[], unwrapper, wrapper );
- newRange = new ve.Range( this.getRange( true ).start, this.getRange(
true ).end + tx.getLengthDifference() );
- this.surface.change( tx, !this.noAutoSelect && newRange );
-
- this.setRange( newRange );
+ this.surface.change( tx, !this.noAutoSelect && tx.translateRange(
this.getRange() ) );
return this;
};
@@ -717,17 +688,14 @@
return this;
}
- var tx, newRange;
+ var tx;
if ( !ve.isArray( wrapper ) ) {
wrapper = [wrapper];
}
tx = ve.dm.Transaction.newFromWrap( this.document, this.getRange(), [],
wrapper, [], [] );
- newRange = new ve.Range( this.getRange( true ).start, this.getRange(
true ).end + tx.getLengthDifference() );
- this.surface.change( tx, !this.noAutoSelect && newRange );
-
- this.setRange( newRange );
+ this.surface.change( tx, !this.noAutoSelect && tx.translateRange(
this.getRange() ) );
return this;
};
@@ -755,7 +723,7 @@
if ( !this.surface ) {
return this;
}
- var i, tx, newRange, unwrapper = [],
+ var i, tx, unwrapper = [],
innerRange = new ve.Range( this.getRange( true ).start + depth,
this.getRange( true ).end - depth );
if ( !ve.isArray( wrapper ) ) {
@@ -771,10 +739,7 @@
}
tx = ve.dm.Transaction.newFromWrap( this.document, innerRange,
unwrapper, wrapper, [], [] );
- newRange = new ve.Range( this.getRange( true ).start, this.getRange(
true ).end + tx.getLengthDifference() );
- this.surface.change( tx, !this.noAutoSelect && newRange );
-
- this.setRange( newRange );
+ this.surface.change( tx, !this.noAutoSelect && tx.translateRange(
this.getRange() ) );
return this;
};
@@ -805,8 +770,7 @@
endSplitRequired = false,
startSplitNodes = [],
endSplitNodes = [],
- fragment = this,
- newRange = new ve.Range( this.getRange( true ).start,
this.getRange( true ).end );
+ fragment = this;
function createSplits( splitNodes, insertBefore ) {
var i, length,
@@ -822,8 +786,7 @@
}
tx = ve.dm.Transaction.newFromInsertion(
fragment.getDocument(), insertBefore ? startOffset : endOffset, data );
- newRange = new ve.Range( newRange.start, newRange.end +
tx.getLengthDifference() );
- fragment.surface.change( tx, !fragment.noAutoSelect && newRange
);
+ fragment.surface.change( tx, !fragment.noAutoSelect &&
tx.translateRange( fragment.getRange() ) );
startOffset += startOffsetChange;
endOffset += endOffsetChange;
diff --git a/modules/ve/dm/ve.dm.Transaction.js
b/modules/ve/dm/ve.dm.Transaction.js
index 5adcd0d..4c6d658 100644
--- a/modules/ve/dm/ve.dm.Transaction.js
+++ b/modules/ve/dm/ve.dm.Transaction.js
@@ -743,13 +743,8 @@
* @returns {ve.Range} Translated range, as it will be after processing
transaction
*/
ve.dm.Transaction.prototype.translateRange = function ( range, reversed ) {
- var start = this.translateOffset( range.start, reversed, false ),
- end = this.translateOffset( range.end, reversed, true );
- if ( range.start <= range.end && start > end ) {
- // translateOffset() has mapped end to jump over start
- // Disable the excludeInsertion behavior in this case
+ var start = this.translateOffset( range.start, reversed, true ),
end = this.translateOffset( range.end, reversed, false );
- }
return range.isBackwards() ? new ve.Range( end, start ) : new ve.Range(
start, end );
};
diff --git a/modules/ve/test/dm/lineardata/ve.dm.ElementLinearData.test.js
b/modules/ve/test/dm/lineardata/ve.dm.ElementLinearData.test.js
index 8327018..3082e25 100644
--- a/modules/ve/test/dm/lineardata/ve.dm.ElementLinearData.test.js
+++ b/modules/ve/test/dm/lineardata/ve.dm.ElementLinearData.test.js
@@ -1312,13 +1312,13 @@
'phrase': '维基百科',
'msg': 'Hanzi characters (cursor in middle)',
'offset': 2,
- 'expected': '百'
+ 'expected': ''
},
{
'phrase': '维基百科',
'msg': 'Hanzi characters (cursor at end)',
'offset': 4,
- 'expected': '科'
+ 'expected': ''
},
{
'phrase': 'Costs £1,234.00 each',
diff --git a/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
b/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
index e71b952..3d62c57 100644
--- a/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
+++ b/modules/ve/test/dm/ve.dm.SurfaceFragment.test.js
@@ -26,27 +26,48 @@
assert.strictEqual( fragment.willAutoSelect(), false, 'noAutoSelect
values are boolean' );
} );
-QUnit.test( 'update', 2, function ( assert ) {
+QUnit.test( 'update', 3, function ( assert ) {
var doc = ve.dm.example.createExampleDocument(),
surface = new ve.dm.Surface( doc ),
- fragment1 = new ve.dm.SurfaceFragment( surface, new ve.Range(
1, 56 ) ),
- fragment2 = new ve.dm.SurfaceFragment( surface, new ve.Range(
2, 4 ) ),
- fragment3 = new ve.dm.SurfaceFragment( surface, new ve.Range(
2, 4 ) );
- fragment1.removeContent();
+ fragment1 = new ve.dm.SurfaceFragment( surface, new ve.Range(
55, 61 ) ),
+ fragment2 = new ve.dm.SurfaceFragment( surface, new ve.Range(
55, 61 ) ),
+ fragment3 = new ve.dm.SurfaceFragment( surface, new ve.Range(
55, 61 ) );
+ fragment1.wrapNodes(
+ [{ 'type': 'list', 'attributes': { 'style': 'bullet' } }, {
'type': 'listItem' }]
+ );
assert.deepEqual(
fragment2.getRange(),
- new ve.Range( 1, 1 ),
- 'fragment range collapses after removeContent'
+ new ve.Range( 55, 69 ),
+ 'fragment range changes after wrapNodes'
);
surface.undo();
assert.deepEqual(
fragment3.getRange(),
- new ve.Range( 4, 4 ),
- 'fragment range moved after undo'
+ new ve.Range( 55, 61 ),
+ 'fragment range restored after undo'
);
+
+ fragment1 = new ve.dm.SurfaceFragment( surface, new ve.Range( 1, 1 ) );
+ surface.breakpoint();
+
+ fragment1.insertContent( '01' );
+ surface.breakpoint();
+
+ fragment1 = fragment1.collapseRange( true );
+ fragment1.insertContent( '234' );
+ fragment2 = fragment1.adjustRange();
+
+ surface.undo();
+ fragment1.insertContent( '5678' );
+ assert.deepEqual(
+ fragment2.getRange(),
+ new ve.Range( 3, 7 ),
+ 'Range created during truncated undo point still translates
correctly'
+ );
+
} );
-QUnit.test( 'adjustRange', 3, function ( assert ) {
+QUnit.test( 'adjustRange', 4, function ( assert ) {
var doc = ve.dm.example.createExampleDocument(),
surface = new ve.dm.Surface( doc ),
fragment = new ve.dm.SurfaceFragment( surface, new ve.Range(
20, 21 ) ),
@@ -54,9 +75,12 @@
assert.ok( fragment !== adjustedFragment, 'adjustRange produces a new
fragment' );
assert.deepEqual( fragment.getRange(), new ve.Range( 20, 21 ), 'old
fragment is not changed' );
assert.deepEqual( adjustedFragment.getRange(), new ve.Range( 1, 56 ),
'new range is used' );
+
+ adjustedFragment = fragment.adjustRange();
+ assert.deepEqual( adjustedFragment, fragment, 'fragment is clone if no
parameters supplied' );
} );
-QUnit.test( 'collapseRange', 3, function ( assert ) {
+QUnit.test( 'collapseRange', 5, function ( assert ) {
var doc = ve.dm.example.createExampleDocument(),
surface = new ve.dm.Surface( doc ),
fragment = new ve.dm.SurfaceFragment( surface, new ve.Range(
20, 21 ) ),
@@ -64,6 +88,10 @@
assert.ok( fragment !== collapsedFragment, 'collapseRange produces a
new fragment' );
assert.deepEqual( fragment.getRange(), new ve.Range( 20, 21 ), 'old
fragment is not changed' );
assert.deepEqual( collapsedFragment.getRange(), new ve.Range( 20, 20 ),
'new range is used' );
+
+ collapsedFragment = fragment.collapseRange( true );
+ assert.deepEqual( fragment.getRange(), new ve.Range( 20, 21 ), 'old
fragment is not changed' );
+ assert.deepEqual( collapsedFragment.getRange(), new ve.Range( 21, 21 ),
'range is at end when collapseToEnd is set' );
} );
QUnit.test( 'expandRange (closest)', 1, function ( assert ) {
@@ -112,13 +140,15 @@
}
} );
-QUnit.test( 'removeContent', 2, function ( assert ) {
+QUnit.test( 'removeContent', 6, function ( assert ) {
var doc = ve.dm.example.createExampleDocument(),
+ originalDoc = ve.dm.example.createExampleDocument(),
+ expectedDoc = ve.dm.example.createExampleDocument(),
surface = new ve.dm.Surface( doc ),
fragment = new ve.dm.SurfaceFragment( surface, new ve.Range( 1,
56 ) ),
- expectedData = ve.copyArray( ve.dm.example.data.slice( 0, 1 ) )
- .concat( ve.copyArray( ve.dm.example.data.slice( 4, 5 )
) )
- .concat( ve.copyArray( ve.dm.example.data.slice( 55 ) )
);
+ expectedData = ve.copyArray( expectedDoc.data.slice( 0, 1 ) )
+ .concat( ve.copyArray( expectedDoc.data.slice( 4, 5 ) )
)
+ .concat( ve.copyArray( expectedDoc.data.slice( 55 ) ) );
ve.setProp( expectedData[0], 'internal', 'changed', 'content', 1 );
fragment.removeContent();
assert.deepEqual(
@@ -128,12 +158,39 @@
);
assert.deepEqual(
fragment.getRange(),
+ new ve.Range( 1, 3 ),
+ 'removing content results in a fragment covering just remaining
structure'
+ );
+ surface.undo();
+ assert.deepEqual(
+ doc.getData(),
+ originalDoc.getData(),
+ 'content restored after undo'
+ );
+ assert.deepEqual(
+ fragment.getRange(),
+ new ve.Range( 1, 56 ),
+ 'range restored after undo'
+ );
+
+ fragment = new ve.dm.SurfaceFragment( surface, new ve.Range( 1, 4 ) );
+ fragment.removeContent();
+ assert.deepEqual(
+ doc.getData( new ve.Range( 0, 2 ) ),
+ [
+ { 'type': 'heading', 'attributes': { 'level': 1 },
'internal': { 'changed': { 'content' : 1 } } },
+ { 'type': '/heading'}
+ ],
+ 'removing content empties node'
+ );
+ assert.deepEqual(
+ fragment.getRange(),
new ve.Range( 1, 1 ),
- 'removing content results in a zero-length fragment'
+ 'removing content collapses range'
);
} );
-QUnit.test( 'insertContent', 3, function ( assert ) {
+QUnit.test( 'insertContent', 4, function ( assert ) {
var doc = ve.dm.example.createExampleDocument(),
surface = new ve.dm.Surface( doc ),
fragment = new ve.dm.SurfaceFragment( surface, new ve.Range( 1,
4 ) );
@@ -145,15 +202,26 @@
);
assert.deepEqual(
fragment.getRange(),
- new ve.Range( 4, 4 ),
- 'inserting content results in a zero-length fragment'
+ new ve.Range( 1, 4 ),
+ 'inserting content results in range around content'
);
+
+ surface.breakpoint();
+ fragment = new ve.dm.SurfaceFragment( surface, new ve.Range( 4 ) );
fragment.insertContent( '321' );
assert.deepEqual(
doc.getData( new ve.Range( 4, 7 ) ),
['3', '2', '1'],
'strings get converted into data when inserting content'
);
+
+ surface.undo();
+ fragment.getRange();
+ assert.deepEqual(
+ fragment.getRange(),
+ new ve.Range( 4 ),
+ 'range restored after undo'
+ );
} );
QUnit.test( 'wrapNodes/unwrapNodes', 10, function ( assert ) {
diff --git a/modules/ve/test/dm/ve.dm.Transaction.test.js
b/modules/ve/test/dm/ve.dm.Transaction.test.js
index 0e16dc4..3449ae3 100644
--- a/modules/ve/test/dm/ve.dm.Transaction.test.js
+++ b/modules/ve/test/dm/ve.dm.Transaction.test.js
@@ -970,8 +970,8 @@
cases = [
{
'before': new ve.Range( 55, 61 ),
- 'after': new ve.Range( 57, 65 ),
- 'msg': 'Wrapped range is translated to inner range'
+ 'after': new ve.Range( 55, 67 ),
+ 'msg': 'Wrapped range is translated to outer range'
},
{
'before': new ve.Range( 54, 62 ),
@@ -980,12 +980,12 @@
},
{
'before': new ve.Range( 54, 61 ),
- 'after': new ve.Range( 54, 65 ),
+ 'after': new ve.Range( 54, 67 ),
'msg': 'Wrapped range plus one on the left'
},
{
'before': new ve.Range( 55, 62 ),
- 'after': new ve.Range( 57, 68 ),
+ 'after': new ve.Range( 55, 68 ),
'msg': 'wrapped range plus one on the right'
}
];
diff --git a/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
b/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
index ff08744..b69c3c5 100644
--- a/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
+++ b/modules/ve/ui/inspectors/ve.ui.LinkInspector.js
@@ -136,7 +136,7 @@
// Call parent method
ve.ui.Inspector.prototype.onClose.call( this, action );
- var i, len, annotations, selection, adjustedFragment,
+ var i, len, annotations, selection,
insert = false,
undo = false,
clear = false,
@@ -165,13 +165,10 @@
}
}
if ( insert ) {
- // Insert default text and select it
fragment.insertContent( target, false );
- adjustedFragment = fragment.adjustRange( -target.length, 0 );
- fragment = adjustedFragment;
- // Move cursor to the end of the inserted content
- selection = new ve.Range( this.initialSelection.start +
target.length );
+ // Move cursor to the end of the inserted content, even if back
button is used
+ this.previousSelection = new ve.Range(
this.initialSelection.start + target.length );
}
if ( undo ) {
// Go back to before we added an annotation
--
To view, visit https://gerrit.wikimedia.org/r/60973
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3dc0b4d00d37bad1ca3076a69b41c5f0b3fa0570
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Esanders <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
