1022646 - [Contacts][Haida] Separate the form view

Status: RESOLVED WONTFIX

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

Description

[Michał Budzyński (:michalbe)]

Attachment: [WIP] patch

Comment 1

[Michał Budzyński (:michalbe)]

According to https://bugzilla.mozilla.org/show_bug.cgi?id=946750#c14, moving the patch in here. Comment to the patch:
"Some tests are still failing, but since it's a huge architecture change, I would love :arcturus to start reviewing it while I'll fix them."

Comment 2

[Jose Manuel Cantera]

I did a basic smoke testing and found

[JavaScript Error: "TypeError: utils.getContactById is not a function" {file: "app://communications.gaiamobile.org/contacts/js/contacts_matching_controller.js" line: 107}]

this happened while editing and updating the family name of a contact.

There are as well performance regressions. When you click on the '+' sign it takes a while before the form appears and when it appears, transiently all the multiple fields appear collapsed and later they appear on their intended state.

Comment 3

[Michał Budzyński (:michalbe)]

Thanks Jose M. for your comments here & on GH, I'll fix rest of the tests and investigate what's wrong with the issue you described here. It's a big patch and your help is very important.

About the delay - what I'm doing now is waiting for the iframe to load requested view, and start animation when iframe's onload() fires. Without this, we will have just plain, white window sliding in, and then the view will be rendered after a while. Neither of the solutions are good. Do you have any idea on how it should work? I thought about displaying a placeholder image in the iframe first (like we did with the system apps while ago) and when view will be rendered, we will remove the image and replace it with HTML. What do you think? Also - I think this issue will not occur when we will have full haida support from the system. 

Thanks one more time!

Comment 4

[Jose Manuel Cantera]

Comment on attachment 8436927 [details] [review]
[WIP] patch

I have made a first quick review and provided sufficient feedback at this moment

Thanks Michal

Comment 5

[Jose Manuel Cantera]

(In reply to Michał Budzyński (:michalbe) from comment #3)
> Thanks Jose M. for your comments here & on GH, I'll fix rest of the tests
> and investigate what's wrong with the issue you described here. It's a big
> patch and your help is very important.
> 
> About the delay - what I'm doing now is waiting for the iframe to load
> requested view, and start animation when iframe's onload() fires. 

I think we can save precious time if we don't load all the script files on the form.html but only load the minimum amount of script to make appear the form page. The other scripts could be loaded later by Lazy Load. 

Without
> this, we will have just plain, white window sliding in, and then the view
> will be rendered after a while. Neither of the solutions are good. Do you
> have any idea on how it should work? I thought about displaying a
> placeholder image in the iframe first (like we did with the system apps
> while ago) and when view will be rendered, we will remove the image and
> replace it with HTML.

I would prefer a solution like the one I've proposed above

 What do you think? Also - I think this issue will not
> occur when we will have full haida support from the system. 
> 

Yes, but for the moment we should ensure no perf regressions are introduced

> Thanks one more time!

Comment 6

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

Comment on attachment 8436927 [details] [review]
[WIP] patch

Stunning work here Michal, congratulations, it's a difficult an delicate patch.

I left some comments on github. Overall I also agree with Jose trying to slim the form.html document from css and js as much as possible, that was one of the goals to be more efficient as well.

Regarding the performance problem that Jose comments, it's true, but we kind of have that problem right now, the first time you go to the detail, anyway, as he says we should tackle that.

Playing with the app found the following errors, I'm sure you'll fix quickly:

- We have an activity, add to existing contact from the call log that is opening directly the form view. I guess that we forgot this condition when changing the activity handler.

- Also in the activity of creating a new contact, which works pretty well without transition, I cannot cancel the activity, or when saving the contact the activity doesn't dismiss.

- When in contact detail and clicking on find duplicates I have the following error:
Find duplicates:
E/GeckoConsole( 5398): Content JS ERROR at about:neterror?e=fileNotFound&u=app%3A//communications.gaiamobile.org/contacts/null&c=UTF-8&f=regular&m=app%3A//communications.gaiamobile.org/manifest.webapp&d=Firefox%20can%27t%20find%20the%20file%20at%20app%3A//communications.gaiamobile.org/contacts/null.:1213 in initPage: net-error

Again, fantastic work

Comment 7

[Michał Budzyński (:michalbe)]

Oh, some of them are regressions then, I remember fixing this in Taipei. I'm on it now, thanks for your great & valuable comments!

Comment 8

[Michał Budzyński (:michalbe)]

Comment on attachment 8436927 [details] [review]
[WIP] patch

I've updated the pull request with a messageBroadcaster to contacts [1]. It's a shim that broadcast messages sent in any of the iframes (haida sheets) to all the other views in the same app. It will be our main way of communication between views in contacts for now, till system support form broadcasting messages will not land. I'm changing parts of the logic now to use it (like getting rid of this pseudo-controller for details - I'll just send event from form to the main view, etc.), but if you guys have some time please share your thoughts on this solution (r? this part now please).

