Create writer for non-semantic view manipulation #1503

f1ames · 2018-08-16T15:31:42Z

Suggested merge commit message (convention)

Internal: Introduced writer for manipulating non-semantic view. Closes ckeditor/ckeditor5#4399.

Additional information

I'm still not sure about naming, the name I used is RawWriter, but we have discussed also Upcast/Downcast writer. Maybe looking at the code it will be easier to decide. I'm not in favor of any, upcast/downcast doesn't fit that much as this writer has basically nothing to do with upcasting/downcasting (apart from the fact that it touches view instance which may get upcasted eventually - but lots of classes do). And raw is somehow descriptive, but still you need to know the context well.

pjasiun · 2018-08-23T13:07:33Z

With @Reinmar and @scofalik we just talked and agreed that UpcastWriter and DowncastWriter will be the best name. Also, I think that both will need to have improved documentation to explain cases where they should be used.

pjasiun · 2018-08-23T13:09:59Z

Also, I disagree that the writer has nothing to do with downcasting, because if it used a lot during downcasting. It is as important for downcasting as converters, doing a big part of creating view during downcasting.

f1ames · 2018-08-23T13:34:28Z

With @Reinmar and @scofalik we just talked and agreed that UpcastWriter and DowncastWriter will be the best name.

I'm fine with it, just one clarification - so the existing Writer should be UpcastWriter and the one introduced in this PR should be DowncastWriter?

Also, I disagree that the writer has nothing to do with downcasting, because if it used a lot during downcasting. It is as important for downcasting as converters, doing a big part of creating view during downcasting.

That's true for the current Writer. But when it comes to writer introduced in this PR it has nothing to do with model, it just manipulates the view - so basically it may work on a view instance which will be upcasted to model or which was downcasted from it, but it does not participate in any way in the process of upcasting/downcasting itself.

pjasiun · 2018-08-23T14:01:41Z

I'm fine with it, just one clarification - so the existing Writer should be UpcastWriter and the one introduced in this PR should be DowncastWriter?

Nope. The existing one (Writer) should be DowncastWriter, the new one (RawWriter) should be UpcastWriter.

pjasiun · 2018-08-23T14:04:59Z

That's true for the current Writer. But when it comes to writer introduced in this PR it has nothing to do with model, it just manipulates the view - so basically it may work on a view instance which will be upcasted to model or which was downcasted from it, but it does not participate in any way in the process of upcasting/downcasting itself.

That's true, but we could bend the definition a little for this case. How, upcasting is View -> Model, but it could be Data -> View -> Model (downcasting Model -> View -> Data). Then it will fit well.

f1ames · 2018-08-24T07:33:27Z

That's true, but we could bend the definition a little for this case. How, upcasting is View -> Model, but it could be Data -> View -> Model (downcasting Model -> View -> Data). Then it will fit well.

Yes, it makes sense and makes it easier to understand with upcasting/downcasting defined in such way 👍

f1ames · 2018-08-27T08:15:29Z

Also, I think that both will need to have improved documentation to explain cases where they should be used.

@pjasiun I will update the naming and docs and ping you when it's ready.

coveralls · 2018-08-27T09:16:48Z

Coverage increased (+0.004%) to 98.621% when pulling 6712b25 on t/1501 into 7d8de56 on master.

f1ames · 2018-08-27T10:02:07Z

@pjasiun I have renamed RawWriter to UpcastWriter. Now I'm working on renaming Writer to DowncastWriter, however I will put it in a separate PR as there will be plenty of changes.

So this PR is ready for R.

scofalik · 2018-08-30T09:47:12Z

src/view/upcastwriter.js

+	 * @param {module:engine/view/element~Element|module:engine/view/documentfragment~DocumentFragment} element Element
+	 * to which items will be appended.
+	 * @param {module:engine/view/item~Item|Iterable.<module:engine/view/item~Item>} items Items to be inserted.
+	 * @fires module:engine/view/node~Node#change


We (at least I ;)) usually write event:change. Does the line above create proper docs?

BTW., those change events on view tree are so internal that I am not sure if it is worth documenting. @pjasiun ?

From what I see, both produce the same link .../module_engine_view_node-Node.html#event:change but I will update it to make it consistent.

However, I just discovered it doesn't work properly in our docs 😱 The generated links are e.g. ...api/module_engine_conversion_upcastdispatcher-UpcastDispatcher.html#event:element, while to working one is ...api/module_engine_conversion_upcastdispatcher-UpcastDispatcher.html#event-element (so - instead of : in event name). I will report it.

scofalik · 2018-08-30T09:52:02Z

src/view/upcastwriter.js

+	 * @see module:engine/view/element~Element#_insertChild
+	 * @param {module:engine/view/element~Element|module:engine/view/documentfragment~DocumentFragment} element Element
+	 * to which items will be inserted.
+	 * @param {Number} index Position where nodes should be inserted.


Offset at which?

scofalik · 2018-08-30T09:54:12Z

src/view/upcastwriter.js

