Some issues after recent EditorUI* refactoring

[Reinmar]

1. We did not discuss moving EditorUI to ckeditor5-core. I see why this was done – no reference to the ckeditor5-ui, plus the EditorWithUI interface is there. However, this is a false promise that this interface is a "generic UI" interface. Realistically, either all plugins use our UI (from the ground up) or a completely different UI lib and then they can also rethink how the "editor with UI" interface may need to look like.

   So, I actually think that this is a wrong direction. If anything, all those things should be moved to ckeditor5-ui.

2. We should make sure that all breaking changes are listed and in a way which will be understandable for people. Basically, this is not enough: ckeditor/ckeditor5-ui@b8cc937. It's not complete and does not help to understand what happened. Actually, this isn't even a correct merge commit. Since it was done already, please list all the breaking changes here in this ticket and I'll include them in the changelog.

3. Remove all occurrences of view.editableElement. It's defined only in ClassicTestEditorUI#init() and used in dozen of places.

[pjasiun]

1. We did not discuss moving EditorUI to ckeditor5-core. I see why this was done – no reference to the ckeditor5-ui, plus the EditorWithUI interface is there. However, this is a false promise that this interface is a "generic UI" interface. Realistically, either all plugins use our UI (from the ground up) or a completely different UI lib and then they can also rethink how the "editor with UI" interface may need to look like.

It was there. We did not move it there.

[Reinmar]

It was there. We did not move it there.

Ouch :D It was moved there in June 2018. I didn't notice this change back then. I still have mixed feelings but in such case let's not touch it now.

[f1ames]

2. We should make sure that all breaking changes are listed and in a way which will be understandable for people. Basically, this is not enough: ckeditor/ckeditor5-ui@b8cc937. It's not complete and does not help to understand what happened. Actually, this isn't even a correct merge commit. Since it was done already, please list all the breaking changes here in this ticket and I'll include them in the changelog.

😭 The commit message didn't make it to the merge commit somehow. I'm not very fluent in writing breaking changes messages, do we have any good examples? Or convention what should be described (I assume information about what had been changed/removed/introduce is valuable in such cases).

3. Remove all occurrences of view.editableElement. It's defined only in ClassicTestEditorUI#init() and used in dozen of places.

It is used only in tests (in 7 files to be precise), but yes, it should be replaced with getEditableElement() so we don't have any such references.

[f1ames]

It is used only in tests (in 7 files to be precise), but yes, it should be replaced with getEditableElement() so we don't have any such references.

I corrected the tests, see:

• Tests: Remove all usages of view.editableElement ckeditor5-core#160
• Tests: Remove all usages of view.editableElement ckeditor5-engine#1652
• Tests: Remove all usages of view.editableElement ckeditor5-typing#178
• Tests: Remove all usages of view.editableElement ckeditor5-ui#465

The above 4 PRs covers all view.editableElement references. Once merged, there should not be any view.editableElement references in the code.

[f1ames]

please list all the breaking changes here in this ticket and I'll include them in the changelog

@Reinmar Did you mean breaking changes related to ckeditor/ckeditor5-ui@b8cc937 or to all changes from PRs related to #1449 issue? There were 7 PRs in 7 different repositories and each has its own set of breaking changes. Should I revisit all?

[oleq]

I closed all PRs. What is left is revisiting the BREAKING CHANGES brought by #1449 constellation.

[Reinmar]

@f1ames:

Those changes are well documented and merged correctly. So it's fine.

So I think it's mostly about ckeditor/ckeditor5-ui@b8cc937.

Regarding examples, you can see many in https://github.com/ckeditor/ckeditor5-engine/blob/master/CHANGELOG.md. The main idea is that it's not enough to describe what changed. But it also needs to be clear how to modify your plugin/editor/integration to align your code to the changes. Sometimes it requires writing this explicitly, sometimes the change itself is clear enough that it does not require additional more information, sometimes it's just a matter of mentioning some other/new API fragment or construct.

[Reinmar]

@f1ames, if you list me here the changes made in ckeditor/ckeditor5-ui@b8cc937 I'll review them here and let you know what we could write.

[f1ames]

The biggest change was refactoring of EditableUIView class which had some redundant properties:

• The externalElement property (which held the element on which editor was created if it was explicitly passed) was removed. It was a public property but wasn't used anywhere in our code.
• In place of externalElement property, _hasExternalElement private property was introduced for internal processing.
• The editableElement property was made private because it points to the same element as editor.ui.getEditableElement() method and should not be accessed directly.
• The changes in docs are adjustments to the entire refactoring and only small part is related to the above EditableUIView changes.

So from my perspective the breaking changes are externalElement property removal and making editableElement property private. Instead of accessing editor.ui.view.editableElement editor.ui.getEditableElement() method should be used, so maybe:

BREAKING CHANGE: The EditableUIView.editableElement property was removed from the public API. To access editor editable element use editor.ui.getEditableElement() method.

The situation is similar with externalElement. If any element was passed to the EditableUIView constructor it can be also accessed via EditorUI#getEditableElement(), so maybe:

BREAKING CHANGE: The EditableUIView#externalElement was removed from the public API. To access editor editable element use editor.ui.getEditableElement() method.

Not sure about the second one, because it seems redundant, but that was the case basically, that those two properties held the same element (apart from the case where externalElement was undefined because nothing was passed).

cc @Reinmar

[Reinmar]

Thanks!