Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/629b Alternative fix infinite selection loop. #671

Merged
merged 24 commits into from
Nov 16, 2016
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
f4c0d25
Fixed: infinite selection fixing loop.
scofalik Nov 2, 2016
b9d6bc2
Fixed: clear infinite loop counter after user interaction.
scofalik Nov 2, 2016
1b39bfb
Fixed: view.Selection#isEqual better algorithm.
scofalik Nov 2, 2016
d2d1e73
Fixed: view.Selection#isEqual for fake selections.
scofalik Nov 2, 2016
a656829
Tests: fixed failed tests and 100% CC.
scofalik Nov 2, 2016
d80b367
Changed: SelectionObserver clearing infinite loop counter in time int…
scofalik Nov 9, 2016
317d3dc
Tests: SelectionObserver infinite loop removed unstable unit tests an…
scofalik Nov 9, 2016
f220109
Tests: updated manual test description.
scofalik Nov 9, 2016
a83c2ab
Merge branch 'master' into t/629b
Reinmar Nov 9, 2016
6b6da0c
Manual ticket tests should also be inside manual/ directory.
Reinmar Nov 9, 2016
3768f04
Tests: Improved cleanup.
Reinmar Nov 10, 2016
3dc704c
Tests: Cleaning listeners should not be necessary since we're disabli…
Reinmar Nov 10, 2016
a8e53ca
Tests: Fixed a selection observer test which could never work, but wa…
Reinmar Nov 10, 2016
dcf65b8
Added: Implemented destroy chain for EditingController->observers->do…
scofalik Nov 15, 2016
0de2bf7
Fixed: Selection#isEqual was throwing in the selection had no ranges.
scofalik Nov 15, 2016
25b8ba2
Docs: added/fixed code comments.
scofalik Nov 15, 2016
a3690d5
Tests: Additional tests for model.Selection and view.SelectionObserver.
scofalik Nov 15, 2016
b1e0e1f
Merge branch 'master' into t/629b
Reinmar Nov 15, 2016
adcdbdc
Changed: simplified implementation of model/view.Selection#isEqual.
scofalik Nov 16, 2016
6a2418b
Tests: added additional test for EditingController#destroy.
scofalik Nov 16, 2016
7ccaa13
Docs: added missing documentation.
scofalik Nov 16, 2016
bf9fc9f
Tests: expanded tests for model/view.Selection to cover more cases.
scofalik Nov 16, 2016
bf87840
Tests: Should not log a warning which is expected to be logged.
Reinmar Nov 16, 2016
6a9306e
Merge branch 'master' into t/629b
Reinmar Nov 16, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/controller/editingcontroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ export default class EditingController {
* Removes all event listeners attached by the EditingController.
*/
destroy() {
this.view.destroy();
this._listenter.stopListening();
}
}
24 changes: 16 additions & 8 deletions src/conversion/view-selection-to-model-converters.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
* @namespace engine.conversion.viewSelectionToModel
*/

import ModelSelection from '../model/selection.js';

/**
* Function factory, creates a callback function which converts a {@link engine.view.Selection view selection} taken
* from the {@link engine.view.Document#selectionChange} event and sets in on the {@link engine.model.Document#selection model}.
Expand All @@ -26,15 +28,21 @@
*/
export function convertSelectionChange( modelDocument, mapper ) {
return ( evt, data ) => {
modelDocument.enqueueChanges( () => {
const viewSelection = data.newSelection;
const ranges = [];
const viewSelection = data.newSelection;
const modelSelection = new ModelSelection();

const ranges = [];

for ( let viewRange of viewSelection.getRanges() ) {
ranges.push( mapper.toModelRange( viewRange ) );
}

for ( let viewRange of viewSelection.getRanges() ) {
ranges.push( mapper.toModelRange( viewRange ) );
}
modelSelection.setRanges( ranges, viewSelection.isBackward );

modelDocument.selection.setRanges( ranges, viewSelection.isBackward );
} );
if ( !modelSelection.isEqual( modelDocument.selection ) ) {
modelDocument.enqueueChanges( () => {
modelDocument.selection.setTo( modelSelection );
} );
}
};
}
24 changes: 15 additions & 9 deletions src/model/selection.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,25 +125,31 @@ export default class Selection {
}

