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

Introduce lock mechanism for Editor#isReadOnly #11441

Merged
merged 23 commits into from
Apr 4, 2022
Merged

Introduce lock mechanism for Editor#isReadOnly #11441

merged 23 commits into from
Apr 4, 2022

Conversation

ma2ciek
Copy link
Contributor

@ma2ciek ma2ciek commented Mar 9, 2022

Suggested merge commit message (convention)

Feature (core): Introduced locking mechanism for Editor#isReadOnly. Read-only mode can now be separately enabled and disabled by multiple features which allows for proper control without conflicts between features. Closes #10496.

Other (core): Editor#isReadOnly is now a read-only property.

MAJOR BREAKING CHANGE: The Editor#isReadOnly property is now read-only. From now, this property is controlled by Editor#enableReadOnlyMode( lockId ) and Editor#disableReadOnlyMode( lockId ), which allow controlling the Editor#isReadOnly state by more than one feature without collisions.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@ma2ciek ma2ciek requested a review from scofalik March 9, 2022 16:13
@pomek
Copy link
Member

pomek commented Mar 9, 2022

This major breaking change breaks our integrations that set editor.isReadOnly=true.

https://github.com/ckeditor/ckeditor5-react/blob/dc3093c98ef566a0a422413e326e4bd086ecd54a/src/ckeditor.jsx#L63-L65

Now, integrations have to support both forms - the previous (as mentioned above), and the new one (that the PR adds).

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Mar 9, 2022

Yes... Perhaps we can still support the old behavior and when encountering this kind of assignments we can try to set/remove a custom lock instead. I'll research it tomorrow.

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Mar 10, 2022

During a recent discussion with @scofalik we decided that we can do a breaking change and then release major releases in our integrations (the BC in these integrations will be the fact that they will be working only with CKEditor 5 v34+), so they will not have to support two versions at the same time.

Introducing a lock mechanism from custom assignments looks pretty bad, imagine this situation:

editor.isReadOnly = false; // clears a custom assignment lock on the `isReadOnly` property.

editor.isReadOnly // still `true` as other features may still have their locks on the `isReadOnly` property.

TBH, the fact that integrations also change the isReadOnly property is another reason of why this PR is important 😄

@pomek
Copy link
Member

pomek commented Mar 10, 2022

I have nothing to do with the API, as you know better what you want to achieve. I agree that the proposed method is much better than a single property.

However, we cannot assume people update their builds. They don't have to, and it's our responsibility to think about it. So, integrations must detect (somehow) a version of the CKEditor 5 API and, based on that, use old or new API.

In the worst scenario, if we cannot support both APIs, we need to release new versions of integrations with adjusted API.

@scofalik
Copy link
Contributor

In the worst scenario, if we cannot support both APIs, we need to release new versions of integrations with adjusted API.

But we will, it's obvious. If we make breaking change in our code, we will fix it :).

Copy link
Contributor

@scofalik scofalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like current method names, I propose:

  • setReadOnly( id ) and clearReadOnly( id ), or
  • enableReadOnly( id ) and disableReadOnly( id ).

I understand that setReadOnly( 'foo' ) looks like we are setting the property value to 'foo', hence the second proposal.

Other than that, maybe you could also check if manually firing set:... event has any change working with bind() and if there are other mechanisms that we should also consider.

*
* @param {String} lockId The lock ID for setting the editor to the read-only state.
*/
hasReadOnlyLock( lockId ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this method is actually needed after giving it a second thought. In overwhelmingly most cases, you set read only basing on another value/event, for example, when the internet connection is turned off, or when you switch on/off a feature, etc.

It looks like this is useful mostly for "toggle buttons" but such buttons do not really exist in real integrations. Do we need hasReadOnlyLock() in our own code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I remember there is no place where this method is used in the source code, but as for the tests there is currently no other way to write it without using the hasReadOnlyLock(). I was thinking about just using the Editor.isReadOnly in manual tests but then these buttons will look broken 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So without using hasReadOnlyLock we would have to either use internals, Editor.isReadOnly, I don't have other ideas.

ma2ciek and others added 6 commits March 23, 2022 07:42

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Szymon Cofalik <s.cofalik@cksource.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Szymon Cofalik <s.cofalik@cksource.com>
@scofalik
Copy link
Contributor

scofalik commented Mar 29, 2022

LGTM except of the new method names. 

I propose:

  • setReadOnly( id, value ) and clearReadOnly( id ), or
  • enableReadOnly( id, value ) and disableReadOnly( id ).

I also think that hasReadOnlyLock() is not needed at the moment. You can use local variable in manual tests.

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Mar 29, 2022

@ckeditor/qa-team  Can I ask you to test this PR? I changed also the ckeditor5-core/tests/manual/readonly test to allow testing setting a few locks at the same time.

Unfortunately, there are a lot of places where we use or observer editor.isReadOnly and set this property via editor.setReadOnly()

scofalik and others added 3 commits March 29, 2022 17:42

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@LukaszGudel
Copy link
Contributor

I've tested this PR, and also CF branch related to this changes and everything seems fine. The lock mechanism is working correctly. I've also tested CF features where read-only mode can be set and that was also working correctly 👍

@scofalik
Copy link
Contributor

Migration guide missing 😬

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Apr 1, 2022

Can we add this guide as a follow-up? The DLL changes already introduce the migration guide for the next iteration and I'd be happy to not merge these files if I do not have to :)

Copy link
Contributor

@scofalik scofalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reworded the migration guide a bit 😬

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@scofalik scofalik merged commit b0234d9 into master Apr 4, 2022
@scofalik scofalik deleted the cke/10496 branch April 4, 2022 11:46
@ma2ciek
Copy link
Contributor Author

ma2ciek commented Apr 4, 2022

I reworded the migration guide a bit 😬

I thought that it will be fine and you rewrote it from scratch ;p

@scofalik
Copy link
Contributor

scofalik commented Apr 4, 2022

It's not at all from scratch. In one place it looks like a lot of changes but I just changed the order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor#isReadOnly should work like disabling commands (blockable multiple times by different features)
4 participants