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

[scofalik]

📝 Provide a description of the improvement

At the moment, Editor#isReadOnly is a simple property that can be set to either true or false. This is limiting and leads to incorrect code.

Consider two mechanisms (features), that may want to set editor to readonly mode. At the beginning, both of them may decide that "there's some kind of error" and switch editor to read-only. But then, one problem is solved, so one of the mechanisms switches back isReadOnly to true. This is incorrect, because the other mechanism still wants to block the editor.

This is the same situation as with commands, where multiple features may want to set a command as disabled.

My proposal is to:

• change Editor#isReadOnly to ... wait for it ... readonly property.
• add Editor#setReadOnly( id:String|Symbol ) and Editor#unsetReadOnly( id ) or (unlockReadOnly( id )) -- similarly as in commands,
• fix all places where we set Editor#isReadOnly.

One note is that Editor#isReadOnly could also be directly linked with document editing permission. Maybe document editing permission should be the only thing that actually changes Editor#isReadOnly and it should be dynamically affected by other mechanisms/features? However, I don't think that this really makes sense. For example, consider losing connection to Cloud Services. In such situation, we switch the editor to read-only. I don't think that it would make sense to switch user permission to "cannot edit" in this situation.

[mlewand]

I wonder if we could somehow avoid breaking changes on this one but I don't think that's possible. The only level of backward compatibility would be to clear all registered locks on doing ediotr.isReadOnly = false but this defeats the purpose of this request.

BTW, to be consistent with what we did already we should go with clearReadOnly() name (as there's clearForceDisabled() in command).

[scofalik]

Maybe we could provide a mixin that could handle that for other classes as well. This is a common pattern in our code.

[scofalik]

We can also leave setting .isReadOnly to true / false as something deprecated (+ log warning on console) and it would work as it did (i.e., setting to false would remove all the locks and setting to true would add one lock). It won't work worse than it does now. We would have a proper logic in our features and those users that used .isReadOnly in their 3rd party plugins would not need to fix the code immediately.

Although personally, I am against such things, for multiple reasons: first, it creates hidden problems, second it will have to be done at some point anyway, third it makes our code messy, fourth it discourages users to update their code ("okay, I'll fix it later" -> forgetting that there's something to fix -> looking way back in the changelog what changed after the deprecated code is removed).

[pomek]

Why does the Editor#enableReadOnlyMode() accept a second parameter that can be set to false? It looks weird: editor.enableReadOnlyMode( 'foo', false ).

We should have only two methods:

• Editor#enableReadOnlyMode( lockId ) to create a new lock.
• Editor#disableReadOnlyMode( lockId ) to remove the lock.

Then, if we think that the "toggle" method is needed, it should be implemented as a new thing: editor.setReadOnlyMode( lockId, isEnabled )

[scofalik]

It was to simplify the most common usage:

myFeature.on( 'change:someValue', () => {
    editor.enableReadOnlyMode( 'myId', myFeature.someValue );
} );

I agree that it looked better when the method was called setReadOnly() (or setReadOnlyMode()).

[mlewand]

We stick to enableReadOnlyMode + disableReadOnlyMode.

But enableReadOnlyMode will not have this second parameter that confused some of us.