1059715 - VCard reader refactor to read contacts before importing them

Status: RESOLVED FIXED

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

Description

[David Garcia [:davidg][retired]]

Currently there is a VCFReader class (in shared/js/contacts/import/utilities/vcard_parser.js) that allows you to read and import contacts from a vcard file, in one atomic operation.
In order to implement bug 849729, we need to be able to read the vcard file to show the user a preview of the contacts in that file before importing them.
To avoid code duplication, I think this class should be refactored so you can read and obtain the contacts first, and import them later, so we can reuse this component for that case too.

Comment 1

[David Garcia [:davidg][retired]]

My approach will be to make VCFReader to just read the contacts from the vcard file. So you will call "process" method, and then in the callback you will get the parsed contacts, just as it is currently doing, but it will not import the contacts.
Then I will use ContactsImporter (shared/js/contacts/import/contacts_importer.js) to import the parsed contacts

Sounds reasonable?

Comment 2

[David Garcia [:davidg][retired]]

I just looked around, and seems that there are other places where VCFReader is used. So in order to be as transparent as possible I think what I will end up doing is having an importOnRead property on VCFReader (true by default) that I can set to false if I don't want the contacts to be imported by default.
Something like:

vcreader = new VCFReader(text); 
vcreader.importOnRead = false;
vcreader.process(function(contacts) { ...

Comment 3

[David Garcia [:davidg][retired]]

Attachment: patch

Comment 4

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

Comment on attachment 8482207 [details] [review]
patch

Hi David,

nice suggestion to split this in a module that reads the vcard file.

However, I would like to do this split right.

I mean, with the current patch we just have a logic for executing an action, but for example, even if we are not going to import we will need to load all the depencencies.

I would like to see a full split into 'pure reading' component and then the exporter (that uses this component + the saving + the matching).

Something that perhaps receives the vcard content and returns a promise that will receive the array of mozcontacts objects. Then later this can be used by anyone and adapted to the specific needs.

Comment 5

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

Sergi as you were the original creator of this, what do you think about my last comment, do you have feedback?

Comment 6

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

I agree with Francisco about modularizing components.

I am also concerned about memory, mainly because people will use the import feature to import a significant amount of contacts most of the time, and storing this in memory could be pretty bad for limited devices.

David, how do you plan to show the contacts to the user? Do you plan to load them all at the same time, or perhaps do some dynamic loading eg. as the user scrolls?

Comment 7

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

Woaaah!

I totally miss the point of data streaming, we definitely need to do it, we already faced some problems in the past with memory an 2000 contacts vcards.

Perhaps we could pass a callback to the function and send chunks of data to that callback until we send [], or something similar.

Comment 8

[David Garcia [:davidg][retired]]

Yes, I agree with making the VCFReader just read the contacts. I saw that it was used in another place, that's why I suggested this "transparent" solution, but I think it worths changing it. 

Currently the UX spec doesn't say anything about pagination, but I guess some kind of dynamic loading will be needed when there is too much contacts. I was planing to reuse the current contact list view, I don't know if it already supports some kind of dynamic loading.

In the reader class I can make an iterator for this [1]. Something like:

var reader = VCFReader(file);
var iterator = reader.getIterator();
var next = iterator.next();
.....

That way you can easily control how many you read from the file. Also if you plan to import them, you can read one, import it, discard it and read the next. So you only need to hold one mozContact object at a time in memory. 

[1] https://developer.mozilla.org/en/docs/Web/JavaScript/Guide/The_Iterator_protocol

Comment 9

[David Garcia [:davidg][retired]]

Attachment: new vcard reader class

Hi! I created this VCardReader class to just read contacts from VCard file.
I tried to copy over most of the code from Sergi, because that is probably well tested on working currently, but I removed all the import contacts related stuff, and the API of the component changes a bit.
You use it like this:

var reader = new VCardReader(vcardtext); // Pretty much same as before
reader.onContactParsed = function(contact) { // Called everytime a contact is red
   console.log(contact); // contact red from vcard. It is a mozContact object
   reader.next(); // Keep reading contacts from vcar reader
};
reader.next(); // Start getting contacts from reader

This way the client can control how many contacts the read from the file.
I'm also thinking in implementing reader.readAll(), so you don't have to keep calling reader.next((.

I still need to change the places where this is used currently, implement the importing part and add tests, but I wanted to ask you for feedback before continuing with the refactor.

What do you think?

Comment 10

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

Comment on attachment 8485694 [details] [review]
new vcard reader class

The interface looks good to me.

I would perhaps instead of exposing in the api a 'next' method will expose something like 'getCursor' or 'start' and that returns your 'cursor' following the iterable pattern.

Thanks! Pretty good job!

Comment 11

[David Garcia [:davidg][retired]]

Attachment: patch v2 with tests

I tried the suggestion from Francisco, but because both, initializing the reader and reading the next contact are async I think this api is simpler.

I was trying to replace the current VCFReader with the new VCardReader + import_contact, for bluetooth contact sharing, and SD card import. But I think this patch is currently quite big, so I think that parts can be refactored in follow up bugs.

Also if we land this, work can be started on bug 849729.

Comment 12

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

Comment on attachment 8494519 [details] [review]
patch v2 with tests

As Sergi was the creator of this, we will need his input here too.

Comment 13

[Jose Manuel Cantera]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #12)
> Comment on attachment 8494519 [details] [review]
> patch v2 with tests
> 
> As Sergi was the creator of this, we will need his input here too.

Yes, but if he cannot give us an ETA for this review it would be better to skip that. We need this bug landed asap as it is blocking other bugs TEF is requesting.

This is only a pure refactoring of code so I believe you can trust in my review

Comment 14

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

(In reply to Jose Manuel Cantera from comment #13)
> (In reply to Francisco Jordano [:arcturus] [:francisco] from comment #12)
> > Comment on attachment 8494519 [details] [review]
> > patch v2 with tests
> > 
> > As Sergi was the creator of this, we will need his input here too.
> 
> Yes, but if he cannot give us an ETA for this review it would be better to
> skip that. We need this bug landed asap as it is blocking other bugs TEF is
> requesting.

Last thing we need to do is skip reviews. If is that important let ask Sergi to give priority to this. Sure he can accomodate that.

> 
> This is only a pure refactoring of code so I believe you can trust in my
> review

I totally trust any code review (not just reviews) coming from you, this is just again having the creator of the code having a look, specially since he already was giving some feedback about this refactor.

Comment 15

[Jose Manuel Cantera]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #7)
> Woaaah!
> 
> I totally miss the point of data streaming, we definitely need to do it, we
> already faced some problems in the past with memory an 2000 contacts vcards.

We have tested FB importation with > 1500 contacts and we didn't face any memory issue 

> 
> Perhaps we could pass a callback to the function and send chunks of data to
> that callback until we send [], or something similar.

Comment 16

[Jose Manuel Cantera]

(In reply to David Garcia [:davidg] from comment #9)
> Created attachment 8485694 [details] [review]
> new vcard reader class
> 
> Hi! I created this VCardReader class to just read contacts from VCard file.
> I tried to copy over most of the code from Sergi, because that is probably
> well tested on working currently, but I removed all the import contacts
> related stuff, and the API of the component changes a bit.
> You use it like this:
> 
> var reader = new VCardReader(vcardtext); // Pretty much same as before
> reader.onContactParsed = function(contact) { // Called everytime a contact
> is red
>    console.log(contact); // contact red from vcard. It is a mozContact object
>    reader.next(); // Keep reading contacts from vcar reader
> };
> reader.next(); // Start getting contacts from reader

I would prefer something more aligned with other APIs, for instance the mozContacts API. What about 

var reader = new VCardReader(vcardtext);
var cursor = reader.getAll();
cursor.onsuccess = function(evt) {
  var contact = evt.target.result;
  if(contact) {
    cursor.continue();
  }
}

> 
> This way the client can control how many contacts the read from the file.
> I'm also thinking in implementing reader.readAll(), so you don't have to
> keep calling reader.next((.
> 
> I still need to change the places where this is used currently, implement
> the importing part and add tests, but I wanted to ask you for feedback
> before continuing with the refactor.
> 
> What do you think?

Comment 17

[Jose Manuel Cantera]

(In reply to Jose Manuel Cantera from comment #15)
> (In reply to Francisco Jordano [:arcturus] [:francisco] from comment #7)
> > Woaaah!
> > 
> > I totally miss the point of data streaming, we definitely need to do it, we
> > already faced some problems in the past with memory an 2000 contacts vcards.
> 
> We have tested FB importation with > 1500 contacts and we didn't face any
> memory issue 

Nonetheless, it is better to have them progressively parsed as we will improve the responsiveness of the importation screen, as we will be able to show it as soon as possible, thus I agree on making the parsing progressive and fully async

> 
> > 
> > Perhaps we could pass a callback to the function and send chunks of data to
> > that callback until we send [], or something similar.

Comment 18

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi David,

Would it be possible to see how the final code in the tree will look in the end? Since your new class is replacing the old vcard reader, it would be nice to see what bits have changed. It is hard to visualize what actually changed now, since you created a new file and github shows all the code as new.

Comment 19

[David Garcia [:davidg][retired]]

Hi Sergi,

I tried to prepare a version that is easier to diff than the other one.
It is here: https://github.com/mozilla-b2g/gaia/pull/24704/files?diff=split
Bear in mind that I changed identations and reordered de code, so it easier to diff against the original version, but I'm not sure this versions works, but at least you can see the changes.

Hope it helps.

Comment 20

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Hi David, 

Thanks, that helps a bit to visualize. But what I am most interested is in having a working PR of the final version of the code, the code that should go directly to master in case it passes the review, and with every modified file in it. Is that possible?

Thanks.

Comment 21

[David Garcia [:davidg][retired]]

Hi Sergi,

Currently VCFReader is used in multiple places, so it is a big refactor affecting different apps. The current patch is quite big already, so my suggestion was to land this class first, so people working on bugs that need to read vcard without importing, and deal with individual refactors on follow-ups.
Does it make sense or do you think it is better to do the whole thing at once?

Comment 22

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

It makes sense, but if it's not much trouble I'd prefer to land it all at once. The reasons for that are:

- It is a big refactor, but it should be easy to test using and extending the existing vcard tests.
- APIs become more clear when seen in context by their consumers

In case you think it is too risky/hard, that's fine. But we would need to create a new bugzilla issue for the task of integrating individual refactors later.

What do you think?

Comment 23

[David Garcia [:davidg][retired]]

Ok, lets do it all at once.
I created a new file vcard_reader.js with VCardReader class. Current is vcard_parser.js with VCFReader class.
As we are not going to maintain the two versions anymore, do you want me to keep the current file/class name?

Comment 24

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

As long as the naming remains equally clear or more, I don't really mind :)

Comment 25

[Jose Manuel Cantera]

Comment on attachment 8494519 [details] [review]
patch v2 with tests

For some reason GH does not properly track the changes even doing a PR over your PR David, so please see the alternative proposal at:

https://github.com/mozilla-b2g/gaia/pull/24821/files

* Basically I have made the following changes:

- remove onfinished callback as it is not needed
- change the implementation of the cursor, to a simpler and more convenient one
- change the tests accordingly 
- there was a bug in the code here https://github.com/empoalp/gaia/blob/bug_1059715_3/shared/js/contacts/import/utilities/vcard_reader.js#L255 
as you will need to call return in order not to continue processing 
- reorder the code in order to show first the published interface
- add comments about the supported API

pleae take this code as a basis for asking review to Sergi

thanks

Comment 26

[Julien Wajsberg [:julienw]]

(In reply to Jose Manuel Cantera from comment #16)

> I would prefer something more aligned with other APIs, for instance the
> mozContacts API. What about 
> 
> var reader = new VCardReader(vcardtext);
> var cursor = reader.getAll();
> cursor.onsuccess = function(evt) {
>   var contact = evt.target.result;
>   if(contact) {
>     cursor.continue();
>   }
> }

I'm quite late here but maybe it would be a good idea to have a more modern way using promises and generators.

See [1] and [2] for an example we just landed in SMS:
[1] https://github.com/mozilla-b2g/gaia/blob/7d54bbbea42343fc30649fd275c08ae247eec1b3/apps/sms/js/thread_list_ui.js#L148-L209
[2] https://github.com/mozilla-b2g/gaia/blob/7d54bbbea42343fc30649fd275c08ae247eec1b3/apps/sms/js/utils.js#L645-L676

Comment 27

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

I agree with Julien. We are already making a point in new Contacts code to use Promises everywhere, and this is a good opportunity to change those APIs to "Promised" ones.

I also didn't know that we could use generators, these are great news!

Comment 28

[Jose Manuel Cantera]

(In reply to Julien Wajsberg [:julienw] (PTO until 7th October, ask :steveck or :azasypkin for SMS stuff) from comment #26)
> (In reply to Jose Manuel Cantera from comment #16)
> 
> > I would prefer something more aligned with other APIs, for instance the
> > mozContacts API. What about 
> > 
> > var reader = new VCardReader(vcardtext);
> > var cursor = reader.getAll();
> > cursor.onsuccess = function(evt) {
> >   var contact = evt.target.result;
> >   if(contact) {
> >     cursor.continue();
> >   }
> > }
> 
> I'm quite late here but maybe it would be a good idea to have a more modern
> way using promises and generators.
> 
> See [1] and [2] for an example we just landed in SMS:
> [1]
> https://github.com/mozilla-b2g/gaia/blob/
> 7d54bbbea42343fc30649fd275c08ae247eec1b3/apps/sms/js/thread_list_ui.js#L148-
> L209
> [2]
> https://github.com/mozilla-b2g/gaia/blob/
> 7d54bbbea42343fc30649fd275c08ae247eec1b3/apps/sms/js/utils.js#L645-L676

We are using the common pattern for cursors that is in indexedDB as well.

Comment 29

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

And that's not bad, but we could embrace ES6 new features.

Speceially those ones created specifically for this kind of problems. If we go and check the definition of a generator it says something like: representing a lazy sequence (that could be infinite).

I think in the same case of promises, the sooner we start using it the better, and moving our code, or our refactors should be a way of introducing new, modern and useful techniques and avoid technical debt.

I want to go with the generators here, cannot see any better place.

Comment 30

[Julien Wajsberg [:julienw]]

(In reply to Jose Manuel Cantera from comment #28)

> We are using the common pattern for cursors that is in indexedDB as well.

José Manuel, I know this. Note that Gecko dev wants to change DOMRequest to Promise-like (by adding a .then method), so likely DOMCursor will change to generators one day too. It's so much easier to code using generators.

It's just a suggestion here, maybe you want to move this to a separate bug. But a refactoring bug looks to be a good bug to do this :)

Comment 31

[Jose Manuel Cantera]

(In reply to Julien Wajsberg [:julienw] (PTO until 7th October, ask :steveck or :azasypkin for SMS stuff) from comment #30)
> (In reply to Jose Manuel Cantera from comment #28)
> 
> > We are using the common pattern for cursors that is in indexedDB as well.
> 
> José Manuel, I know this. Note that Gecko dev wants to change DOMRequest to
> Promise-like (by adding a .then method), so likely DOMCursor will change to
> generators one day too. It's so much easier to code using generators.
> 
> It's just a suggestion here, maybe you want to move this to a separate bug.
> But a refactoring bug looks to be a good bug to do this :)

If we use generators how would look like the API ?

Comment 32

[Julien Wajsberg [:julienw]]

Could be something like:

  var reader = new VCardReader(vcardtext);
  for (var contact of reader.getAll()) {
    // do something with contact
  }

In that case it's synchronous.

If we want it to be asynchronous, it's possible too, but a little more involved.

Comment 33

[Jose Manuel Cantera]

(In reply to Julien Wajsberg [:julienw] (PTO until 7th October, ask :steveck or :azasypkin for SMS stuff) from comment #32)
> Could be something like:
> 
>   var reader = new VCardReader(vcardtext);
>   for (var contact of reader.getAll()) {
>     // do something with contact
>   }
> 
> In that case it's synchronous.
> 
> If we want it to be asynchronous, it's possible too, but a little more
> involved.

we need it to be async :)

Comment 34

[Jose Manuel Cantera]

As we agreed with Francisco and Sergi this morning we are aiming at landing this with the cursor approach. This will facilitate to make progress with bug 974589 . 

Next sprint we will deal with generators. For that purpose I have opened bug 1079916

Comment 35

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Comment on attachment 8494519 [details] [review]
patch v2 with tests

Looks good, but I have some simple questions on the code. Once these comments are answered/solve please assign review back to me.

Thanks!

Comment 36

[David Garcia [:davidg][retired]]

Attachment: patch v3

New patch including comments from Sergi, based on the cursor implementation from José Manuel. 
Includes implementation of activity 'import' for contacts using the new class. You can test this sending a vcard via bluetooth to the Dut.
Also rebased bug 999491 into the new class.

Comment 37

[Jose Manuel Cantera]

Comment on attachment 8504711 [details] [review]
patch v3

cancelling review. I left substantive comments on GH. Once they are fixed, please ask for another review

thanks!

Comment 38

[Jose Manuel Cantera]

Comment on attachment 8504711 [details] [review]
patch v3

David,

I left more substantive comments. Please check them. Also please respond to the question online or offline. 

thanks!

Comment 39

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Comment on attachment 8504711 [details] [review]
patch v3

I wrote some comments and agree with Jose Manuel's suggestions.

Comment 40

[Jose Manuel Cantera]

Comment on attachment 8504711 [details] [review]
patch v3

David,

thanks for the good work here. Please check GH as I left a couple of substantive comments that affect the robustness of the module.

Now I left Sergi to deal with further comments :)

cheers

Comment 41

[David Garcia [:davidg][retired]]

Thanks for the review Jose Manuel. I updated the patch with your comments.

Comment 42

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

Comment on attachment 8504711 [details] [review]
patch v3

It's a r+ for me when my comments in Github are addressed.

Thanks David!

Comment 43

[Adrián de la Rosa]

Hi, David. I am using your commit for bug https://bugzilla.mozilla.org/show_bug.cgi?id=974589 and I am getting this error when importing contacts with photos.

[JavaScript Error: "TypeError: utils.thumbnailImage is not a function" {file: "app://communications.gaiamobile.org/shared/js/contacts/import/utilities/vcard_reader.js" line: 715}]

And, if I'm not mistaken, it should be fixed by adding '/shared/js/contacts/utilities/image_thumbnail.js' to the list of dependencies of vcard_reader.js.

Gaia is closed, but every cloud has a silver lining ;) 

Thanks for your work!

Comment 44

[David Garcia [:davidg][retired]]

Hi Adrian. This just got landed before I could see your comment :(
Let me create a follow up for this :)

Comment 45

[David Garcia [:davidg][retired]]

Master: edde948b44c14b022bdc58540df33175e99d55e9