Closed Bug 1022646 Opened 9 years ago Closed 5 years ago

[Contacts][Haida] Separate the form view

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: mbudzynski, Unassigned)

References

Details

(Whiteboard: [p=12])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
arcturus
: feedback+
Details | Review
Attached file [WIP] patch
      No description provided.
Attachment #8436927 - Flags: review?(francisco)
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."
Attachment #8436927 - Flags: review?(jmcf)
Summary: [Contacts][Haida] Sepate the form view → [Contacts][Haida] Separate the form view
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.
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 on attachment 8436927 [details] [review]
[WIP] patch

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

Thanks Michal
Attachment #8436927 - Flags: review?(jmcf)
(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!
Whiteboard: [p=]
Target Milestone: --- → 2.0 S4 (20june)
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
Attachment #8436927 - Flags: review?(francisco)
Oh, some of them are regressions then, I remember fixing this in Taipei. I'm on it now, thanks for your great & valuable comments!
Whiteboard: [p=] → [p=3]
Attachment #8436927 - Flags: review?(jmcf)
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
Attachment #8436927 - Flags: review?(francisco)
Attachment #8436927 - Flags: review?(jmcf)
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.
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 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!
Attachment #8436927 - Flags: review?(francisco) → review-
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
Attachment #8436927 - Flags: review?(jmcf)
Guys, I just wanted you to R the broadcasting messanger file, nothing else, sorry for not being clear on that.
(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!!
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.
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
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)
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
UPDATE: All the issues related to tags are fixed now [1].

[1] https://github.com/michalbe/gaia/commit/81fad9e4e4fd22d35a5aaa152e2fb4e108f95818
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Whiteboard: [p=3] → [p=12]
Changing current contact sends broadcasting event now (to prevent Bug 1015060 to happen again). https://github.com/michalbe/gaia/commit/11312b90ecd3abf865e87587a417c0af31fe3b4a
Whiteboard: [p=12] → [p=3]
Whiteboard: [p=3] → [p=12]
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
'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.
Video that shows this behavior is attached to contacts wiki: https://wiki.mozilla.org/Gaia/Contacts/Scrum/3#Demos
(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!
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
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
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
`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
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
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
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 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).
Attachment #8436927 - Flags: review?(jmcf)
Attachment #8436927 - Flags: review?(francisco)
Attachment #8436927 - Flags: review-
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!
Attachment #8436927 - Flags: review?(francisco) → feedback+
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}]
Attachment #8436927 - Flags: review?(jmcf) → review-
(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.
(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.
I've created an etherpad to track dependencies of the FormView https://etherpad.mozilla.org/contacts-form-view-dependencies
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.
Attachment #8436927 - Flags: review-
Attachment #8436927 - Flags: feedback?(jmcf)
Attachment #8436927 - Flags: feedback?(francisco)
Attachment #8436927 - Flags: feedback+
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).
Details tests fixed, all tests in contacts pass now. https://github.com/michalbe/gaia/commit/081d617932008b8747871fc9179e245b5eb17152

Getting to marionette tests now.
In the meantime, fixed merge bugs found by Francisco: https://github.com/michalbe/gaia/commit/81cb8b297d2c0bc74de37f54ac2ea5d2c990df6d
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
Attachment #8436927 - Flags: feedback?(jmcf) → feedback-
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.
Attachment #8436927 - Flags: feedback- → feedback?(jmcf)
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
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.
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 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.
Attachment #8436927 - Flags: feedback?(francisco) → feedback+
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
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Depends on: 1036370
Depends on: 1037003
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Comment on attachment 8436927 [details] [review]
[WIP] patch

cancelling f? until we have another version
Attachment #8436927 - Flags: feedback?(jmcf)
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Target Milestone: 2.1 S3 (29aug) → 2.1 S4 (12sep)
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
Assignee: mbudzynski → jeroentulp
Depends on: 1067306
Target Milestone: 2.1 S5 (26sep) → ---
No longer depends on: 1067306
Assignee: jeroentulp → nobody
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.