/**
* Checks whether, this selection is equal to given selection. Selections equal if they have the same ranges and directions.
* Checks whether this selection is equal to given selection. Selections are equal if they have same directions,
* same number of ranges and all ranges from one selection equal to a range from other selection.
*
* @param {engine.model.Selection} otherSelection Selection to compare with.
* @returns {Boolean} `true` if selections are equal, `false` otherwise.
*/
isEqual( otherSelection ) {
const rangeCount = this.rangeCount;

if ( rangeCount != otherSelection.rangeCount ) {
if ( this.rangeCount != otherSelection.rangeCount ) {
return false;
} else if ( this.rangeCount === 0 ) {
return true;
}

for ( let i = 0; i < this.rangeCount; i++ ) {
if ( !this._ranges[ i ].isEqual( otherSelection._ranges[ i ] ) ) {
return false;
}
if ( !this.anchor.isEqual( otherSelection.anchor ) || !this.focus.isEqual( otherSelection.focus ) ) {
return false;
}

return this.isBackward === otherSelection.isBackward;
// Every range from this selection...
return Array.from( this.getRanges() ).every( ( rangeA ) => {
// ... has a range in other selection...
return Array.from( otherSelection.getRanges() ).some( ( rangeB ) => {
// ... which it is equal to.
return rangeA.isEqual( rangeB );
} );
} );
}

/**
Expand Down
6 changes: 6 additions & 0 deletions src/view/document.js
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,12 @@ export default class Document {
observer.enable();
}
}

destroy() {
for ( let observer of this._observers.values() ) {
observer.destroy();
}
}
}

mix( Document, ObservableMixin );
Expand Down
6 changes: 5 additions & 1 deletion src/view/observer/domeventobserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@ export default class DomEventObserver extends Observer {
const types = typeof this.domEventType == 'string' ? [ this.domEventType ] : this.domEventType;

types.forEach( type => {
domElement.addEventListener( type, domEvent => this.isEnabled && this.onDomEvent( domEvent ) );
this.listenTo( domElement, type, ( eventInfo, domEvent ) => {
if ( this.isEnabled ) {
this.onDomEvent( domEvent );
}
} );
} );
}

Expand Down
6 changes: 6 additions & 0 deletions src/view/observer/mutationobserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,12 @@ export default class MutationObserver extends Observer {
this._mutationObserver.disconnect();
}

destroy() {
super.destroy();

this._mutationObserver.disconnect();
}

/**
* Handles mutations. Deduplicates, mark view elements to sync, fire event and call render.
*
Expand Down
10 changes: 10 additions & 0 deletions src/view/observer/observer.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
* For licensing, see LICENSE.md.
*/

import DomEmitterMixin from '../../../utils/dom/emittermixin.js';
import mix from '../../../utils/mix.js';

/**
* Abstract base observer class. Observers are classes which observe changes on DOM elements, do the preliminary
* processing and fire events on the {@link engine.view.Document} objects. Observers can also add features to the view,
Expand Down Expand Up @@ -59,6 +62,11 @@ export default class Observer {
this.isEnabled = false;
}

destroy() {
this.disable();
this.stopListening();
}

/**
* Starts observing the given root element.
*
Expand All @@ -67,3 +75,5 @@ export default class Observer {
* @param {String} name The name of the root element.
*/
}

mix( Observer, DomEmitterMixin );
34 changes: 27 additions & 7 deletions src/view/observer/selectionobserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
* For licensing, see LICENSE.md.
*/

/* global setInterval, clearInterval */

import Observer from './observer.js';
import MutationObserver from './mutationobserver.js';

import log from '../../../utils/log.js';

/**
Expand Down Expand Up @@ -68,6 +69,8 @@ export default class SelectionObserver extends Observer {
*/
this._documents = new WeakSet();

this._clearInfiniteLoopInterval = setInterval( () => this._clearInfiniteLoop(), 2000 );

/**
* Private property to store the last selection, to check if the code does not enter infinite loop.
*
Expand Down Expand Up @@ -101,11 +104,19 @@ export default class SelectionObserver extends Observer {
return;
}

domDocument.addEventListener( 'selectionchange', () => this._handleSelectionChange( domDocument ) );
this.listenTo( domDocument, 'selectionchange', () => {
this._handleSelectionChange( domDocument );
} );

this._documents.add( domDocument );
}

destroy() {
super.destroy();

clearInterval( this._clearInfiniteLoopInterval );
}

/**
* Selection change listener. {@link engine.view.observer.MutationObserver#flush Flush} mutations, check if
* selection changes and fires {@link engine.view.Document#selectionChange} event.
Expand Down Expand Up @@ -151,10 +162,6 @@ export default class SelectionObserver extends Observer {
newSelection: newViewSelection,
domSelection: domSelection
} );

// If nothing changes on `selectionChange` event, at this point we have "dirty DOM" (changed) and de-synched
// view (which has not been changed). In order to "reset DOM" we render the view again.
this.document.render();
}

/**
Expand All @@ -179,12 +186,25 @@ export default class SelectionObserver extends Observer {
this._loopbackCounter = 0;
}

if ( this._loopbackCounter > 10 ) {
// This counter is reset every 2 seconds. 50 selection changes in 2 seconds is enough high number
// to be very difficult (impossible) to achieve using just keyboard keys (during normal editor use).
if ( this._loopbackCounter > 50 ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deserves some comment that you empirically tested that making more than 50 selection changes in 2s is not possible. BTW. What about making selections by dragging over the text?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW. What about making selections by dragging over the text?

The selection has to be same as previous or pre-previous selection, so you would have to drag precisly, which means that it would be probably impossible to do 50 changes in 2 secs.

return true;
}

return false;
}

/**
* Clears `SelectionObserver` internal properties connected with preventing infinite loop.
*
* @protected
*/
_clearInfiniteLoop() {
this._lastSelection = null;
this._lastButOneSelection = null;
this._loopbackCounter = 0;
}
}

