Closed Bug 950671 Opened 6 years ago Closed 6 years ago

Detail contacts shows the previous photo contact for a while and the animation is not very synchronized

Categories

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

All
Gonk (Firefox OS)
defect
Not set

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: crdlc, Assigned: crdlc)

References

Details

Attachments

(1 file)

1) Run the transitions when the photo is ready
2) Reuse url image from list
3) Remove the last photo contact when the view is closed
Assignee: nobody → crdlc
Status: NEW → ASSIGNED
Attached file Patch v1
Attachment #8348035 - Flags: review?(francisco.jordano)
Attachment #8348035 - Flags: feedback?(jmcf)
Attachment #8348035 - Flags: feedback?(bkelly)
Comment on attachment 8348035 [details]
Patch v1

I don't think this will work correctly.  The URL object is cached in photo.url, but when we scroll the contact off the screen that URL object will be revoked.  This will leave a broken URL object in the cache.

Since we have to do manual revokation of that URL objects (as far as I can tell), I think we should avoid any caching of them.  Each location that sets the URL should probably create and revoke them individually.
Attachment #8348035 - Flags: feedback?(bkelly) → feedback-
(In reply to Ben Kelly [:bkelly] from comment #2)
> Comment on attachment 8348035 [details]
> Patch v1
> 
> I don't think this will work correctly.  The URL object is cached in
> photo.url, but when we scroll the contact off the screen that URL object
> will be revoked.  This will leave a broken URL object in the cache.
> 
> Since we have to do manual revokation of that URL objects (as far as I can
> tell), I think we should avoid any caching of them.  Each location that sets
> the URL should probably create and revoke them individually.

We are thinking on backing out the patch concerning revokeObjectUrls as it has broken the contact search screen.
(In reply to Jose M. Cantera from comment #3)
> We are thinking on backing out the patch concerning revokeObjectUrls as it
> has broken the contact search screen.

Did the patch I provided on that bug not help?  If not, I'm ok with backing it out.
(In reply to Ben Kelly [:bkelly] from comment #2)
> Comment on attachment 8348035 [details]
> Patch v1
> 
> I don't think this will work correctly.  The URL object is cached in
> photo.url, but when we scroll the contact off the screen that URL object
> will be revoked.  This will leave a broken URL object in the cache.
> 

But we cannot open the detail of a contact that is not being shown on the screen

> Since we have to do manual revokation of that URL objects (as far as I can
> tell), I think we should avoid any caching of them.  Each location that sets
> the URL should probably create and revoke them individually.
(In reply to Jose M. Cantera from comment #5)
> (In reply to Ben Kelly [:bkelly] from comment #2)
> > Comment on attachment 8348035 [details]
> > Patch v1
> > 
> > I don't think this will work correctly.  The URL object is cached in
> > photo.url, but when we scroll the contact off the screen that URL object
> > will be revoked.  This will leave a broken URL object in the cache.
> > 
> 
> But we cannot open the detail of a contact that is not being shown on the
> screen

I guess my issue is more with the "getPhotoURL()" function exposed by list.js here, and not its particular use in the details window.  As you say, it should work for details because of how the apps is designed.  If someone else tries to use it, though, they may be surprised that the returned URLs just magically become bad at some point later.
Is performing the createObjectURL() really slow or something?  What do we gain by sharing it between the list and the details window?

Like I said, though, if the other problems with revokeObjectURL() aren't solvable, I'm ok just backing it out.  It would be nice to free that memory for contacts not being viewed, though.
(In reply to Ben Kelly [:bkelly] from comment #7)
> Is performing the createObjectURL() really slow or something?  What do we
> gain by sharing it between the list and the details window?

I am pretty sure that sharing is faster than creating a new one but... no so much

> 
> Like I said, though, if the other problems with revokeObjectURL() aren't
> solvable, I'm ok just backing it out.  It would be nice to free that memory
> for contacts not being viewed, though.

Two solutions:

1) Don't share this info
2) or share this info adding a comment to that function (this url is valid if the contact is on the viewport or something like that)
Considering we re-execute createObjectURL() during scrolling, I don't think its particularly expensive.  Given that memory is at a premium, I'd rather go with don't share the URL objects and have details window createObjectURL/revokeObjectURL itself.  Just my opinion.
Comment on attachment 8348035 [details]
Patch v1

code looks good and works perfectly. A great improvement that kills an old bug in the Contacts App, making the product better
Attachment #8348035 - Flags: feedback?(jmcf) → feedback+
blocking-b2g: --- → 1.3?
(In reply to Ben Kelly [:bkelly] from comment #4)
> (In reply to Jose M. Cantera from comment #3)
> > We are thinking on backing out the patch concerning revokeObjectUrls as it
> > has broken the contact search screen.
> 
> Did the patch I provided on that bug not help?  If not, I'm ok with backing
> it out.

German is working on that. He is on PTO today but he will be on it tomorrow
(In reply to Ben Kelly [:bkelly] from comment #6)
> (In reply to Jose M. Cantera from comment #5)
> > (In reply to Ben Kelly [:bkelly] from comment #2)
> > > Comment on attachment 8348035 [details]
> > > Patch v1
> > > 
> > > I don't think this will work correctly.  The URL object is cached in
> > > photo.url, but when we scroll the contact off the screen that URL object
> > > will be revoked.  This will leave a broken URL object in the cache.
> > > 
> > 
> > But we cannot open the detail of a contact that is not being shown on the
> > screen
> 
> I guess my issue is more with the "getPhotoURL()" function exposed by
> list.js here, and not its particular use in the details window.  As you say,
> it should work for details because of how the apps is designed.  If someone
> else tries to use it, though, they may be surprised that the returned URLs
> just magically become bad at some point later.

maybe that can be solved by deleting the URL from the cache if that's revoked, but of course probably it will not be needed if we finally back out revokeObjectUrl patch
(In reply to Ben Kelly [:bkelly] from comment #7)
> Is performing the createObjectURL() really slow or something?  What do we
> gain by sharing it between the list and the details window?
> 
> Like I said, though, if the other problems with revokeObjectURL() aren't
> solvable, I'm ok just backing it out.  It would be nice to free that memory
> for contacts not being viewed, though.

Be careful, We are not proposing to reuse the URL to avoid its creation but to avoid loading the photo one more time in the details view. Our point here is that AFAIK the contact.photo field is retrieved lazily once it is accessed, so if we reuse the photo and the URL from contacts.List module, we will saving one more photo retrieval, thus making it faster to load in the details section.
Comment on attachment 8348035 [details]
Patch v1

Hi,

already left a comment on github.

Basically I'm ok with the patch except the part of sharing the images from the contact list and the detail. Not just cause of the reasons that Ben comments, but mainly cause of the dependency that we create between the list and the details. We are trying to avoid those kind of dependencies in bug 950665.

So will be ok just recoding that :)

Thanks a lot Cristian for the great job!
Attachment #8348035 - Flags: review?(francisco.jordano)
Comment on attachment 8348035 [details]
Patch v1

Done. Ben & Fran won, Cris & Jose lost jajaja :)
Attachment #8348035 - Flags: review?(francisco.jordano)
(In reply to Cristian Rodriguez (:crdlc) PTO (12-20 to 01-13) from comment #15)
> Comment on attachment 8348035 [details]
> Patch v1
> 
> Done. Ben & Fran won, Cris & Jose lost jajaja :)

for the sake of consensus and team work :)
Comment on attachment 8348035 [details]
Patch v1

Lovely, thanks guys!

r+ :)
Attachment #8348035 - Flags: review?(francisco.jordano) → review+
Merged in master:

https://github.com/crdlc/gaia/commit/ee40eaadd571c1ab63a82a983a5edfb3a11d27c6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
triage: 1.3+
blocking-b2g: 1.3? → 1.3+
Uplifted ee40eaadd571c1ab63a82a983a5edfb3a11d27c6 to:
v1.3: 28b251d3476cdeae0fbb57961900191bfc48ad61
Duplicate of this bug: 952323
You need to log in before you can comment on or make changes to this bug.