Opened 10 years ago
Last modified 10 years ago
#12929 confirmed Bug
Single entry point for command state
Reported by: | Piotr Jasiun | Owned by: | |
---|---|---|---|
Priority: | Normal | Milestone: | |
Component: | General | Version: | |
Keywords: | Cc: |
Description (last modified by )
The problem:
Now the command state (if it is disable or not) is based on the "setState" method which is called by many plugins. It can be changed when selection change, content in selection change (#12618), mode change, editor is in readOnly mode and maybe some more cases. Also many plugins set state of the commands manually. We get a state spaghetti and it is very often that one part of code try to disable command and another enable it back.
The solution:
There should be one entry point for all parts of code which want to change the state. Good candidate seems to be refresh
method and event. The example scenario can be readonly feature. It will:
- add
refresh
event listener and disable every command if editor is in the read only mode, - call
refresh()
function when editor switch to thereadonly
mode.
This way every time the state of the command is suppose to be changed all listeners are called and we can be sure that we do not enable feature which should be disabled.
In fact direct usage of setState()
is always wrong because we can not be sure if any other plugin want to change the state too.
The idea of changes is:
- mark
setState()
as deprecated, refresh
event getevt.data.state
parameter which can be changed by any listener.commandDefinition
methods (refresh
andexec
) are only (other thencommand.refresh()
) methods which can change state directly (by adding data.state parameter or returning new state),refresh()
method contents may be moved to therefresh
listeners, so it will be possible to add listener before them,- everything what
refresh()
will do is firingrefresh
event and setting the state based on what listeners will set.
This way if someone want to disable command and be sure no one else will enable it back, can add refresh
listener with 0
priority, set evt.data.state = CKEDITOR.TRISTATE_DISABLED
and cancel event.
It seems to be possible to apply these changes without breaking backward compatibility. setState
method will still work (as bad as it works now), but it will be deprecated. Also it is pretty huge change, because setState
is used in many plugins.
Change History (3)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
Status: | new → pending |
---|
Actually, I'm not sure if this ticket is valid. If what I wrote in http://dev.ckeditor.com/ticket/12618#comment:5 is true, then we won't need another mechanism for this. The problems I described in comment:1, if I understand correctly this ticket, are different.
comment:3 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Status: | pending → confirmed |
Summary: | Imrove command disabling mechanism → Single entry point for command state |
The command states problem is so wide that we would need to rework this system totally. I remember at least one ticket where after digging and digging we had to give up, because we couldn't find a solution. It was somewhere around 4.2.0 where @a.nowodzinski was working on indent buttons. I also remember a total confusion in some other cases, especially when I was working on the ACF which disables some commands.
So, do not have high hopes :D.