Opened 4 years ago

Closed 4 years ago

#12332 closed Task (fixed)

Undo Manager should have lower listener priorities

Reported by: Marek Lewandowski Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.4.5
Component: Core : Undo & Redo Version: 4.4.4
Keywords: Cc:

Description (last modified by Artur Delura)

This is a follow up of #12327.

Current implementation is really fragile to code execution order. If undo manager is initialised before some code that cancels keydown undo manager's listener will be executed, but if the order is opposite, then it won't.

The listeners should be added with low priorities (999), so they are always handled at the end - if nothing else handled them.

Changes in branch:t/12332.

Change History (6)

comment:1 Changed 4 years ago by Marek Lewandowski

Status: newconfirmed

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

Description: modified (diff)

comment:3 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:4 Changed 4 years ago by Artur Delura

Description: modified (diff)
Status: assignedreview

Undo manager bind events with priority 10(default). There is a chance that some plugin bind same event with same priority. So now callback execution depends on order of binding callbacks to events. There is a chance that callback in undo plugin is called first and then other callback is called and stop() event. In this situation our callback shouldn't be called. To avoid such situation we lower priorities of callbacks in undo plugin - just to be sure that stopping event in other part of code prevent our callback execution.

How about:

I think we could see at this problem from global perspective. This problem could be handled in events library. I.e. throw an error/warning when there are two callbacks binded with the same priority, and one of them stop() event execution. Of course only in development version.

Or to provide extra flag which inform that some callback can stop() event. And then this callbacks will be handled first.

Changes in branch:t/12332.

Last edited 4 years ago by Artur Delura (previous) (diff)

comment:5 Changed 4 years ago by Piotrek Koszuliński

I think we could see at this problem from global perspective. This problem could be handled in events library. I.e. throw an error/warning when there are two callbacks binded with the same priority, and one of them stop() event execution. Of course only in development version.

In some (or even most) cases it will be totally correct. As long as the order of listeners does not depend on some external condition, like plugins loading order, this situation is clear and stable.

Or to provide extra flag which inform that some callback can stop() event. And then this callbacks will be handled first.

Then, why not using high priority, instead of setting this argument?

I don't see a big problem here as long as we are aware of possible problems and we prevent them (just like in this ticket).

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

Resolution: fixed
Status: reviewclosed

Fixed with git:ea48ac5.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy