Julien Wajsberg [:julienw] Assignee
	Description • 11 years ago

+++ This bug was initially created as a clone of Bug #907562 +++

STR:
* receive a SMS while you're not in the thread for this sender. For example you can send a sms to yourself, and quickly go to the thread list view while the message is sending
* tap on the thread to go to the thread
* immediately tap the left arrow to go back to the thread list
=> the unread mark has been removed
(you can wait a long time now if you want, this is not time-related here)
* kill the app
* restart the app
=> the unread mark is back

If you wait before tapping the left arrow, the unread mark is really applied.

The problem happens because we ask to mark the messages as read only when the all the messages in the thread have been displayed. So the bug will happen more for big threads.

Therefore I'm asking leo because I think this has a quite important issue for our users (but I'd be ok by putting this in koi... but we need it in koi for sure).

	Julien Wajsberg [:julienw] Assignee
	Comment 1 • 11 years ago

There are several possible fixes here.

1. we mark as read asap when we load the thread. But that means we'd have 2 "getMessages" call in parallel (for marking as read, we use a special filter to get only the unread messages). So this could be problematic.

2. we extend the getMessages option object to have 2 end callbacks: renderEnd, done. Currently, we only have a "end" callback that gets called only when the whole thread has rendered.

But I also think it could be a good idea to have an API to "mark as read" a whole thread, because that's actually the operation we want to do. We never need to "mark as read" an individual message. If we'd have this API we could call it asap without a problem. So NI Gene to have his advice on this.

	Vicamo Yang [:vicamo][:vyang]
	Comment 2 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #1)
> 1. we mark as read asap when we load the thread. But that means we'd have 2
> "getMessages" call in parallel (for marking as read, we use a special filter
> to get only the unread messages). So this could be problematic.
> 
> But I also think it could be a good idea to have an API to "mark as read" a
> whole thread, because that's actually the operation we want to do. We never
> need to "mark as read" an individual message. If we'd have this API we could
> call it asap without a problem. So NI Gene to have his advice on this.

Have markMessageRead() accept a SmsFilter, then you don't bother getMessages() again.

	Julien Wajsberg [:julienw] Assignee
	Comment 3 • 11 years ago

Yep, works for me too :)

	Gene Lian (I already quit Mozilla)
	Comment 4 • 11 years ago

If we let markMessageRead(...) accepts a SmsFilter do this, you can imagine what's going to happen when the user wants to enter a thread. We have to call the following 2 functions separately:

  - getMessages({filterId: xxx})
  - markMessageRead({filterId: xxx})

The backend of these 2 functions actually do lots of similar things for filtering. Calling them separately still sounds redundant to me. However, it still works for me because I don't like to add a flag or callback for getMessages(...), though. Specifically, the |renderEnd| callback sounds more like a UI concept and the Gecko shouldn't be aware of that.

-----
Anyway, back to comment #0, 

> * immediately tap the left arrow to go back to the thread list
> => the unread mark has been removed

I believe this remove is done by Gaia. However, why do we want to do this? If the thread doesn't completely *display* all the messages, marking the thread as unread still makes sense to me because I didn't actually see/read those messages yet by my eyes! Entering and out the thread quickly doesn't imply I've already read all the messages.

Personally, this bug sounds a no-issue to me. Well, not an leo+ at least. What do you guys think?

	Julien Wajsberg [:julienw] Assignee
	Comment 5 • 11 years ago

Gene, the "renderEnd" part was really for Gaia, not for Gecko, so don't worry :)

About the bug: when a user reads the thread, the unread part is likely the latest messages, that are displayed at the very start.

Another fix comes to my mind: while rendering, we could mark "read" only those messages that we rendered and were unread. We would still need the "renderEnd" part (still in Gaia :) ) to know when to do the "mark as read" operation. That sounds more sensible because we would actually mark "read" only those messages that were actually displayed. Of course, this could mark "read" messages that are displayed not currently visible (because above the top), but this is imho not a big problem.

	Vicamo Yang [:vicamo][:vyang]
	Comment 6 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #5)
> Gene, the "renderEnd" part was really for Gaia, not for Gecko, so don't
> worry :)

Don't quite know what the effort to implement "renderEnd"?  Do you need an extra event or something else for it?

	Gene Lian (I already quit Mozilla)
	Comment 7 • 11 years ago

One interesting to notice: the formal W3 API is proposing a markConversationRead() function, which takes conversationID as the parameter (equivalent to our threadID). Maybe, we should make it that way instead of adding a big filter into markMessageRead especially we always use the filter to input threadID only.

[1] http://messaging.sysapps.org/#widl-MessagingManager-markConversationRead-Promise-DOMString-conversationID-boolean-value

	Julien Wajsberg [:julienw] Assignee
	Comment 8 • 11 years ago

