Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Post-fixer: create paragraph in empty root. #3301

Closed
scofalik opened this issue Apr 12, 2017 · 8 comments · Fixed by ckeditor/ckeditor5-paragraph#21
Closed

Post-fixer: create paragraph in empty root. #3301

scofalik opened this issue Apr 12, 2017 · 8 comments · Fixed by ckeditor/ckeditor5-paragraph#21

Comments

@scofalik
Copy link
Contributor

@scofalik scofalik commented Apr 12, 2017

Original issue: #331

The "quick win" solution was already proposed by @Reinmar - on changesDone check if there are any contents in editor and if not, add <paragraph> to the model.

As I was about to implement this solution one thing came to my mind: we could have multiple roots. Since paragraph feature should be universal I think it should be ready to handle editors with multiple roots. So my idea is to check all the roots on changesDone (except of graveyard root) and fix all roots that are empty (and can have <paragraph> element according to Schema).

@scofalik

This comment has been minimized.

Copy link
Contributor Author

@scofalik scofalik commented Apr 12, 2017

It appears that even the easiest solution is more complicated than it looks on surface:

  1. Post-fixing will create an undo step: https://github.com/ckeditor/ckeditor5-engine/issues/921
  2. If we use changesDone, the <paragraph> will be added only after editor.setData() is called. And setting data does not even happen when initing core.StandardEditor. I guess that at some point changesDone will be finally called, but maybe the fixer should also be called on Editor#event:dataReady? But what if someone will create root after the editor is inited?

😱 This was supposed to be an easy issue 😱

@scofalik

This comment has been minimized.

Copy link
Contributor Author

@scofalik scofalik commented Apr 12, 2017

And last but not least, the editor may have unexpected behavior in tests, where we usually have empty editor and want to test whether some feature properly adds or converts something. Test already fail in heading and list feature because of it.

Do we still want to go with this solution?

@Reinmar

This comment has been minimized.

Copy link
Member

@Reinmar Reinmar commented Apr 13, 2017

TBH, I don't understand. Can't we adjust the last batch on changesDone to not create a new undo step? Initial data loading is not causing an undo step anyway and later on editor can be emptied only by some new batch so we can change exactly that batch.

Is https://github.com/ckeditor/ckeditor5-engine/issues/921 exactly about it?

Regarding failing tests, etc. I'd need to see them. We will need this behaviour coded in some way anyway. It may be done on the model layer, on the view layer, even on rendering layer – but still, it will affect some tests.

@scofalik

This comment has been minimized.

Copy link
Contributor Author

@scofalik scofalik commented Apr 13, 2017

TBH, I don't understand. Can't we adjust the last batch on changesDone to not create a new undo step?

Is ckeditor/ckeditor5-engine#921 exactly about it?

Yes, we don't have an access to last batch on changesDone ATM.

@scofalik

This comment has been minimized.

Copy link
Contributor Author

@scofalik scofalik commented Apr 13, 2017

Regarding failing tests, etc. I'd need to see them.

I've pushed t/19.

@scofalik

This comment has been minimized.

Copy link
Contributor Author

@scofalik scofalik commented Apr 27, 2017

I manage to did it without adding "last batch" to the document (no changes in engine were needed). Current solution may be not the best when it comes to efficiency but it required least development.

There are two listeners. First listens to change event and mark an empty root if a change made it empty. Also it saves a batch which made that root empty. Then on changesDone all marked roots are fixed and the fixing delta is added to appropriate batch. Works for multiple roots, added tests that check undo integration.

@Reinmar

This comment has been minimized.

Copy link
Member

@Reinmar Reinmar commented Apr 27, 2017

I manage to did it without adding "last batch" to the document (no changes in engine were needed). Current solution may be not the best when it comes to efficiency but it required least development.

But if we need the last batch anyway and plan to implement it anyway, then wouldn't it be better to have it first?

@scofalik

This comment has been minimized.

Copy link
Contributor Author

@scofalik scofalik commented Apr 27, 2017

It works without it. I don't know how many features will need this, because it appears that this is more complicated than just "having last batch". For example if two operations in different batches changed different roots you probably would run into problems if you add both fixing deltas to one batch:

  1. Batch 1: remove all contents from root 1
  2. Batch 2: remove all contents from root 2
  3. Batch 2: add paragraph in root 1
  4. Batch 2: add paragraph in root 2

Undo batch 2 -> will correctly undo root 2 but will also remove paragraph from root 1. Then root 1 will be fixed again and that change will be added to "undo batch" which lands on redo stack. Another undo will add content removed in operation 1 but extra paragraph would stay.

I don't know if that would end up exactly like this cause I didn't test (writing from top off my head) but I can imagine problematic scenarios. (If other features would need "last batch" for something and then would add fixes to wrong batches).

If we (...) plan to implement it anyway, then wouldn't it be better to have it first?

We don't plan to implement it. Yet :). We've just seen that it might be helpful sometimes but as proved above maybe we were wrong. I even thought that maybe each root could have last batch but then it is skewed because of this concrete problem that we are trying to solve in this issue. Also roots may have cross-linking operations.

Reinmar referenced this issue in ckeditor/ckeditor5-paragraph Apr 28, 2017
Feature: Paragraph will be automatically created if loaded empty data or if programmatically emptied a root element. Closes #19.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-paragraph Oct 9, 2019
@mlewand mlewand added this to the iteration 11 milestone Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.