Opened 10 years ago
Closed 10 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 )
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 10 years ago by
Status: | new → confirmed |
---|
comment:2 Changed 10 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 10 years ago by
Owner: | set to Artur Delura |
---|---|
Status: | confirmed → assigned |
comment:4 Changed 10 years ago by
Description: | modified (diff) |
---|---|
Status: | assigned → review |
comment:5 Changed 10 years ago by
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 10 years ago by
Resolution: | → fixed |
---|---|
Status: | review → closed |
Fixed with git:ea48ac5.
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.