Opened 6 years ago

Last modified 6 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 Piotr Jasiun)

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 the readonly 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 get parameter which can be changed by any listener.
  • commandDefinition methods (refresh and exec) are only (other then command.refresh()) methods which can change state directly (by adding data.state parameter or returning new state),
  • refresh() method contents may be moved to the refresh listeners, so it will be possible to add listener before them,
  • everything what refresh() will do is firing refresh 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 = 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 6 years ago by Piotrek Koszuliński

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.

comment:2 Changed 6 years ago by Piotrek Koszuliński

Status: newpending

Actually, I'm not sure if this ticket is valid. If what I wrote in 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 6 years ago by Piotr Jasiun

Description: modified (diff)
Status: pendingconfirmed
Summary: Imrove command disabling mechanismSingle entry point for command state
Note: See TracTickets for help on using tickets.
© 2003 – 2021 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy