829399 - Add a limit to the maximum number of calls logged inside the call log database

Status: RESOLVED FIXED

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

Description

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

Maybe this is already the case but I'm pretty sure it is not but I would like to add a limit to the number of entries that can live in the call log. My fear is that the call log keeps growing up from month to month and that it ends up beeing unusable.

Comment 1

[David Scravaglieri [:scravag]]

Need UX input for this.

Comment 2

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys! Just wanted to tell you that currently there is a limit on the number of entries shown in the Call Log (extracted from the database) but not on the number of entries of this database. The presentation limit is 100 entries (an entry is a call or group of call, this is, a row in the Call Log) -> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/recents.js#L355 In fact, I just noticed that we should extract that 100 number to a "constant" ;-)

Comment 3

[Josh Carpenter [:jcarpenter]]

I spoke w/ Ayman and there are no strong opinions on this issue from UX, so long as the number of entries stored does not impede performance (scrolling, load time, etc) of the Call Log.

Comment 4

[Germán Toro del Valle (:gtorodelvalle)]

Obviously this is highly dependent on the concrete device as well as on many other aspects so I guess we should stick to some "magical" value :-p In case it helps, it seems in Android (4.1.1) it is 500 entries: http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/4.1.1_r1/android/provider/CallLog.java?av=f#373

500 it is??? :-)

Comment 5

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

(In reply to gtorodelvalle from comment #4)
> Obviously this is highly dependent on the concrete device as well as on many
> other aspects so I guess we should stick to some "magical" value :-p In case
> it helps, it seems in Android (4.1.1) it is 500 entries:
> http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/
> android/4.1.1_r1/android/provider/CallLog.java?av=f#373
> 
> 500 it is??? :-)

I have no strong opinion - I would say.... 250 out of my hat!

Comment 6

[Mihai Cirlanaru [:mihai][:mcirlanaru]]

It seems this bug depends on Bug 848027, which should bring an important amount of performance enhancements, including a limit param to the Recents DB query methods.

