Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIMOB-23390] Android: Implement contacts offset and max #8112

Closed
wants to merge 3 commits into from

Conversation

ashcoding
Copy link
Contributor

JIRA: https://jira.appcelerator.org/browse/TIMOB-23390

Description:

This is only for Android.

This to implement Pagination in Ti.Contacts.getAllPeople(). An example
for usage is as follows:

var people = Ti.Contacts.getAllPeople({max:5,offset:10});

This will set the max number of persons returned to be 5 and sets the
offset to be from index 10 onwards. Eg, Persons number 10, 11, 12, 13
and 14.

@ashcoding ashcoding changed the title [TIMOB-23390] Android: Implement contacts paginate [TIMOB-23390] Android: Implement contacts offset and max Jul 7, 2016
@ashcoding
Copy link
Contributor Author

Test code:-

Ti.UI.backgroundColor = 'white'; 
var win = Ti.UI.createWindow({ 
    exitOnClose : true, 
    layout : 'vertical' 
}); 

var button = Titanium.UI.createButton({
   title: 'Get Contacts Here',
   top: 60
});

win.add(button);

win.open(); 

var performAddressBookFunction = function(){
    var singleValue = [
      'recordId', 'firstName', 'middleName', 'lastName', 'fullName', 'prefix', 'suffix', 
      'nickname', 'firstPhonetic', 'middlePhonetic', 'lastPhonetic', 'organization', 
      'jobTitle', 'department', 'note', 'birthday', 'created', 'modified', 'kind'
    ];
    var multiValue = [
      'email', 'address', 'phone', 'instantMessage', 'relatedNames', 'date', 'url'
    ];
    Ti.API.info('getSizeOfAllPeople Total contacts: ' + Ti.Contacts.getSizeOfAllPeople());
    var people = Ti.Contacts.getAllPeople({max:30,offset:20});
    Ti.API.info('Total contacts: ' + people.length);


    for (var i=0, ilen=people.length; i<ilen; i++){
      Ti.API.info('---------------------'+i);

      var person = people[i];
      if(person != null) {
          var firstname = person.firstName || "Null";
          var lastname = person.lastName || "Null"
          Ti.API.info(firstname+"-"+lastname);
      } else {
        Ti.API.info("Person is null at"+i);
      }
      /*
      for (var j=0, jlen=singleValue.length; j<jlen; j++){
        Ti.API.info(singleValue[j] + ': ' + person[singleValue[j]]);
      }
      for (var j=0, jlen=multiValue.length; j<jlen; j++){
        Ti.API.info(multiValue[j] + ': ' + JSON.stringify(person[multiValue[j]]));
      }
      */
    }


};
var addressBookDisallowed = function(){
alert("Not Allowed");
};

button.addEventListener('click',function(e)
{
   if (Ti.Contacts.hasContactsPermissions()) {
    performAddressBookFunction();
} else {
    Ti.Contacts.requestContactsPermissions(function(e) {
        if (e.success) {
            performAddressBookFunction();
        } else {
            addressBookDisallowed();
        }
    });
}

});

@ashcoding
Copy link
Contributor Author

ashcoding commented Jul 7, 2016

Do not merge yet
Needs review and improvement

@mrkiley
Copy link
Contributor

mrkiley commented Jul 12, 2016

@ashcoding We're trying to merge this into a new branch we are maintaining off of 5_3_X. Do you have a timeline for resolving these merge conflicts? It'd help greatly. Thanks!

@ashcoding
Copy link
Contributor Author

@mrkiley can you direct me to your repo/branch? This PR is being merged into master and it might be very different from your 5_3_X.

In any case, I'll get this merged conflict resolved.

This is only for Android.

This to implement Pagination in Ti.Contacts.getAllPeople(). An example
for usage is as follows:

```javascript
var people = Ti.Contacts.getAllPeople({max:5,offset:10});
```

