Move widget initialization from autoembed to widget repo's method

[Piotrek Koszuliński]

This is:

		var defaults = typeof widgetDef.defaults == 'function' ? widgetDef.defaults() : widgetDef.defaults,
			element = CKEDITOR.dom.element.createFromHtml( widgetDef.template.output( defaults ) ),
			instance,
			wrapper = editor.widgets.wrapElement( element, widgetDef.name ),
			temp = new CKEDITOR.dom.documentFragment( wrapper.getDocument() );

		temp.append( wrapper );
		instance = editor.widgets.initOn( element, widgetDef );

		if ( !instance ) {
			finalizeCreation();
			return;
		}

[Piotrek Koszuliński]

Just to clarify - we are talking here about a new method.

[Artur Delura]

Changes and tests in ​branch:t/13408.

[Piotr Jasiun]

New function is over-complicated and unclear.

You return object which, beside the instance, returns wrapperParent which is a temporary documentFragment, which needs to be used at some point later as a parameter of editor.widgets.finalizeCreation... It would be perfect if finalizeCreation would be called automatically and this method return just an instance.

Secondly, the method is called createFromTemplate but there is no template parameter and it is not mentioned in the description. I am not sure if it should really be called this way. Maybe createFromDefinion or simple create would be better? But if the template is needed in the definition to make this method work this should be mentioned in the documentation.

In fact description is very enigmatic: Creates a widget based on template and adds one to repository. Why I can not use simply widget constructor or initOn method? What is the difference?

I rebased the branch on the newest master.

[Piotrek Koszuliński]

Excerpt from my F2F discussion with @scofalik – the code was good and only some polish should be applied to it.

First of all – why there and not in the constructor or initOn()? Because it cannot be merged into some other function. This process includes many phases and for the sake of testability as well as composability and reusability, we should not create one monolithic method.

Then, there's the question about "is it a good API?". Well – it's definitely confusing. First of all – why the container? Why so many strange checks? Unfortunately, all this is needed in so generic API as widgets are. If we knew all these cases from the beginning we would definitely shape it differently, perhaps abstracted it. Right now we need to base on documentation.

So what I would see improved?

• The name of the method – create() will be enough. It should accept widget definition and startupData.
• Return value – { container, instance }.
• Documentation – I will write it during review.

[Szymon Cofalik]

I've pushed ​branch:t/13408 with the changes we discussed.

[Olek Nowodziński]

Rebased ​​branch:t/13408 on latest master.

I see like 20 red tests in ​http://tests.ckeditor.dev:1030/#tests/is:unit,path:/tests/plugins/widget,path:/tests/plugins/autoembed,is:failed (Chrome, Firefox).

[Szymon Cofalik]

Pushed fixed version on ​branch:t/13408. Sorry for that mistake. I re-run tests on all browsers and they seem fine now.

[Piotrek Koszuliński]

@a.nowodzinski - please, leave this the review to me because I have to rethink all the weird cases which may happen during widget initialisation and write docs for this.

PS. Postponing, because this is nothing crucial.

[Olek Nowodziński]

Replying to Reinmar:

@a.nowodzinski - please, leave this the review to me because I have to rethink all the weird cases which may happen during widget initialisation and write docs for this.

PS. Postponing, because this is nothing crucial.

Alright. Enjoy your toys :)

[Marek Lewandowski]

Moving tickets to the next milestone.

[Marek Lewandowski]

Moving to 4.6.2 minor release, as 4.6.1 is mostly about polishing 4.6.0.

[Marek Lewandowski]

This ticket has already nice rescheduling streak :D Moving it simply to our todo list, we'll get back to it as we have time, but it's not a crucial one for the time being.