The fix for Bug 848027 will enable a more elegant approach to limiting the number of calls logged than simply hiding the ones over the predefined limit (set to 100 as detailed in comment#2) -- cf. https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/recents.js#L334

Comment 7

[Borja Salguero [:borjasalguero]]

Hi Mihai, we should wait to Ayman feedback, but with the new approach coming it's gonna be so easy to change this param and adjust it ;)

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Mihai Cirlanaru [:mcirlanaru] from comment #6)
> It seems this bug depends on Bug 848027, which should bring an important
> amount of performance enhancements, including a limit param to the Recents
> DB query methods.
> 

I don't think this bug depends on Bug 848027. If I understand correctly the title and description, this bug is referring to a limit of the actual number of records *stored* in the call log database, not a limit of the calls shown in the UI. We already added that limit as part of Bug 845274. Is that correct Vivien? If you was referring in your description to a limit in the UI, then this bug should probably be closed as a dup of Bug 845274.

A limit param for the query of the recents DB was also added in Bug 847404.

> The fix for Bug 848027 will enable a more elegant approach to limiting the
> number of calls logged than simply hiding the ones over the predefined limit
> (set to 100 as detailed in comment#2) -- cf.
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/
> js/recents.js#L334

We already did that in Bug 845274.

Comment 9

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

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #8)
> (In reply to Mihai Cirlanaru [:mcirlanaru] from comment #6)
> > It seems this bug depends on Bug 848027, which should bring an important
> > amount of performance enhancements, including a limit param to the Recents
> > DB query methods.
> > 
> 
> I don't think this bug depends on Bug 848027. If I understand correctly the
> title and description, this bug is referring to a limit of the actual number
> of records *stored* in the call log database, not a limit of the calls shown
> in the UI. We already added that limit as part of Bug 845274. Is that
> correct Vivien? If you was referring in your description to a limit in the
> UI, then this bug should probably be closed as a dup of Bug 845274.
>

It was both I guess. I won't be against closing this bug if there is now a limit on the UI and on the DB. (For what it worth I opened this bug during the night before the deadline for basecamp so things are a little bit blurry in my head about what I meant in the first place :)).

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #9)
> >
> 
> It was both I guess. I won't be against closing this bug if there is now a
> limit on the UI and on the DB. (For what it worth I opened this bug during
> the night before the deadline for basecamp so things are a little bit blurry
> in my head about what I meant in the first place :)).

Thanks Vivien! Actually, there is no limit in the number of records *stored* in the DB but in the number of records *retrieved* from the database in a query. Is that enough?

I am not sure about what fix is going to be implemented for bug 853350, but it seems like it should take care of low memory space situations in general, so a big call log database purge might be included with that fix, if that is what concerns to you.

Comment 11

[Germán Toro del Valle (:gtorodelvalle)]

As introduced in comment 4, in Android the oldest entries (beyond certain number) are deleted when new entries are stored. Maybe we could do something similar to this playing with IDBKeyRange and IDBObjectStore.count(). Just saying... ;-)

Comment 12

[Gregor Wagner [:gwagner]]

We should remove entries from the DB if we don't use them any more. Adding only doesn't scale.

Comment 13

[alberto.pastor]

I would add a DB purgue for not storing more than a certain number of logs. Who should make this decision?

Comment 14

[Daniel Coloma:dcoloma]

(In reply to Alberto Pastor [:albertopq] from comment #13)
> I would add a DB purgue for not storing more than a certain number of logs.
> Who should make this decision?

I think we need to fix a limit, I am not sure how many, but I would say something around 100-200 items. Does it make sense?

Anyhow, I would love Peter's input here.

Comment 15

[Wilfred Mathanaraj [:WDM]]

I think a max number of 200 makes sense.

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

Everybody seems to agree about the need of this db limit, so leo?

Comment 17

[Wayne Chang [:wchang]]

Triage agrees to leo+ this as a limitation seems to be the sensible implementation for the DB

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

This patch is based on top of the patch attached to bug 862385

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 761959 [details] [diff] [review]
v1

Review of attachment 761959 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/communications/dialer/js/call_log_db.js
@@ +7,5 @@
>    _dbGroupsStore: 'dialerGroups',
>    _dbVersion: 4,
>    _observers: {},
> +  _maxNumberOfGroups: 250,
> +  _numberOfGroupsToDelete: 50,

s/250/200

Comment 20

[Etienne Segonzac (:etienne)]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #18)
> Created attachment 761959 [details] [diff] [review]
> v1
> 
> This patch is based on top of the patch attached to bug 862385

Can we make a version that goes on top of master?
I'd like to prioritize this bug and land it first.

Thanks!

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Sure!

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Comment 23

[Etienne Segonzac (:etienne)]

Comment on attachment 762615 [details] [diff] [review]
v1

Review of attachment 762615 [details] [diff] [review]:
-----------------------------------------------------------------

How do you feel about passing a callback to _keepDbPrettyAndFit and then have something like:

```
self._keepDbPrettyAndFit(function() {
  callback(self._getGroupObject(group));
});

```

in CallLogDBManager.add ?

This way we can remove the ugly timeout from the tests (which will surely orange on travis).
And since we're not blocking anyway I see no harm in having the add callback triggered a bit later.

Also, we probably want to make sure that after a purge, if the user goes to edit mode and remove one of the already purged group we will handle it gracefully.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Thanks Etienne!

(In reply to Etienne Segonzac (:etienne) from comment #23)
> Comment on attachment 762615 [details] [diff] [review]
> v1
> 
> Review of attachment 762615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How do you feel about passing a callback to _keepDbPrettyAndFit and then
> have something like:
> 
> ```
> self._keepDbPrettyAndFit(function() {
>   callback(self._getGroupObject(group));
> });
> 
> ```
> 
> in CallLogDBManager.add ?
> 
> This way we can remove the ugly timeout from the tests (which will surely
> orange on travis).
> And since we're not blocking anyway I see no harm in having the add callback
> triggered a bit later.
> 

I usually don't like to modify the code because of the tests, but in this case I agree that it doesn't really make much of a difference.

> Also, we probably want to make sure that after a purge, if the user goes to
> edit mode and remove one of the already purged group we will handle it
> gracefully.

We already do it properly :). If an already deleted group is deleted via edit mode, we just remove it from the UI.

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Comment 26

[Etienne Segonzac (:etienne)]

Comment on attachment 763457 [details] [diff] [review]
v2

Review of attachment 763457 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, thanks for the followup!

Sadly, I just realized something:
* there is an outdated (old params) |deleteGroup| call in the |deleteGroupList| method 
* we didn't catch this because the |deleteGroupList| method is not tested :/

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

You are right!

Comment 28

[Etienne Segonzac (:etienne)]

Comment on attachment 763540 [details] [diff] [review]
v3

Review of attachment 763540 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome!

r=me with the test-related comment addressed.

::: apps/communications/dialer/test/unit/calllog_db_test.js
@@ +730,5 @@
> +
> +    test('Add another call', function(done) {
> +      CallLogDBManager.add(call2, function(group) {
> +        groupList.push(group);
> +        CallLogDBManager.deleteGroupList(groupList, function(result) {

wouldn't mind a test asserting that getGroupList gives us an empty array too.

(like we do for the keepDBFit tests)

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

Thanks Etienne!

https://github.com/mozilla-b2g/gaia/commit/c74d0ee0692de0171af7b3a3f28b640a3f4fbec1

Comment 30

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 c74d0ee0692de0171af7b3a3f28b640a3f4fbec1
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

v1-train: https://github.com/mozilla-b2g/gaia/commit/f638e04951c3d560d75f58dfabb28e8db3ccc7e7

Comment 32

[John Ford [:jhford] CET/CEST Berlin Time]

Marking branch resolution per comment 31

Comment 33

[John Ford [:jhford] CET/CEST Berlin Time]

1.1hd: f638e04951c3d560d75f58dfabb28e8db3ccc7e7