Closed Bug 946064 Opened 6 years ago Closed 6 years ago

[fugu][buri][contact] contact photo refresh when taping on "favorite" icon

Categories

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

Other
Gonk (Firefox OS)
defect
Not set
minor

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed)

RESOLVED FIXED
1.3 C1/1.4 S1(20dec)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: angelc04, Assigned: jmcf)

References

Details

Attachments

(1 file)

This can be reproduced on both buri V1.2 and V1.3.

STR:
1. Create a contact with photo
2. Launch Contact, tap on the contact created in step 1
3. Tap on the "star" icon to add contact to favorite
   --> You will see the contact photo refreshed. 

video for STR can be found: https://mozilla.box.com/s/79gttqx7ul5b8tr77oi5

buri 1.2 build
Gaia:     075e60c878b0eca68fba9e00bc85cb6eac03578a                         
Gecko:                                                                     
http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/14868788d50e         
BuildID   20131202004001                                                   
Version   26.0                                                             
ro.build.version.incremental=eng.archermind.20131114.105818  

buri 1.3 build
Gaia:     df070d4eed244e782dd3a7c2a7586d0741eac09f                         
Gecko:    http://hg.mozilla.org/mozilla-central/rev/9ac7ed427cd2           
BuildID   20131203151100                                                   
Version   28.0a1                                                           
ro.build.version.incremental=eng.zxliu.20131101.143946
that creates a strange visual effect on the left hand side of the photo.

To my mind a blocker at least for v1.3 and maybe also for v1.2
Assignee: nobody → jmcf
blocking-b2g: --- → 1.3?
See Also: → 942907
Attached file 14540.html
Attachment #8345342 - Flags: review?(francisco.jordano)
Interesting problem.

I would say that we have a problem of updating the detail view when the contact modified is the one that we are viewing right now.

This is pretty obvious in case of favourites, but also we could have the remote possibility of a facebook contact that we are viewing and gets updated cause the sync process. Remote possibility but same root.

I do agree that the reload is awful, Jose why don't we try to modify the approach, why if when we do a detail view refresh, we check that the image that we are setting is the same one that is already there, in order to not to force the refresh?

wdyt?
Flags: needinfo?(jmcf)
triage: 1.3+
blocking-b2g: 1.3? → 1.3+
(In reply to Francisco Jordano [:arcturus] from comment #3)
> Interesting problem.
> 
> I would say that we have a problem of updating the detail view when the
> contact modified is the one that we are viewing right now.
> 
> This is pretty obvious in case of favourites, but also we could have the
> remote possibility of a facebook contact that we are viewing and gets
> updated cause the sync process. Remote possibility but same root.

in that case we will be updating the data, as lastFavoriteId will not be set. 

> 
> I do agree that the reload is awful, Jose why don't we try to modify the
> approach, why if when we do a detail view refresh, we check that the image
> that we are setting is the same one that is already there, in order to not
> to force the refresh?

that would mean that we would need to query the DB one more time to try to find the new image, which it is awfully slow. By the way my patch also fixes a problem that was there for ages, the Facebook data is queried 2 times when getting access to the contact detail and that makes it awfully slow x 2. :)

> 
> wdyt?
Flags: needinfo?(jmcf)
(In reply to Jose M. Cantera from comment #5)
> that would mean that we would need to query the DB one more time to try to
> find the new image, which it is awfully slow. By the way my patch also fixes
> a problem that was there for ages, the Facebook data is queried 2 times when
> getting access to the contact detail and that makes it awfully slow x 2. :)
> 

My idea was more related to the |doReloadContactDetails| or |renderPhoto| itself, just checking that the dom alreay has the same photo, in that case we don't repaint.

If the patches fixes another bug, that's another bug :)

Anyway will check again.

Thanks.
Still not convinced with my previous comment?
Flags: needinfo?(jmcf)
(In reply to Francisco Jordano [:arcturus] from comment #6)
> (In reply to Jose M. Cantera from comment #5)
> > that would mean that we would need to query the DB one more time to try to
> > find the new image, which it is awfully slow. By the way my patch also fixes
> > a problem that was there for ages, the Facebook data is queried 2 times when
> > getting access to the contact detail and that makes it awfully slow x 2. :)
> > 
> 
> My idea was more related to the |doReloadContactDetails| or |renderPhoto|
> itself, just checking that the dom alreay has the same photo, in that case
> we don't repaint.

But how do would you check the equality of two photos? 

> 
> If the patches fixes another bug, that's another bug :)
> 
> Anyway will check again.
> 
> Thanks.
Flags: needinfo?(jmcf)
What about if do a simple check:

- Check the style.backgroundImage property, if it's filed we don't reupdate.
- Ensure that property is cleaned when we leave the detail view.

Maybe I'm missing something else, but we could be sure that we are free of bugs of images changing from one user to another and fix this bug as well?

wdyt?
Alternative approach for not reloading detail img implemented. Flagging ni to Francisco.
Flags: needinfo?(francisco.jordano)
Comment on attachment 8345342 [details]
14540.html

nit comment on github.

Could you add a unit tests (with a fake image bytes, same and different).

Cheers!
r+plusing!
Attachment #8345342 - Flags: review?(francisco.jordano) → review+
Flags: needinfo?(francisco.jordano)
https://github.com/mozilla-b2g/gaia/commit/7173a82a65719aeae83c5b393f1320368bc4b5c0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Note: The commit includes unit test and the nit changes requested by the reviewer
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
Uplifted 7173a82a65719aeae83c5b393f1320368bc4b5c0 to:
v1.3: ba90382ab827570f441715bff53d9b8b3146874d
You need to log in before you can comment on or make changes to this bug.