Support adding 2+ items at once to Collection and FocusTracker

[Reinmar]

Code from the split button view:

this.children.add( this.actionView );
this.children.add( this.arrowView );
this.focusTracker.add( this.actionView.element );
this.focusTracker.add( this.arrowView.element );

cc @oleq

[ConnorSinnott]

Am I also correct in assuming that Collection cant be instantiated with an array? I wish to take a pre-existing array and move it into a collection. The only way I seem to be able to achieve this is

const messages = ['One','Two','Three'];

// Incorrect
const collection = new Collection(messages);

// Correct?
const collection = new Collection();
messages.forEach(m => collection.add(m)); 

This enhancement would help with the above issue, allowing me to remove the forEach, but its strange I cant pass the array to the constructor.

[oleq]

I agree this should be the right way to do it. @mlewand?

[mlewand]

Allowing for value members initialization thorough the constructor sounds like a nice addition. In this case options would become a second parameter, but code would be backward compatible and still detect options passed as the first parameter (another option though would be simply static Collection.from, but constructor has a much better discoverability). But it does not solve the original problem of this issue as we're not calling a constructor there.

It might be tempting to just allow Collection.add to get an array in which case it would automatically spread it and insert, but that's a no-no as Collections might already be used to store arrays, and it would break this code.

How about adding a Collection.push that would act as a Array.push counterpart? In this case we would go with

this.children.push( this.actionView, this.actionView );

And if one wants to append an array at a runtime, then it could be used like:

this.children.push( ...[ this.actionView, this.actionView ] );

[oleq]

It might be tempting to just allow Collection.add to get an array in which case it would automatically spread it and insert, but that's a no-no as Collections might already be used to store arrays, and it would break this code.

A good catch!

OTOH, I ran all the editor tests and checked if we use Arrays as Collection items and there's no such use–case. So maybe this not a bad idea after all.

Anyway, we could allow Collection.add( itemA, itemB, itemC, ..., index ). Since collection items must be Objects, we can clearly tell when it comes to the index. This way you can Collection.add( ...[ someArray ], index ) and the problem is gone.

TL;DR, I propose two changes to the API:

new Collection( Array, [ options] );
collection.add( itemA, itemB, itemC, ..., index );

cc @jodator

[jodator]

Yeah, since we have collection.add() not .push() so the proposed API is nice :). Also the constructor should allow adding array to seed the collection 👍

[ConnorSinnott]

TL;DR, I propose two changes to the API:

new Collection( Array, [ options] );
collection.add( itemA, itemB, itemC, ..., index );

cc @jodator

Just to confirm because this is your proposal, options would be housed in an array?

[jodator]

Just to confirm because this is your proposal, options would be housed in an array?

I get that as an optional parameter (as one would read this in the docs):
https://github.com/ckeditor/ckeditor5-utils/blob/2c46d4e1c66630f855ba1a311bb9f8e90b9040e4/src/collection.js#L31

Also other parts of the code use object for options.

[ConnorSinnott]

Ah! Apologies. I had taken

new Collection( Array, [ options] );
collection.add( itemA, itemB, itemC, ..., index );

literally. I didn't realize in this context the braces were indicating options as an optional parameter.

Thank you