847409 - Add infinite scroll to the call log

Status: RESOLVED DUPLICATE of bug 1070021

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

Comment 1

[alberto.pastor]

Attachment: Pointer to PR 9847

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Why don't you use visibility_monitor.js? That will let you pre-create the rows and keep the scrollbar. Also it will avoid duplicating the code on a per app basis for the same issues.

I would like to understand the rationale before merging anything :)

Comment 3

[Etienne Segonzac (:etienne)]

Comment on attachment 751027 [details]
Pointer to PR 9847

The code looks good.

I agree with Vivien that using visibility monitor would be better though.

Since it would enable us to bring back the scroll bar and limit the memory consumption (it can get pretty huge).

So we should sort this out before landing anything.

Comment 4

[alberto.pastor]

Th 3 main reasons for not using visibility_monitor yet were:

- Visibility monitor doesn't support nested child. We'll only receive events when a header is shown/hidden (Bug 865750).

- Current implementation of edit mode, needs all the elements in the DOM in order to perform a "Delete all"

- Partners want to see the call log speed up as soon as possible, in order to take some time measurements.

That said, I totally agree we need to add visibility_monitor to our lists.
What about creating another bug for improving memory performance, and land this one at it is right now?

Thanks!

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alberto Pastor [:albertopq] from comment #4)
> Th 3 main reasons for not using visibility_monitor yet were:
> 
> - Visibility monitor doesn't support nested child. We'll only receive events
> when a header is shown/hidden (Bug 865750).

David has started on it already or will start soon. I won't mind if this bug depends on bug 865750.

> 
> - Current implementation of edit mode, needs all the elements in the DOM in
> order to perform a "Delete all"
> 

That's ok. The code for visibility_monitor.js will make you keep the container for each row. So you will still have all a container with an id. That should be enough right?

> - Partners want to see the call log speed up as soon as possible, in order
> to take some time measurements.
>

That's not a very technical argument :) Also since this bug is not leo+ while the underlying bug is I would assume that the partner would appreciate if priorities are taken into account ;P 
 
> That said, I totally agree we need to add visibility_monitor to our lists.
> What about creating another bug for improving memory performance, and land
> this one at it is right now?

I fear that landing this one right now will make it fast for the call log but will de-prioritize the lower level bug that will try to provide some code to help all apps that use groups for list (like the call log but also the sms app, settings, music, etc..).

Also Gecko history has proven that once something seems to work it stays for a long time (you wil lfind tons of old comments in Gecko that says: temporary hack!)
> 
> Thanks!

Sorry I don't want to slow you down, just want to make it the best way I can think of for the product.

Comment 6

[alberto.pastor]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #5)
> (In reply to Alberto Pastor [:albertopq] from comment #4)
> > Th 3 main reasons for not using visibility_monitor yet were:
> > 
> > - Visibility monitor doesn't support nested child. We'll only receive events
> > when a header is shown/hidden (Bug 865750).
> 
> David has started on it already or will start soon. I won't mind if this bug
> depends on bug 865750.
> 
> > 
> > - Current implementation of edit mode, needs all the elements in the DOM in
> > order to perform a "Delete all"
> > 
> 
> That's ok. The code for visibility_monitor.js will make you keep the
> container for each row. So you will still have all a container with an id.
> That should be enough right?

Not really. It depends on the checkbox inside the container. Anyway, we could add more logic for handling that.

> 
> > - Partners want to see the call log speed up as soon as possible, in order
> > to take some time measurements.
> >
> 
> That's not a very technical argument :) Also since this bug is not leo+
> while the underlying bug is I would assume that the partner would appreciate
> if priorities are taken into account ;P 

I didn't say they were all technical reasons :P . At the other hand, I think will be leo+ soon... 
>  
> > That said, I totally agree we need to add visibility_monitor to our lists.
> > What about creating another bug for improving memory performance, and land
> > this one at it is right now?
> 
> I fear that landing this one right now will make it fast for the call log
> but will de-prioritize the lower level bug that will try to provide some
> code to help all apps that use groups for list (like the call log but also
> the sms app, settings, music, etc..).
> 
> Also Gecko history has proven that once something seems to work it stays for
> a long time (you wil lfind tons of old comments in Gecko that says:
> temporary hack!)
> > 
> > Thanks!
> 
> Sorry I don't want to slow you down, just want to make it the best way I can
> think of for the product.

I would say, depending on how long is going to take Bug 865750 to be fixed, we can land this and change it later, or wait. If David already started to work (it was just today!), I agree it makes no sense landing this to change it later :)

