Changed: Fixed detaching model.LiveRange in model.LiveSelection.

[scofalik]

Suggested merge commit message (convention)

Fix: model.LiveSelection now correctly stops listening to removed model.LiveRanges. Closes ckeditor/ckeditor5#4036.

---

Additional information

After a minute of monkey clicking, there is just one instance of LiveRange and also LiveSelection retains just 4kb of data.

[Reinmar]

I can see you used a different solution than we discussed initially. Was it due to those graveyard ranges or due to https://github.com/ckeditor/ckeditor5-utils/issues/144? If due to https://github.com/ckeditor/ckeditor5-utils/issues/144 then I would wait for that.

[scofalik]

Mostly due to linked issue. Graveyard ranges are whatever.

Other solution would be to do: this.stopListening(); this.off() in LiveRange#detach. But I've gone with solution that is more logical given current implementation of events -- since it's LiveSelection who attaches callback, it's LiveSelection who is wholly responsible to take it back.

[scofalik]

I've made a PR in utils so we can close this issue.

[scofalik]

I've updated the solution, but keep in mind that it needs ckeditor/ckeditor5-utils#145

[jodator]

This fix should work with changes in t/144-new.

The main difference that prevents memory leaks is changing this:

this.listenTo( liveRange, 'change:range' () => {} )

to:

liveRange.on( 'change:range', () => {} );

Above change allows live ranges to be garbage collected because it is important where events are physically stored.

As when using this.listenTo the DocumentSelection will hold references to LiveRanges. So one would have to on removing LiveRange:

liveRange.deatch(); // this is just `liveRange.stopListening()`
// but it only stop listening event registered on this emitter.
// so this would be also needed:
this.stopListening( liveRange );

[Reinmar]

This branch needs to be rebased in the first place :D

[jodator]

I'd simplify this change to:

_prepareRange( range ) {
    // ... rest
    liveRange.on( 'change:range', ( evt, oldRange, data ) => {
    // ... rest
}

on line:

ckeditor5-engine/src/model/documentselection.js

Line 328 in 598bc78

this.listenTo( liveRange, 'change:range', ( evt, oldRange, data ) => {

As problematic here is the size of DocumentSelection[ _listeingTo ] size that keeps growing with each selection change.

Before:

After:

The reason is as previously stated the place where listeningTo are stored - it's better to keep it on shorter-living entity or remember to call this.stopListening( emitter, 'event' ) every time emiter is not used any more. But doing emitter.on( 'event' ) will "do it for us" as it will not store reference to emitter on listener.

[scofalik]

AFAIR (and it was a long time ago) what you propose was my initial idea, but @Reinmar wanted a better solution that won't lead to problems in future (hence changes in EmitterMixin).

[Reinmar]

Discareded by #1181.