+	 * @see module:engine/view/element~Element#_removeChildren
+	 * @param {module:engine/view/element~Element|module:engine/view/documentfragment~DocumentFragment} element Element
+	 * from which children will be removed.
+	 * @param {Number} index Number of the first node to remove.


Offset from which?

scofalik · 2018-08-30T09:54:33Z

src/view/upcastwriter.js

+	 * @param {Number} index Number of the first node to remove.
+	 * @param {Number} [howMany=1] Number of nodes to remove.
+	 * @fires module:engine/view/node~Node#change
+	 * @returns {Array.<module:engine/view/node~Node>} The array of removed nodes.


containing?

scofalik · 2018-08-30T10:05:44Z

src/view/upcastwriter.js

+	 * Removes given element from the view structure. Will not have effect on detached elements.
+	 *
+	 * @param {module:engine/view/element~Element} element Element which will be removed.
+	 * @returns {Array.<module:engine/view/node~Node>} The array of removed nodes.


with removed nodes?

scofalik · 2018-08-30T10:07:28Z

src/view/upcastwriter.js

+	 */
+	remove( element ) {
+		const parent = element.parent;
+		if ( parent ) {


New line please? :)

scofalik · 2018-08-30T10:07:58Z

src/view/upcastwriter.js

+	 * Replaces given element with the new one in the view structure. Will not have effect on detached elements.
+	 *
+	 * @param {module:engine/view/element~Element} oldElement Element which will be replaced.
+	 * @param {module:engine/view/element~Element} newElement Element which will inserted in the place of the old element.


scofalik · 2018-08-30T10:09:30Z

src/view/upcastwriter.js

+	}
+
+	/**
+	 * Adds or overwrite element's attribute with a specified key and value.


overwrites (I see that this is a typo also at the place you copied from :P)

scofalik · 2018-08-30T10:17:03Z

I have one overall issue: the parameters in methods in this class have a different order than the parameters in DowncastWriter. There we follow the rule that element is the last parameter. Here it is first. Sorry.

pjasiun · 2018-08-30T10:28:35Z

Note that DOMConverter should now use this writer. In can be changed as a part of this ticket or reported as a follow-up.

f1ames · 2018-08-30T10:37:49Z

I have one overall issue: the parameters in methods in this class have a different order than the parameters in DowncastWriter. There we follow the rule that element is the last parameter. Here it is first.

Yes... The reason I did it that way is that in a former Writer (DowncastWriter) class, the only methods which has element as a parameter are set/remove ones (the other usually have position or range, etc) and it is indeed the last parameter there.

However, in the UpcastWriter all methods have one parameter in common which is an element and the rest changes, e.g.:

clone( element, deep = false ) { ... }
appendChild( element, items ) { ... }
removeChildren( element, index, howMany = 1 ) { ... }
rename( element, newName ) { ... }

So:

It makes it consistent internally, since all methods needs element and it is always the first parameter.
To keep it consistent with DowncastWriter, element parameter should be the last parameter, but then for methods with default parameters it actually can't be the last keeping things inconsistent, like:
- clone( element, deep = false ) { ... }
- rename( newName, element ) { ... }
- removeChildren( index, element, howMany = 1 ) { ... }
- insertChild( index, items, element ) { ... }

From my perspective it is a choice between keeping it internally consistent or trying to make it consistent with some DowncastWriter methods by making it internally inconsistent. WDYT @scofalik ?

Btw. DowncastWriter has also a rename method with rename( viewElement, newName ) { ... } signature (I know it is viewElement not element but still).

f1ames · 2018-08-30T10:43:34Z

Note that DOMConverter should now use this writer. In can be changed as a part of this ticket or reported as a follow-up.

I will propose a follow-up issue. 🤔 @pjasiun I don't see DOMConverter using Writer internally, could you elaborate a little where exactly UpcastWriter should be used?

scofalik · 2018-08-30T10:52:41Z

Wasn't DOMConverter using Elements API? I think that's what PJ might have on his mind.

pjasiun · 2018-08-30T10:55:54Z

Yep. DOM converter now is using protected elements API and it should use this writer instead.

scofalik · 2018-08-30T11:46:09Z

@f1ames Correct me if I am wrong, but AFAICS the methods that are not available in DowncastWriter are:

clone( element, deep = false )
appendChild( element, items )
insertChild( element, index, items )
removeChildren( element, index, howMany = 1 )
remove( element )
replace( oldElement, newElement )
rename( element, newName )

So if we would change set/removes, those are which would be "inconsistent". Let's break them down.

clone() - can't do much here, but since deep really is an optional parameter, I think it is fine.
appendChild() and insertChild() make sense with a different parameters order too.
removeChildren looks bad, TBH, but I'd drop default value there and make element the last parameter. This is the only method that IMO will look bad.
remove() is inconsistent only because downcastWriter takes a range only. TBH, I'd change downcastWriter to accept both range and element and simply check if element is passed, if so, create a range on it (Range.createOn()). When this is done, the methods are consistent enough to get a thumbs up from me.
replace() is fine as it is as it takes two elements, however, to be consistent, oldElement probably should be the second parameter, but I don't like it that way. I'd leave it as it is.
rename() can be changed to be consistent and it will still look okay.

scofalik · 2018-08-30T11:46:50Z

Sorry, rename() is available in DowncastWriter and has wrong order. I'd change the order there.

f1ames · 2018-08-31T08:53:21Z

remove() is inconsistent only because downcastWriter takes a range only. TBH, I'd change downcastWriter to accept both range and element and simply check if element is passed, if so, create a range on it (Range.createOn()). When this is done, the methods are consistent enough to get a thumbs up from me.

rename() is available in DowncastWriter and has wrong order. I'd change the order there.

Makes sense. I will extract those changes to a separate issue to not block this PR for too long ;)

f1ames · 2018-08-31T08:54:03Z

As I applied all requested changes, it is ready another R.

f1ames mentioned this pull request Aug 23, 2018

Support for pasting flat lists ckeditor/ckeditor5-paste-from-office#7

Merged

f1ames added 5 commits August 27, 2018 11:09

Added writer for manipulating non-semantic view.

b6948f0

Tests: writer unit tests - WIP.

e098d3e

Writer docs rewording and code adjustments.

f75d8d6

Tests: writer unit tests - 100% cc.

1cfe049

Renamed 'RawWriter' to 'UpcastWriter'.

d0ea328

f1ames force-pushed the t/1501 branch from 3e22bf9 to d0ea328 Compare August 27, 2018 09:10

Docs rewording.

9cc3729

pjasiun requested a review from scofalik August 30, 2018 08:23

scofalik reviewed Aug 30, 2018

View reviewed changes

scofalik added the status:review- label Aug 30, 2018

f1ames added 2 commits August 31, 2018 09:31

Params order refactoring.

ac4b0cb

New params order tests and code adjustments.

6712b25

f1ames removed the status:review- label Aug 31, 2018

scofalik merged commit f66e722 into master Aug 31, 2018

scofalik deleted the t/1501 branch August 31, 2018 12:49

f1ames mentioned this pull request Oct 9, 2019

Make Upcast and Downcast writers more consistent ckeditor/ckeditor5#4409

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create writer for non-semantic view manipulation #1503

Create writer for non-semantic view manipulation #1503

f1ames commented Aug 16, 2018 •

edited by pomek

Loading

pjasiun commented Aug 23, 2018

pjasiun commented Aug 23, 2018

f1ames commented Aug 23, 2018

pjasiun commented Aug 23, 2018

pjasiun commented Aug 23, 2018

f1ames commented Aug 24, 2018

f1ames commented Aug 27, 2018

coveralls commented Aug 27, 2018 •

edited

Loading

f1ames commented Aug 27, 2018

scofalik Aug 30, 2018

f1ames Aug 31, 2018

scofalik Aug 30, 2018 •

edited

Loading

scofalik Aug 30, 2018

scofalik Aug 30, 2018

scofalik Aug 30, 2018

scofalik Aug 30, 2018

scofalik Aug 30, 2018

scofalik Aug 30, 2018

scofalik commented Aug 30, 2018

pjasiun commented Aug 30, 2018

f1ames commented Aug 30, 2018 •

edited

Loading

f1ames commented Aug 30, 2018 •

edited

Loading

scofalik commented Aug 30, 2018

pjasiun commented Aug 30, 2018

scofalik commented Aug 30, 2018 •

edited

Loading

scofalik commented Aug 30, 2018

f1ames commented Aug 31, 2018

f1ames commented Aug 31, 2018

Create writer for non-semantic view manipulation #1503

Create writer for non-semantic view manipulation #1503

Conversation

f1ames commented Aug 16, 2018 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

pjasiun commented Aug 23, 2018

pjasiun commented Aug 23, 2018

f1ames commented Aug 23, 2018

pjasiun commented Aug 23, 2018

pjasiun commented Aug 23, 2018

f1ames commented Aug 24, 2018

f1ames commented Aug 27, 2018

coveralls commented Aug 27, 2018 • edited Loading

f1ames commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scofalik commented Aug 30, 2018

pjasiun commented Aug 30, 2018

f1ames commented Aug 30, 2018 • edited Loading

f1ames commented Aug 30, 2018 • edited Loading

scofalik commented Aug 30, 2018

pjasiun commented Aug 30, 2018

scofalik commented Aug 30, 2018 • edited Loading

scofalik commented Aug 30, 2018

f1ames commented Aug 31, 2018

f1ames commented Aug 31, 2018

f1ames commented Aug 16, 2018 •

edited by pomek

Loading

coveralls commented Aug 27, 2018 •

edited

Loading

scofalik Aug 30, 2018 •

edited

Loading

f1ames commented Aug 30, 2018 •

edited

Loading

f1ames commented Aug 30, 2018 •

edited

Loading

scofalik commented Aug 30, 2018 •

edited

Loading