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

Removed the "model.Writer#setTextData()" method #1394

Merged
merged 7 commits into from
Apr 5, 2018
Merged

Removed the "model.Writer#setTextData()" method #1394

merged 7 commits into from
Apr 5, 2018

Conversation

pomek
Copy link
Member

@pomek pomek commented Apr 4, 2018

Suggested merge commit message (convention)

Other: Removed the unnecesary model.Writer#setTextData() and view.Writer#setTextData() methods. Closes ckeditor/ckeditor5#4316.

Should be merged with ckeditor/ckeditor5-list#102.

@coveralls
Copy link

coveralls commented Apr 4, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling f67fd27 on t/1363 into 09486ce on master.

@Reinmar
Copy link
Member

Reinmar commented Apr 4, 2018

You didn't check whether this method is used somewhere... and it is.

@scofalik, I checked that we can't simply remove this bit: https://github.com/ckeditor/ckeditor5-list/blob/d2023267caa88eb383b1fab773e137c233cf5b8d/src/converters.js#L455-L462

Do you think that we could use writer.remove() there to remove these trailing spaces?

@Reinmar
Copy link
Member

Reinmar commented Apr 4, 2018

BTW, it seems that we can also fix https://github.com/ckeditor/ckeditor5-list/issues/101 easily because it should also have access to the writer.

@pomek
Copy link
Member Author

pomek commented Apr 5, 2018

You didn't check whether this method is used somewhere... and it is.

So the view.Writer#setTextData() isn't used anywhere.

@Reinmar
Copy link
Member

Reinmar commented Apr 5, 2018

So the view.Writer#setTextData() isn't used anywhere.

May be true. We both got a bit confused when discussing this in #1363 with @pjasiun.

Anyway, the model.Writer#setTextData() should be removed not because it's barely used, but because it's dangerous. You cannot change the text like this in the model.

@pomek
Copy link
Member Author

pomek commented Apr 5, 2018

After removing setTextData, what should we do in lists?

Remove the child of Text and insert a new with trimmed value?

@Reinmar
Copy link
Member

Reinmar commented Apr 5, 2018

Model writer offers a simple writer.remove() method which accepts a range. Based on this you can remove a piece of text. I think that it will be the most optimal solution.

A simpler solution would indeed be replacing an entire text node, but then we need to take care of its attributes. That's a bit worse to me.

@pomek
Copy link
Member Author

pomek commented Apr 5, 2018

Everything sounds good but the write instance is the instance of the model but the child is an instance of the view. So at this moment, model.setTextData() changes value of the view.

@scofalik
Copy link
Contributor

scofalik commented Apr 5, 2018

I am for model.Writer#remove if it wasn't resolved yet.

Edit: To be honest, just removing and adding a text node might probably by simpler than evaluating a range and passing to the writer. Why are you worried about attributes? You are processing child-after-child, so every child has different attributes.

Edit: Now I got what you wrote @pomek. This is weird. Let me look at this.

@Reinmar
Copy link
Member

Reinmar commented Apr 5, 2018

Everything sounds good but the write instance is the instance of the model but the child is an instance of the view. So at this moment, model.setTextData() changes value of the view.

WAT... You're right. Should converters actually modify the view being converted? It doesn't seem like a big problem, but it's certainly surprising.

Also, is the view writer available there? I guess not.

@Reinmar
Copy link
Member

Reinmar commented Apr 5, 2018

OK, this is a deeper problem. And it's the same problem as in https://github.com/ckeditor/ckeditor5-list/issues/101 – we need a view writer somewhere. And we need a place to clean up the view being converted.

For now, let's use protected methods of view text nodes to change their data. This will be in line with what we have reported in https://github.com/ckeditor/ckeditor5-list/issues/101.

@pomek
Copy link
Member Author

pomek commented Apr 5, 2018

ckeditor/ckeditor5-list#102

This PR is ready to review once again.

PS: I fixed failed, not related to my changes test.

@Reinmar Reinmar merged commit b484822 into master Apr 5, 2018
@Reinmar Reinmar deleted the t/1363 branch April 5, 2018 10:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should model.Writer#setTextData() be removed?
4 participants