936638 - [Settings] Make ListView automatically observe observable items in its array

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Corey Frang [:gnarf]]

There is currently a bad pattern starting to form where templates are binding to an Observable inside of the ListView.  This will create problems if any of the previously bound Observables change data in the case of a "recycled" element. 

Example of a bad scenario: https://gist.github.com/gnarf/6a2d331253e37495bf74 
Example in actual code: https://github.com/mozilla-b2g/gaia/blob/b27c0249c5877f6a55a8de37f0eef8770b515711/apps/settings/js/keyboard.js#L305-L310 

We can avoid this problem by having ListView automatically Observe any Observable in it's array, and call the template function whenever it changes.  This will require making Observable have an .observe("*") as well.

Comment 1

[Arthur Chen [:arthurcc][inactive after 6/30]]

The proposed way seems to refresh the entire associated DOM element when a property in an Observable changes.

How to make use of the information in an Observable is determined by the template instead of the ListView. And it is not suggested to observe every properties of an Observable as it may contain information more than a ListView actually needs.

We can make the template function return an object that has functions allowing us to get the associated DOM element and to refresh the DOM element with a new Observable. In the function of refreshing, we are able to un-bind functions from the Observable.

Comment 2

[Corey Frang [:gnarf]]

It then becomes a bad trap, on the responsibility of the template method to make sure to unbind (or throw away the old dom elements).  The template must also now be able to know what the old observable was to unbind (something that is much easier for ListView to do).  If we put this into the templates, it also again increases the reference count to everything in the whole cycle (element -> currently bound element) making everything another potential big memory leak.

I would highly suggest we make ListView do the "smart" thing and handle the Observing of Observables, this lets every template method just be dumb, and automatically get the bonus!

re "return an object that has functions":  Returning a bucket of methods from the template also increases the chance of leaking cyclical references to elements and observers.

I could also implement this in a way that it would only bind to properties on the observable that are accessed from inside the template function.  Yes, this would still call the entire template function for any watch property change, but I think stuffing data/text into the existing DOM is going to be very cheap, it shouldn't be a noticeable perf hit to do them all at once.

I think this gives the cleanest api for us to use.

Comment 3

[Arthur Chen [:arthurcc][inactive after 6/30]]

This somehow limits the usage and flexibility of template functions. A template function is not just for turning data into UI, it also deals with basic interactions with UI.

In template functions we do things like listening to changes of UI and updating the bound observable object accordingly. If we call the template function every time when a property changes, the listeners are added multiple times. Just like addEventListeners, we must always keep removing listeners in mind.

I would be better for ListView taking care of un-bind observables, but I think there are better ways not sacrificing what we can do with template functions. 

A WIP to illustrate my idea: https://github.com/crh0716/gaia/commit/c64226b176ded2cebdcd2b43291dfaf7607349e0

Comment 4

[Corey Frang [:gnarf]]

(In reply to Arthur Chen [:arthurcc] from comment #3)
> In template functions we do things like listening to changes of UI and
> updating the bound observable object accordingly. If we call the template
> function every time when a property changes, the listeners are added
> multiple times. Just like addEventListeners, we must always keep removing
> listeners in mind.

This is also something that shouldn't be done via the template function.  This should be an event listener bound on the <ul> itself that catches bubbled events and checks which element was interacted with based on delegation.  We shouldn't be binding events in the templates either.

> I would be better for ListView taking care of un-bind observables, but I
> think there are better ways not sacrificing what we can do with template
> functions. 

I think what you're interpreting as "sacrifice" is actually just avoiding these traps where the List and the Observable get so bound up with each other you can never get rid of the memory they use.  A template method should not do anything more than create and populate DOM.

Comment 5

[Arthur Chen [:arthurcc][inactive after 6/30]]

A template function can be more than creating and populating DOM. We just need to figure out a way to describe bindings used in a template function, then we can use the information to do un-binding when the content is replaced with other Obversables.

Creating bindings with declarative ways might be a solution. We can have something like "ListViewTemplate(templateFunc, bindings)" to create a template object and it handles all of the binding things.

Comment 6

[Corey Frang [:gnarf]]

I think the solution provided by the helper.observeAndCall being added in bug 933167 will be enough to handle this.

Comment 7

[Corey Frang [:gnarf]]

See bug 933167 - Added the solution in there with observeAndCall