[1] https://github.com/michalbe/gaia/blob/Bug946750-form/apps/communications/contacts/js/broadcast_message.js

Comment 9

[Michał Budzyński (:michalbe)]

I'm rebuilding now the way we select Tags for phone numbers in forms using broadcasted events. It's a little bit bigger then I expected, but I should finish today.

Comment 10

[Michał Budzyński (:michalbe)]

The problem with ContactTags is that in some places we pass Dom nodes between modules, and we cannot do that with postMessage, so I'm trying to separate those views as deep as possible.

Comment 11

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

Comment on attachment 8436927 [details] [review]
[WIP] patch

Nice,

we will be able to reuse that event listener with minimun changes when we have all contacts haidified.

Just did a r- cause:

- Checked the errors commented abovoe and still on place.
- When using the broadcast emitter to change a tag I found the following error:
Timestamp: 17/06/2014 19:45:35
Error: DataCloneError: The object could not be cloned.
Source File: app://communications.gaiamobile.org/contacts/js/broadcast_message.js
Line: 64

Thanks for the work, we are getting close!

Comment 12

[Jose Manuel Cantera]

Comment on attachment 8436927 [details] [review]
[WIP] patch

canceling my review as per Francisco's comments more work is needed. I will resume it once a new version of the patch is available

Comment 13

[Michał Budzyński (:michalbe)]

Guys, I just wanted you to R the broadcasting messanger file, nothing else, sorry for not being clear on that.

Comment 14

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

(In reply to Michał Budzyński (:michalbe) from comment #13)
> Guys, I just wanted you to R the broadcasting messanger file, nothing else,
> sorry for not being clear on that.

Oh :), sorry went for a full review again.

So yeah, you can ask for feedback instead of review, anyway just left some comments on the broadcaster messenger, feedback + definitely.

Left a couple of ni, the most important par is about |isTopWindow|, i would change it to a function that checks, instead of a value that is calculated at the begining of the script.

A part from that looking pretty good :D

Thanks Michal!!

Comment 15

[Michał Budzyński (:michalbe)]

I partially fix changing/adding the tag to contacts so for instance the error described by :arcturus in Comment 11 doesn't occur anymore. Getting back to fixing rest of it.

Comment 16

[Michał Budzyński (:michalbe)]

UPDATE: Changing tags and setting custom tags works now [1], the only thing left in 'tags' is disabling duplication of date related tags. Working on it now.

[1] https://github.com/michalbe/gaia/commit/1d8a2755aad23f82f447051e26f6980489777b6b

Comment 17

[Michał Budzyński (:michalbe)]

UPDATE: I moved some parts of the logic to different places & connect everything with broadcasting messages, exclusive tags are not duplicated now. I'm working on implementing same thing on the form view (because for now when you add new date, it's not filtering tags)

Comment 18

[Michał Budzyński (:michalbe)]

UPDATE: Parts of the ContactsTags moved before to form returned to ContactsTags, it will be easier to manage tags now.

[1] https://github.com/michalbe/gaia/commit/b4c79df6cd5b650cec2988e16e3eca7e1f0d7f3e

Comment 19

[Michał Budzyński (:michalbe)]

UPDATE: All the issues related to tags are fixed now [1].

