Closed Bug 1112135 Opened 5 years ago Closed 5 years ago

[Messages] When starting as an activity/notification/, delay threads rendering until the right panel is open

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S3 (9jan)

People

(Reporter: julienw, Assigned: azasypkin)

References

Details

(Whiteboard: [p=2])

Attachments

(1 file)

While working on bug 1109745, we see that the logic to open the right panel competes with the thread rendering that happens in background.

In this bug, the goal is to delay the thread rendering so that opening the right panel is really faster.
Some updates:

* Delaying threads rendering (and retrieving as currently it's highly coupled) works quite good when next panel is Composer("share" activity and "new" activity when contact doesn't have existing thread);

* For "notification" and "new" activity(for contact with existing thread) cases we have dependencies for the thread object (Thread.active) and Drafts that are fulfilled during thread list rendering. At the first look "new" activity kind of partially works as ActivityHandler creates corresponding Thread object (via Threads.registerMessage), but we'll still miss draft for this thread if it exists.

Will think how we can overcome this.
We can use Drafts.request() ?
(eventually we need to convert this to promise, btw :) )
(In reply to Julien Wajsberg [:julienw] from comment #2)
> We can use Drafts.request() ?
> (eventually we need to convert this to promise, btw :) )

Yep!
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
Hey Julien,

Here is the WIP, without units for now. I played a bit with it, and it feels better on flame + heavy workload. 

The only thing is that there's noticeable delay when you open Thread with draft via "new" activity.

Could you please give it a try?

Thanks!
Attachment #8539435 - Flags: feedback?(felash)
Comment on attachment 8539435 [details] [review]
GitHub pull request URL

I tested "share" and "new" activities, and both are really faster now. I couldn't "notification" but I suspect it's the same :) Good work!
Attachment #8539435 - Flags: feedback?(felash) → feedback+
Comment on attachment 8539435 [details] [review]
GitHub pull request URL

(In reply to Julien Wajsberg [:julienw] (PTO 12/25 -> 12/29) from comment #5)
> Comment on attachment 8539435 [details] [review]
> GitHub pull request URL (WIP)
> 
> I tested "share" and "new" activities, and both are really faster now. I
> couldn't "notification" but I suspect it's the same :) Good work!

Hey Julien,

Thanks for the feedback! I've added unit tests + some adjustments as separate commit.

Could you please review it?
Attachment #8539435 - Attachment description: GitHub pull request URL (WIP) → GitHub pull request URL
Attachment #8539435 - Flags: review?(felash)
Comment on attachment 8539435 [details] [review]
GitHub pull request URL

Added comments on github.

This is really looking good. I compared both with master and with my v2.0 phone, and it's now faster than both versions for new, share and notification.

For share, we can still feel that displaying the image struggles because we started rendering the thread list. I think we can do better in the future, like appending the image in beforeEnter and resolving the returned promise only once the image is loaded. It will be for a separate bug but it can be a nice improvement, and this will look slicker because the panel will be nearly ready when it will be displayed. Anyway, this is for a separate bug, let's finish this bug first!
Attachment #8539435 - Flags: review?(felash)
Comment on attachment 8539435 [details] [review]
GitHub pull request URL

Hey Julien,

Fixed per your comments + rebased patch on the latest master as it had some conflicts.

Could you please take a look again?

Thanks!
Attachment #8539435 - Flags: review?(felash)
Comment on attachment 8539435 [details] [review]
GitHub pull request URL

r=me with a last nit in test

please land with green tests !

Thanks a lot, this is great :)
Attachment #8539435 - Flags: review?(felash) → review+
(In reply to Julien Wajsberg [:julienw] (PTO until 1/5) from comment #9)
> Comment on attachment 8539435 [details] [review]
> GitHub pull request URL
> 
> r=me with a last nit in test
> 
> please land with green tests !
> 
> Thanks a lot, this is great :)

Thanks a lot for review! Treeherder is green.

Master: https://github.com/mozilla-b2g/gaia/commit/fce979e88240aba9f106c41b299151d3676bed99
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1136633
Depends on: 1153808
You need to log in before you can comment on or make changes to this bug.