Any time estimation for the visibility_monitor bug?

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alberto Pastor [:albertopq] from comment #6)


> I would say, depending on how long is going to take Bug 865750 to be fixed,
> we can land this and change it later, or wait. If David already started to
> work (it was just today!), I agree it makes no sense landing this to change
> it later :)
> 
> Any time estimation for the visibility_monitor bug?

I can't answer for David. Also it would be nice if you can coordinate with him in order to ensure what he is doing fit with your use case!

Let needinfo him to make sure he is aware of the work here.

Comment 8

[David Flanagan [:djf]]

I have started work on a generalized visibility monitor that will allow monitoring of grandchildren.  It turns out to be non-trivial, and I don't have anything checked in.  I'm heading to San Diego today and will be working on Leo media-related bugs Tuesday and Wednesday. But I hope to work on this one on the plane at least.  (Its by far the most interesting bug I've got :-)

Having a concrete use case like this is helpful. Thanks for letting me know about it.  I'll let you know as soon as I have something ready to be tried out.  And if I can't finish something on the plane today, I'll at least try to land a WIP patch so that someone can take over the bug if necessary.

Comment 9

[David Flanagan [:djf]]

Alberto,

The visibility monitor extension I'm working on won't be a fully-general one. It will add the ability to monitor the grandchildren of the scrolling element instead of monitoring the children.  I'll probably also add the ability to limit the elements monitored by the tagname of their parent, and possibly by the tagname of the elements themselves.  So you could monitor the grandchildren of the list that are children of a div, for example.  Or maybe monitor all li elements that are children of a div that are children of the list.  Will this work for your use case here?

Comment 10

[alberto.pastor]

Hi David,
Thanks for taking this. I would say that your description would work for us, but just to make sure, anything that allow us to know when each of the elements (the "li" tags) in the lists defined by the Building Blocks (http://buildingfirefoxos.com/building-blocks/lists/) are in the viewport, will work in for most of apps.

Thanks!

Comment 11

[Francisco Jordano [:arcturus] [:francisco]]

Vivien, my understanding here after reading the comments is we will wait till bug 865750 is fixed, isnt?

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Francisco Jordano [:arcturus] from comment #11)
> Vivien, my understanding here after reading the comments is we will wait
> till bug 865750 is fixed, isnt?

I would like too!

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Etienne, Vivien, is dialer still planning to implement this now that the tag_visibility_monitor landed in bug 865750?

I think in contacts we may have come to the conclusion that a streaming cursor approach may be more scalable.  The issue with visibility_monitor is it wants most of the DOM in place in order to detect when elements are coming on screen.  It seems keeping a minimal DOM is a bigger win even if we have to keep adding/removing.

Comment 14

[Etienne Segonzac (:etienne)]

Well if we investigated both approaches and found a winner I trust the perf team on that one :)

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Etienne Segonzac (:etienne) from comment #14)
> Well if we investigated both approaches and found a winner I trust the perf
> team on that one :)

I don't know that its completely settled.  The contacts API has held us back from really implementing/proving out that solution.  We will probably need to migrate to Datastore before we can conclusively say which is better.

I guess we were just wondering if you had plans around this for dialer or not.  Thanks!

Comment 16

[Kevin Grandon :kgrandon]

We are also looking at solutions for this in bug 940428.

Comment 17

[Dave Huseby [:huseby]]

This sounds like a feature request, not a perf issue.  Once we have a good solution for infinite scrolling, if there are perf issues with that, then file a new bug and we'll look at it.

Comment 18

[Francisco Jordano [:arcturus] [:francisco]]

Anthony, I inherit this when Alberto left, I'm not putting enough love here but I'm sure you will.

Cheers,
F.

Comment 19

[Anthony Ricaud (:rik)]

http://robert.ocallahan.org/2014/02/implementing-virtual-widgets-on-web.html is a good start when we'll implement this.

Wilfred: can we put this in our backlog?

Comment 20

[Anthony Ricaud (:rik)]

Let me also add that implementing this will provide better reactivity when opening the call log and use less memory.

Comment 21

[Wilfred Mathanaraj [:WDM]]

backlog for now for v1.4. revisit when we have done v1.4