/**
Expand Down
28 changes: 17 additions & 11 deletions src/view/selection.js
Original file line number Diff line number Diff line change
Expand Up @@ -283,33 +283,39 @@ export default class Selection {
}

/**
* Checks whether, this selection is equal to given selection. Selections equal if they have the same ranges and directions.
* Checks whether, this selection is equal to given selection. Selections are equal if they have same directions,
* same number of ranges and all ranges from one selection equal to a range from other selection.
*
* @param {engine.view.Selection} otherSelection Selection to compare with.
* @returns {Boolean} `true` if selections are equal, `false` otherwise.
*/
isEqual( otherSelection ) {
const rangeCount = this.rangeCount;

if ( rangeCount != otherSelection.rangeCount ) {
if ( this.isFake != otherSelection.isFake ) {
return false;
}

if ( this.isFake != otherSelection.isFake ) {
if ( this.isFake && this.fakeSelectionLabel != otherSelection.fakeSelectionLabel ) {
return false;
}

if ( this.isFake && this.fakeSelectionLabel != otherSelection.fakeSelectionLabel ) {
if ( this.rangeCount != otherSelection.rangeCount ) {
return false;
} else if ( this.rangeCount === 0 ) {
return true;
}

for ( let i = 0; i < this.rangeCount; i++ ) {
if ( !this._ranges[ i ].isEqual( otherSelection._ranges[ i ] ) ) {
return false;
}
if ( !this.anchor.isEqual( otherSelection.anchor ) || !this.focus.isEqual( otherSelection.focus ) ) {
return false;
}

return this._lastRangeBackward === otherSelection._lastRangeBackward;
// Every range from this selection...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I see correctly that the ranges can be in a different order? Why so? Then, the _lastRangeBackward property would need a more precise check. Besides, if we don't expect this to happen frequently, I'd consider these selections different.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backward/forward selection check is indirectly implemented when we check anchor and focus. Basically, if anchor and focus are equal and ranges are equal, this means that selections are equal. If the last range was added in different direction, anchor and focus would differ.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was rather asking why is this method trying to find a range in the other selection which matches range from this selection... for each range in this selection? Moreover, this implementation is very ineffective, cause it creates new array on every call of every() callback and makes huge number of comparisons (in a bit random way). And last but not least – since we're checking anchor and focus first, then checking ranges again will for a collapsed selection tripple the amount of work and for a single range non collapsed selection double it.

Therefore, I've been asking about the order of ranges (whether it shouldn't be the same in both selections) because the easiest possible implementation is to loop through both selections' ranges and compare the pairs. Not only this will be much faster, but also cleaner.

Copy link
Contributor Author

@scofalik scofalik Nov 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I see.

Ranges are not sorted upon insertion. This means that we have to check whether every range in selection A has a matching range in selection B the way we do. If the ranges are ordered that would be easier.

It is a matter of us deciding whether the order range is important. Being strict, I think that we should not care about the ranges order, because from "outside" selection: [ 1, 4 ], [ 5, 7 ], [ 8, 9 ] forward behaves exactly the same as [ 5, 7 ], [ 1, 4 ], [ 8, 9 ] forward. On the other hand, the amount of situations when this will be important is close to 0.

I can change the implementation but... To be honest selection will usually have just one range, except of tables where it may be more but in 99% scenarios it will will be less than 10-20 ranges. I agree with changing so the arrays are not created. If you want to change "order logic" I'm fine with that as well, just confirm please whether we are doing it or leaving it as is.

And last but not least – since we're checking anchor and focus first, then checking ranges again will for a collapsed selection triple.

So, instead one operation we have three? And it is me who get complaints about premature optimization? :)

Copy link
Contributor Author

@scofalik scofalik Nov 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was rather asking why is this method trying to find a range in the other selection which matches range from this selection... for each range in this selection

BTW. I don't know if you haven't got too confused...

For each range in selection A, we check if there is a matching range in selection B. The only way we can "speed" this up is if we already have sorted arrays... (or care about the order).

return Array.from( this.getRanges() ).every( ( rangeA ) => {
// ... has a range in other selection...
return Array.from( otherSelection.getRanges() ).some( ( rangeB ) => {
// ... which it is equal to.
return rangeA.isEqual( rangeB );
} );
} );
}

/**
Expand Down
19 changes: 12 additions & 7 deletions tests/controller/editingcontroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,16 @@ import { getData as getViewData } from 'ckeditor5/engine/dev-utils/view.js';

describe( 'EditingController', () => {
describe( 'constructor()', () => {
let model, editing;

beforeEach( () => {
model = new ModelDocument();
editing = new EditingController( model );
} );

it( 'should create controller with properties', () => {
const model = new ModelDocument();
const editing = new EditingController( model );

expect( editing ).to.have.property( 'model' ).that.equals( model );
expect( editing ).to.have.property( 'view' ).that.is.instanceof( ViewDocument );
expect( editing ).to.have.property( 'mapper' ).that.is.instanceof( Mapper );
expect( editing ).to.have.property( 'modelToView' ).that.is.instanceof( ModelConversionDispatcher );

editing.destroy();
} );
} );

Expand All @@ -54,6 +52,10 @@ describe( 'EditingController', () => {
editing = new EditingController( model );
} );

afterEach( () => {
editing.destroy();
} );

it( 'should create root', () => {
const domRoot = createElement( document, 'div', null, createElement( document, 'p' ) );

Expand Down Expand Up @@ -128,6 +130,7 @@ describe( 'EditingController', () => {
after( () => {
document.body.removeChild( domRoot );
listener.stopListening();
editing.destroy();
} );

beforeEach( () => {
Expand Down Expand Up @@ -274,6 +277,8 @@ describe( 'EditingController', () => {
} );

expect( spy.called ).to.be.false;

editing.destroy();
} );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I miss a test that destroy() destroys the view.

} );
} );
4 changes: 4 additions & 0 deletions tests/conversion/buildmodelconverter.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ describe( 'Model converter builder', () => {
dispatcher.on( 'remove', remove() );
} );

afterEach( () => {
viewDoc.destroy();
} );

describe( 'model element to view element conversion', () => {
it( 'using passed view element name', () => {
buildModelConverter().for( dispatcher ).fromElement( 'paragraph' ).toElement( 'p' );
Expand Down
4 changes: 4 additions & 0 deletions tests/conversion/model-selection-to-view-converters.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ beforeEach( () => {
dispatcher.on( 'selection', convertCollapsedSelection(), { priority: 'low' } );
} );

afterEach( () => {
viewDoc.destroy();
} );

describe( 'default converters', () => {
beforeEach( () => {
// Selection converters for selection attributes.
Expand Down
20 changes: 20 additions & 0 deletions tests/conversion/view-selection-to-model-converters.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ describe( 'convertSelectionChange', () => {
convertSelection = convertSelectionChange( model, mapper );
} );

afterEach( () => {
view.destroy();
} );

it( 'should convert collapsed selection', () => {
const viewSelection = new ViewSelection();
viewSelection.addRange( ViewRange.createFromParentsAndOffsets(
Expand Down Expand Up @@ -106,4 +110,20 @@ describe( 'convertSelectionChange', () => {
expect( modelGetData( model ) ).to.equal( '<paragraph>f[o]o</paragraph><paragraph>b[a]r</paragraph>' );
expect( model.selection.isBackward ).to.true;
} );

it( 'should not enqueue changes if selection has not changed', () => {
const viewSelection = new ViewSelection();
viewSelection.addRange( ViewRange.createFromParentsAndOffsets(
viewRoot.getChild( 0 ).getChild( 0 ), 1, viewRoot.getChild( 0 ).getChild( 0 ), 1 ) );

convertSelection( null, { newSelection: viewSelection } );

const spy = sinon.spy();

model.on( 'changesDone', spy );

convertSelection( null, { newSelection: viewSelection } );

expect( spy.called ).to.be.false;
} );
} );
Loading