Vicamo> nope, it's purely a gaia thing. Basically, we end our rendering loop at one point, and the result is that we don't call the "end" function in that case. This is a small effort, I just need a few hours to get this ready.

	bhavana bajaj [:bajaj]
	Comment 9 • 11 years ago

Adding qawanted to see how easily and often this is reproducible and also compare the behavior with 1.0.1 before blocking here.

	Sarah Parsons
	Comment 10 • 11 years ago

I was able to reproduce this issue very easily 3/5 attempts on both Leo 1.1 Comm and Unagi 1.0.1 

Unagi 1.0.1

Build ID: 20130828043201
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9c62297d11b0
Gaia: 054cdc27404e2daca91d3065d9783681032b2151
Platform Version: 18.0

Leo 1.1

Build ID: 20130828041203
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/c3b4683d1360
Gaia: 518178ca234c98eb1ca0c0997b9d59faea640a85
Platform Version: 18.1
RIL Version: 01.01.00.019.205 

The "unread" icon reappears after user opens SMS thread, immediately selects "back" to go back to the main SMS screen, kills app process, then reopens SMS app.

	Preeti Raghunath(:Preeti)
	Comment 11 • 11 years ago

Minus since it is common to both 1.1 and 1.0.1.

	Wilfred Mathanaraj [:WDM]
	Comment 12 • 11 years ago

when this is landed in master please re-nominate

	Julien Wajsberg [:julienw] Assignee
	Comment 13 • 11 years ago

When 912943 will be done I think we'll still want to implement a "renderEnd" property in Gaia Messages App' MessageManager (_not_ in the Mobile Message API), because writing to the db while we're reading it seems a bad idea performance-wise.

So I think we can implement this in this bug, and change the current markMessagesRead implementation to use bug 912943 in another bug. What do you think Gene ?

	Gene Lian (I already quit Mozilla)
	Comment 14 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #13)
> When 912943 will be done I think we'll still want to implement a "renderEnd"
> property in Gaia Messages App' MessageManager (_not_ in the Mobile Message
> API), because writing to the db while we're reading it seems a bad idea
> performance-wise.

Don't really get it at comment #5 but I'm totally fine with that since it's not related to the Gecko change. :)

> So I think we can implement this in this bug, and change the current
> markMessagesRead implementation to use bug 912943 in another bug. What do
> you think Gene ?

Sounds good to me. So, adding the 'renderEnd' in the Gaia can solve this issue independently? Or the Gaia still needs the support of markConversationRead()?

	Julien Wajsberg [:julienw] Assignee
	Comment 15 • 11 years ago

This will solve it independently.

	Gene Lian (I already quit Mozilla)
	Comment 16 • 11 years ago

OK, I removed the dependency of bug 912943 and let's do the fix with 'renderEnd' in this bug. Thanks!

	Julien Wajsberg [:julienw] Assignee
	Comment 17 • 11 years ago

Github pull request is https://github.com/mozilla-b2g/gaia/pull/11964

* added a markThreadRead in MessageManager, make the code easier to read and it
  will make it easier to use the new method from Bug 912943.
* added a "done" callback that is always called by getMessages
* we always mark the messages as read so removed unnecessary arguments to this
  method
* made renderMessages take a threadId instead of a filter, this was a leftover
  of our conversion to threadId instead of phone numbers
---
 apps/sms/js/message_manager.js              |   48 ++++++++----
 apps/sms/js/thread_ui.js                    |   76 +++++++++----------
 apps/sms/test/unit/message_manager_test.js  |  107 ++++++++++++++++++++++++++-
 apps/sms/test/unit/mock_message_manager.js  |    4 +-
 apps/sms/test/unit/mock_moz_sms_filter.js   |    1 +
 apps/sms/test/unit/mock_navigatormoz_sms.js |   47 ++++++++++++
 apps/sms/test/unit/thread_ui_test.js        |   17 +++++
 7 files changed, 244 insertions(+), 56 deletions(-)
 create mode 100644 apps/sms/test/unit/mock_moz_sms_filter.js

	Borja Salguero [:borjasalguero]
	Comment 18 • 11 years ago

Comment on attachment 800286 [details] [diff] [review]
patch v1

Review of attachment 800286 [details] [diff] [review]:
-----------------------------------------------------------------

Great work. No comments to add! R+ for sure :). Gracias!

	Julien Wajsberg [:julienw] Assignee
	Comment 19 • 11 years ago

So, this was failing a unit test (in the old sms_test.js test file), so I modified this and will see how it looks like in Travis.

carrying r=borja for this

	Julien Wajsberg [:julienw] Assignee
	Comment 20 • 11 years ago

It's green !

master: 503d9f1847032ca0c62a80c930d749c4e19397b3