[1] https://github.com/michalbe/gaia/commit/81fad9e4e4fd22d35a5aaa152e2fb4e108f95818

Comment 20

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

\o/

Comment 21

[Michał Budzyński (:michalbe)]

Changing current contact sends broadcasting event now (to prevent Bug 1015060 to happen again). https://github.com/michalbe/gaia/commit/11312b90ecd3abf865e87587a417c0af31fe3b4a

Comment 22

[Michał Budzyński (:michalbe)]

It's to early to introduce separate controllers for every view (so far we have jus 2 of them separated), so we removed them & moved necessary methods to `utils` https://github.com/michalbe/gaia/commit/16f0d63632de2faaee25eadb4b692a7874649115

Comment 23

[Michał Budzyński (:michalbe)]

'new' activity fixed, since we don't use 'launch_activity' function there anymore (with a separated view we don't need to change hash & handle animation there) the `handle` switch set the name of the current activity now.

Comment 24

[Michał Budzyński (:michalbe)]

Video that shows this behavior is attached to contacts wiki: https://wiki.mozilla.org/Gaia/Contacts/Scrum/3#Demos

Comment 25

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

(In reply to Michał Budzyński (:michalbe) from comment #24)
> Video that shows this behavior is attached to contacts wiki:
> https://wiki.mozilla.org/Gaia/Contacts/Scrum/3#Demos

Awesome! Thanks Michal!

Comment 26

[Michał Budzyński (:michalbe)]

UPDATE: Somehow I removed the callback support in initForm() method of contacts.js, thats why `update` activity didn't work as expected, problem is partially solved now: https://github.com/michalbe/gaia/commit/c4e77d0890522dd58d7e347fdae1e544e9ea5229

Comment 27

[Michał Budzyński (:michalbe)]

UPDATE: params.extras from contacts view are now sent to the form view and properly parsed there, therefore we can add a phone number to existing contact with `update` activity again. There are still some minor issues I work on now. Commit: https://github.com/michalbe/gaia/commit/21debd6eded8428e3c7381dbae2446eed5f23708

Comment 28

[Michał Budzyński (:michalbe)]

UPDATE: I could find 4 issues with current patch:

 * `callback is not a function` in navigation.js:244 on changing the date tag in edit/form view
 * after updating the contacts with `update` activity it's not dismissing the contacts view
 * when opening contact from dialer for the first time nothing happen but error: `fb.isFbContact is not a function` in stray.js:19
 * When removing contact from dialer (or `update/open` activities) `Contacts.navigation is undefined` on form.js:644

working on fixing those now

Comment 29

[Michał Budzyński (:michalbe)]

`callback is not a function` in navigation.js:244 fixed. Somehow sometimes we get mouse event object there instead of a function. https://github.com/michalbe/gaia/commit/5f0c181e3805322cd930d4e63d6b24fbff0b1086

Comment 30

[Michał Budzyński (:michalbe)]

UPDATE: As explained in one of the comments in the commit[1], since `update` activity starts in the contact list and then open form view (separate iframe), they both have their own scope and ActivityHandler instance. Since we cannot send a contact object using messageBroadcaster, we send just the id (from the form to the main view) so the activity handler that initialized the process can use utils.getContactById() to get the contact that will be posted as an activity result.


[1] Getting back from `update` activity fixed https://github.com/michalbe/gaia/commit/8d793f486caffed1c3d1a2fc8d301b3972e88c3a

Comment 31

[Michał Budzyński (:michalbe)]

UPDATE: third found regression, `fb.isFbContact()  is not a function`, was there because of the race condition we had with LazyLoader & opening the view during the activity. It's fixed by introducing getDepencencies() function that returns dependencies when needed in the following commit: https://github.com/michalbe/gaia/commit/5fb83af1a9380cf79904af2ebd9a986c784e3a1b

Comment 32

[Michał Budzyński (:michalbe)]

UPDATE: (next error - When removing contact from dialer `Contacts.navigation is undefined` on form.js:644)    
Somehow reference to the navigation manger still tried to call it from Contacts. Fixed: https://github.com/michalbe/gaia/commit/f1b3856ab52478a06ddc8f57d16226fdbf25d5b6

Comment 33

[Michał Budzyński (:michalbe)]

Comment on attachment 8436927 [details] [review]
[WIP] patch

Ok guys, with todays changes, as predicted during the morning's contacts meeting, I would like you to take a look and r my patch again. For last two days I was fighting with regressions and spend most of my time smoketesting the app from all the angles, but you probably know more edge case than I do, or your fresh look will help us eliminate rest of the bugs. In the meantime, I work on fixing tests (so far we have 38 units & 6 marionettes failing).

Comment 34

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

Comment on attachment 8436927 [details] [review]
[WIP] patch

Hi Michal,

thanks again for the patch, definitely we are in the correct direction.

Left some minor comments on github.

Only two things:
- tags are not working while we access contact from the dialer
- some times the navigation does something weird and lose the transitions, the view just appear and dissapear.
- unneeded files in form.html I've tried to run some kind of static analysis tool but took me a lot of time and didn't get anything cause of the complexity of gaia apps, but still aware that maybe we are loading too much unneded JS code on the form.html

Thanks!

Comment 35

[Jose Manuel Cantera]

Comment on attachment 8436927 [details] [review]
[WIP] patch

I've just applied the Haida patch and found the same problems as in the previous versions, i.e. performance when loading the page. 

Moreover I cannot add any contact 

[JavaScript Error: "TypeError: utils.getContactById is not a function" {file: "app://communications.gaiamobile.org/contacts/js/contacts_matching_controller.js" line: 107}]

Comment 36

[Michał Budzyński (:michalbe)]

(In reply to Francisco Jordano [:arcturus] [:francisco] from comment #34)
> Comment on attachment 8436927 [details] [review]
> [WIP] patch
> 
> Hi Michal,
> 
> thanks again for the patch, definitely we are in the correct direction.
> 
> Left some minor comments on github.
> 
> Only two things:
> - tags are not working while we access contact from the dialer

Oh, it's a broadcaster issue, I'm on it now.

> - some times the navigation does something weird and lose the transitions,
> the view just appear and dissapear.

Could you write something more? I worked with this code on daily basis and never experienced anything like this.

> - unneeded files in form.html I've tried to run some kind of static analysis
> tool but took me a lot of time and didn't get anything cause of the
> complexity of gaia apps, but still aware that maybe we are loading too much
> unneded JS code on the form.html

I'm on it as well.

Comment 37

[Michał Budzyński (:michalbe)]

(In reply to Jose Manuel Cantera from comment #35)
> Moreover I cannot add any contact 
> 
> [JavaScript Error: "TypeError: utils.getContactById is not a function"
> {file:
> "app://communications.gaiamobile.org/contacts/js/
> contacts_matching_controller.js" line: 107}]

Could you describe exactly your way to reproduce this bug? I don't have anything like this (maybe try today's rebased version of the patch?), as far as I know Francisco don't either.

Comment 38

[Michał Budzyński (:michalbe)]

I've created an etherpad to track dependencies of the FormView https://etherpad.mozilla.org/contacts-form-view-dependencies

Comment 39

[Michał Budzyński (:michalbe)]

I made a little research, and remove files we don't use in the FormView from form.html.

CSS: https://github.com/michalbe/gaia/commit/b265bd7d69efd687e3371cca191c9a2064f976c8
JS: https://github.com/michalbe/gaia/commit/ee3071469548b442717e25d9c4e47e12cc421af8

Comment 40

[Michał Budzyński (:michalbe)]

Comment on attachment 8436927 [details] [review]
[WIP] patch

All the concerns you guys had before are fixed now, so I'm getting back to fixing tests, and ask you for review/feedback again. Thanks.

Comment 41

[Michał Budzyński (:michalbe)]

7 tests in Activities fixed - https://github.com/michalbe/gaia/commit/efc934db210af56ec44d769f09a8fb4c83e8e1bc

38 more to go.

Comment 42

[Michał Budzyński (:michalbe)]

UPDATE:

Test fixing progress
Generic export tests fixed: https://github.com/michalbe/gaia/commit/ffe6c938376da077d132ad3131905d162dbed0a6
Import tests fixed https://github.com/michalbe/gaia/commit/3d6ee756ddca32bc2f7f9653ab3d0c19b0bb1389
contacts_test.js tests fixed https://github.com/michalbe/gaia/commit/55750107bc31edb0c3de291a680c57ffb91cf3ec
contacts_bulk_delete_test.js https://github.com/michalbe/gaia/commit/c14b55bde5382f91a380a54355209ec3685c90db
Details view tests https://github.com/michalbe/gaia/commit/555d189b6c25b7c7542137ea4573eb28718e23ec

8 failing tests left.

Comment 43

[Michał Budzyński (:michalbe)]

UPDATE:

tests fixed in form_test.js https://github.com/michalbe/gaia/commit/89f588abda2f75a74ad3af38878ba30c90119116

Just 3 are still failing, they need rewriting original tests for updatePhoto (working with francisco on fixing it). Also - 4 test are failing in dialer (mmi), and according to Rik they are not connected to the work made by this bug (failing also on master).

Comment 44

[Michał Budzyński (:michalbe)]

Details tests fixed, all tests in contacts pass now. https://github.com/michalbe/gaia/commit/081d617932008b8747871fc9179e245b5eb17152

Getting to marionette tests now.

Comment 45

[Michał Budzyński (:michalbe)]

In the meantime, fixed merge bugs found by Francisco: https://github.com/michalbe/gaia/commit/81cb8b297d2c0bc74de37f54ac2ea5d2c990df6d

Comment 46

[Jose Manuel Cantera]

just my report for today

a/ the candy progress bar when adding contacts has disappeared
b/ contacts duplicates, does not work. at least when editing a contact

just a couple of reasons to be negative one more time

Comment 47

[Michał Budzyński (:michalbe)]

Comment on attachment 8436927 [details] [review]
[WIP] patch

Sorry Jose but your feedback is not satisfying me. I expected you (and thought it was clear after yesterdays Contact meeting) to finally make a proper, full review - if you'll find more issues at the beginning I'll be able to fix all of them at once, not ask you for feedback every 2 minor issues because "it's just a couple of reasons to be negative one more time". We are really close to the end, and I don't want to wait for r+ days after finishing final PR because the patch is quite big (almost 30 files affected). I hope I'm clear here. 
Also - what do you mean by 'does not work'? Could you be more clear when you find a bug? How to reproduce, what happened, what should, etc? Asking for feedback again, please do it as you should this time.

Comment 48

[Jose Manuel Cantera]

Michal,

I'm not a QA person for you. So when I start reviewing a patch the minimum thing I expect is that it works. And yours does not work. It is YOUR WORK to test all the regressions and fix them. My work is to review the code not to be a tester for you

thanks

Comment 49

[Michał Budzyński (:michalbe)]

I don't think you read what I wrote in previous comment, but since everything was explained on IRC by me & Francisco during todays daily Contacts meeting, I'm waiting for your detailed feedback today.

Comment 50

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

Hi Jose,

you are right, you are not QA, and Michal is not expecting that you do a QA review of the patch.

But is a good practice to give feedback even before if the patch is not 100% complete and we know that are parts that are failing.

The earlier the feedback it is the better for everyone, so Michal has time to address everything.

Thanks!

Comment 51

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

Comment on attachment 8436927 [details] [review]
[WIP] patch

Sorry, gave the feedback in IRC but not here.

Looking good, we saw some problems, and we are in the direction.
Performance increased once that we are not loading all the unnesary scripts \o/

Will try to give a hand on the tests, but ready to start with the full review.

Thanks Michal for the amazing work here, we all know this is an impressive and complicated task.

Comment 52

[Jose Manuel Cantera]

Apart from the performance issues found before, which in the case of Facebook contacts is even worse, I've found that when editing a Facebook contact the photo cannot be changed. That's a regression.

I left more comments on GH, although the most important thing to be fixed right now is the performance when loading the form. Please let me know if I can help

thanks Michal for the work

Comment 53

[Jose Manuel Cantera]

Comment on attachment 8436927 [details] [review]
[WIP] patch

cancelling f? until we have another version

Comment 54

[BMO Automation]

Firefox OS is not being worked on