DisplayContainer: Do no remove child on hide

[EAlexRojas]

On the show method is understandable that if the element is not a child of the widget, it has to add it.

But on the hide method, if the element is actually a child of the widget, it's really needed to remove it from the DOM? Maybe the child has to be removed only if it has been added on the show method?

or maybe hide method doesn't have to remove the element ?

[wkeese]

ISTM that currently, show() and hide() just typically show and hide the element, rather than adding/removing it from the DOM. Removing and then re-adding elements every time they are hidden/shown seems unnecessarily expensive because it will trigger calls to removedCallback()/attachedCallback() for all the widgets contained in the elements.

[EAlexRojas]

Actually, on hide() method, the element is removed every time it is hidden (https://github.com/ibm-js/delite/blob/master/DisplayContainer.js#L178)

// if view is not already removed, remove it
    if (self.getIndexOfChild(value.child) !== -1) {
        self.removeChild(value.child);
    }

And on show() method, the element is added if it is not a child of the widget (https://github.com/ibm-js/delite/blob/master/DisplayContainer.js#L106)

// if view is not already a child this means we loaded a new view (div), add it
    if (self.getIndexOfChild(value.child) === -1) {
        self.addChild(value.child, value.index);
    }

[wkeese]

Ah, I misunderstood. I agree w/what you originally wrote/implied, that it's weird to remove the element every time.

[cjolif]

Before I start implementing something we need to decide what. I guess there are two options:

1. We remove the element only if was added by us (so we need to keep track of that information)
2. We remove the element only if a particular param is passed to hide like { remove: true }.

What do people think?

[wkeese]

I think you should pick option 3: never remove the element.

Why? Because the method is called "hide", not "remove", and also because I don't know of any cases where applications actually want to remove the element. Is there something I missed?

[cjolif]

@wkeese the reason remove is here is that users have asked for it (cc: @edchat) in a application framework you might want to unload views that where dynamically added. So it seems 2. is the solution. This allows the ones that want to remove it (the use-case I just mentioned) to get it removed and for your own use-case it will never be removed as I delite/deliteful elements (outside of dapp) will never put that parameter on.

[edchat]

@cjolif we have had users ask for the ability to unload views in dapp, and we have implemented it there, but dapp did not expect it to happen automatically on hide (that would be a bad idea). It could happen conditionally on hide, but support to unload a view separately from hide may be more useful. dapp has support for the app to request a view to be unloaded currently, and had plans to be able to setup a maxChildren type of flag which if set would mean we would remove the oldest child view when the max is exceeded. So from dapp's perspective, I don't think the flag to remove the view is required, we could use it in the one case where the view which is currently showing is the one to be removed, but we would still need to support the ability to remove other views.

[cjolif]

Ok, then if even dapp does not need this I'll remove it entirely.