This will set the max number of persons returned to be 5 and sets the
offset to be from index 10 onwards. Eg, Persons number 10, 11, 12, 13
and 14.
Implemented a method called ```Ti.Contacts.getSizeOfAllPeople()``` that
will get the total size of all the contacts for Android. This is to be
used with the properties ```max`` and ```offset``` in
```Ti.Contacts.getAllPeople()```.
@ashcoding
Copy link
Contributor Author

@sgtcoolguy Could you take a look and review this? Thank you.

@mrkiley
Copy link
Contributor

mrkiley commented Jul 12, 2016

Thanks for getting back to me so quickly @ashcoding, and thanks for resolving the merge conflicts.

We are still experiencing some issues. We set our max to 500, with an offset of 0 (to start). We have a total of 2708 contacts to loop through. After retrieving the first 500, the offset gets set to 500. Upon the second retrieval attempt, the JNI error pops up again:

07-12 00:25:29.465: E/dalvikvm(5977): JNI ERROR (app bug): local reference table overflow (max=512)
07-12 00:25:29.485: E/dalvikvm(5977): Failed adding to JNI local ref table (has 512 entries)
07-12 00:25:29.485: E/dalvikvm(5977): VM aborting
07-12 00:25:29.485: A/libc(5977): Fatal signal 6 (SIGABRT) at 0x00001759 (code=-6), thread 5991 (KrollRuntimeThr)

I only adjusted you sample code above a tiny bit in order to accommodate looping over multiple iterations to retrieve all of the contacts in the user's address book. See below:

`
var performAddressBookFunction = function(){

var singleValue = [
  'recordId', 'firstName', 'middleName', 'lastName', 'fullName', 'prefix', 'suffix', 
  'nickname', 'firstPhonetic', 'middlePhonetic', 'lastPhonetic', 'organization', 
  'jobTitle', 'department', 'note', 'birthday', 'created', 'modified', 'kind'
];
var multiValue = [
  'email', 'address', 'phone', 'instantMessage', 'relatedNames', 'date', 'url'
];
Ti.API.info('getSizeOfAllPeople Total contacts: ' + Ti.Contacts.getSizeOfAllPeople());

var totalPeople = [];
var sizeOfAllPeople = Ti.Contacts.getSizeOfAllPeople();

while (totalPeople.length < sizeOfAllPeople) {
    Application.Log('Going in for another iteration: offset is ' + totalPeople.length);
    var people = Ti.Contacts.getAllPeople({max:500,offset: totalPeople.length});

    Ti.API.info('Total contacts this iteration: ' + people.length);

    totalPeople = people.concat(totalPeople);

    for (var i=0, ilen=people.length; i<ilen; i++){
        Ti.API.info('---------------------'+i);

        var person = people[i];
        if (person != null) {

            var firstname = person.firstName || "Null";
            var lastname = person.lastName || "Null"
            Ti.API.info(firstname+"-"+lastname);
        }

        else {
            Ti.API.info("Person is null at"+i);
        }
    }
}

};`

I also attempted to call the same getAllPeople() method twice, by placing it on the line immediately after the first call, and it seems to have caused the JNI error.

Thoughts on what's going on here, and a potential fix? Is the workaround still hitting the underlying issue?

@ashcoding
Copy link
Contributor Author

It looks like it might still be the same issue.
Failed adding to JNI local ref table (has 512 entries)
Are you still having issues with this or are you able to work around with this for now? @mrkiley

@mrkiley
Copy link
Contributor

mrkiley commented Jul 13, 2016

@ashcoding, we are still having issues with it, and none of our lower-level workarounds have been successful. The developer (@nbannister85) who has been taking point on it will add in some details around that below.

@ghost
Copy link

ghost commented Jul 13, 2016

@ashcoding

I think the issue goes beyond contacts in general, but could represents a deeper problem.
I found this from a while ago:
http://www.tidev.io/2014/07/08/dont-let-callbacks-cross-the-bridge/

I could be wrong but in getting contacts, appcelerator creates an array of PersonProxy's....
In their creation, are notification being set across the jni barrier and memory is being leaked?

@ashcoding
Copy link
Contributor Author

@nbannister85 thanks for the info. You could be on to something.

Will need to investigate this further.

cc: @sgtcoolguy fyi.

@mrkiley
Copy link
Contributor

mrkiley commented Jul 18, 2016

Any updates on this @ashcoding?

@sgtcoolguy
Copy link
Contributor

@ashcoding @mrkiley I think this PR doesn't address the root cause. I believe this should fix the issue: #8147

@sgtcoolguy
Copy link
Contributor

Not to say we wouldn't want to improve the API to allow more control over the number of records returned...

@ashcoding
Copy link
Contributor Author

Closing this PR as it is unnecessary. #8147 PR aims to solve the root of the issue.

@ashcoding ashcoding closed this Jul 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants