Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#12018 closed Bug (fixed)

[Nested widgets] Widgets garbage collecting needs to be reviewed

Reported by: Piotrek Koszuliński Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.5.0 Beta
Component: UI : Widgets Version: 4.3 Beta
Keywords: Cc:

Description

Part of: #10931.

  1. Should repo.destroy( widget) destroy also its nested widgets?
  2. Should existing nested widgets be destroyed when setting nested editable's data? I think so, but then that will happen also on first widget initialization (on editor.setData()).
  3. I think that we'll need a new parameter for destroyAll() which will limit it to specific container. Or maybe better - we need a new method for that, because such parameter will change the method's algorithm completely. This method would be used in nested editable's setData().
  4. If nested widgets will be destroyed when destroying outer widget, then repository.destroyAll() may need to be rewrote, because it iterates over instances object which will be uncontrollably mutated and the order in which widgets are destroyed will be unpredictable. So, perhaps a better idea is to destroy only outer widgets in destroyAll(), so inner ones are reached in instances order.
  5. repo.checkWidgets bases on these methods so it has to be tested and modified accordingly.

Change History (5)

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

Status: newconfirmed

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

Owner: set to Piotrek Koszuliński
Status: confirmedreview

On pre-review. We'll need to review it again after we're done with Bender and I port tests.

comment:3 Changed 4 years ago by Olek Nowodziński

R+.

Please, answer your questions in the description because answers are not easy to find in the changeset. At least for outsiders.

Anyway, I was struck by the idea that editor.widgets.instances should somehow reflect the inheritance and relations between widgets, e.g. widgets that reside in a nested editable should be grouped as a subset of the parent widget. At the moment editor.widgets.instances is completely flat and it may be a problem for developers to create and manage complex structures this way. I'm not sure if editor.widgets.instances is the right place to do it (pretty sure it isn't), but developers should be able to access child widgets by referring to the property of the parent, i.e. to easily iterate child instances.

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

Milestone: CKEditor 4.4.2
Resolution: fixed
Status: reviewclosed

Closed on major with git:511070d. Comments are in git:89b013bfc. The answers for questions raised in the ticket can be found in them.

Anyway, I was struck by the idea that editor.widgets.instances should somehow reflect the inheritance and relations between widgets, e.g. widgets that reside in a nested editable should be grouped as a subset of the parent widget. At the moment editor.widgets.instances is completely flat and it may be a problem for developers to create and manage complex structures this way. I'm not sure if editor.widgets.instances is the right place to do it (pretty sure it isn't), but developers should be able to access child widgets by referring to the property of the parent, i.e. to easily iterate child instances.

Yep, I agree. A nice structure would be very useful, but it's too much effort now to rewrite all methods to use it, so we have to struggle with the flat instances object.

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

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