947180 - [DSDS][Message] Message notification informs of SIM

Status: VERIFIED FIXED

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

Description

[Wilfred Mathanaraj [:WDM]]

As a user when I am notified about a message, I want to be informed on which SIM card the message has been received.

Acceptance Criteria

AC1: I want an easy to understand, on the receiving call screen, to show via which Dual SIM the message is being received

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: FFOS_MessagesAppCommsApp_DSDS_V1.4_20140128_V3.0.pdf

Ref: page 11

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

features should not block release, remove blocking flag

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

ni? ayman for final text

Comment 4

[Julien Wajsberg [:julienw]]

Ayman, I just want to know if the text you've put in the wireframes page 12 is final?

Comment 5

[ayman maat :maat]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Ayman, I just want to know if the text you've put in the wireframes page 12
> is final?

Short answer yes. long answer...

The 'SIM1 -' that prefixes the senders name on frames '1. Notification' and '2. Notifications tray' follows the pattern laid out in the DSDS General Guidelines as produced by Carrie, so this final unless the General Guidelines are altered. (referencing page 17 of wireframe pack '[1.4 DSDS] General guideline v0.2')

The 'SIM1' that prefixes the date in the message thread module in frame '3. message thread' should reflect the string 'SIM1 -' that prefixes the senders name on frames '1. Notification' and '2. Notifications tray'

The 'Preview of Whatever Luzie wrote' is just placeholder text for illustration purposes and should be replaced with a preview of the text contained within the message, which is exactly the same functionality we have at the moment ;)

Comment 6

[Julien Wajsberg [:julienw]]

Thanks, then it's workable!

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: Github PR

WIP

I still need to write some unit tests, I may not have the time tonight.

Still Steve I'd like your early feedback on the changes I made here, as I expect we'll reuse this in other places.

Comment 8

[Steve Chung [:steveck]]

Comment on attachment 8378423 [details] [review]
Github PR

Feedback is given in github and overall looks great to me, especially some refactoring in settings.js!

Comment 9

[Julien Wajsberg [:julienw]]

Carrie, I'm in the process of finishing this, but I see something is not right.

When the sender is known as a contact, we'll have this as a header: "SIM1 - Carrie Wang". But when the sender is unknown, we'll have this: "SIM1 - [phone]". This is really not pretty.

Discussing with Ayman, it seems that using "(SIM1) [phone]" is a possible solution, but we need your solution, because I understand this needs to be consistent accross apps.

Comment 10

[ayman maat :maat]

as an FYI Carrie '(SIM1) [phone]' uses exactly the same amount of space as 'SIM1 - [phone]' but is cleaner and clearer to read. So unless there is an superior solution, or it creates a conflict with another notification format i would advise to encapsulate the 'SIM1' in parentheses across the notifications.

Comment 11

[Julien Wajsberg [:julienw]]

Attachment: github PR

I created a new PR because I removed all promises stuff and this was easier like this.

Now that I actually access the iccId information, it seems that we get a penalty (~150ms), so we may actually want to do the Settings.init part in a setTimeout. But this will be for another bug. I kept a commit for the Promise work in https://github.com/julienw/gaia/commit/f8d95facf9c79b0de88fddb44af6ec0f1d1ce7ab

Depending on what Carrie is answering we'll need to change the locale string, but this should be enough.


This shows a prefix in the message notification when we are in a Dual SIM
scenario.

This also introduces an utility function to find the correct SIM index and to
detect the Dual SIM scenario.
---
 apps/sms/js/activity_handler.js             |  41 ++--
 apps/sms/js/compose.js                      |   2 +-
 apps/sms/js/settings.js                     |  57 ++++-
 apps/sms/locales/sms.en-US.properties       |   4 +
 apps/sms/test/unit/activity_handler_test.js |  72 ++++++-
 apps/sms/test/unit/dialog_test.js           |  11 +-
 apps/sms/test/unit/mock_messages.js         |   2 +
 apps/sms/test/unit/mock_settings.js         |  10 +-
 apps/sms/test/unit/settings_test.js         | 313 +++++++++++++++++++---------
 9 files changed, 380 insertions(+), 132 deletions(-)

Comment 12

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #11)
> Created attachment 8381433 [details] [review]
> github PR

> Now that I actually access the iccId information, it seems that we get a
> penalty (~150ms), so we may actually want to do the Settings.init part in a
> setTimeout. But this will be for another bug. I kept a commit for the
> Promise work 
Keep the Settings.init in setTimeout seems not a safe solution. I would prefer your previous promise patch if you want to init Settings asynchronously. But are you sure that this 150ms penalty is caused by initializing Settings? I thought all the settings fetching stuff should be done asynchronously in the init and it should not block the thread list rendering.

Comment 13

[Carrie Wang [New account: [e-mail]]]

Hi, 
I don't have strong opinion on this issue. Yes, the "- +" is quite confusing in a way, but I think if we want to change it, both Dialer and Message should be changed. Otherwise, it will be a mess in the notification centre (imaging SIM 1 - for missed call, but (SIM 1) for message). Hence, I want to make sure that changing this is not a big effort and we can fix it now. If not, maybe we can keep it for now and do it later in next release. 
ni? Anthony to confirm the Dialer part implementation.

In addition, maybe next time we can raise this kind of "APPs-related" issue earlier when the general guideline is proposed. :)

Thanks!

Comment 14

[Julien Wajsberg [:julienw]]

(In reply to Carrie Wang [:carrie] from comment #13)
> Hi, 
> I don't have strong opinion on this issue. Yes, the "- +" is quite confusing
> in a way, but I think if we want to change it, both Dialer and Message
> should be changed. Otherwise, it will be a mess in the notification centre
> (imaging SIM 1 - for missed call, but (SIM 1) for message). Hence, I want to
> make sure that changing this is not a big effort and we can fix it now. If
> not, maybe we can keep it for now and do it later in next release. 
> ni? Anthony to confirm the Dialer part implementation.

Yep absolutely.

> 
> In addition, maybe next time we can raise this kind of "APPs-related" issue
> earlier when the general guideline is proposed. :)

Yes of course, but it's not always easy to see everything before coding...


(In reply to Steve Chung [:steveck] from comment #12)
> (In reply to Julien Wajsberg [:julienw] from comment #11)
> > Created attachment 8381433 [details] [review]
> > github PR
> 
> > Now that I actually access the iccId information, it seems that we get a
> > penalty (~150ms), so we may actually want to do the Settings.init part in a
> > setTimeout. But this will be for another bug. I kept a commit for the
> > Promise work 

> Keep the Settings.init in setTimeout seems not a safe solution. I would
> prefer your previous promise patch if you want to init Settings
> asynchronously.

Yep, if we do a setTimeout here, I'll use the promise patch, that's why I kept it somewhere ;) But I thought that we want to measure this better in another bug, maybe this could be fixed in Gecko as well.

> But are you sure that this 150ms penalty is caused by
> initializing Settings? I thought all the settings fetching stuff should be
> done asynchronously in the init and it should not block the thread list
> rendering.

The 150ms penalty is just made by accessing the conns in "init"... My guess is that by accessing conns.iccId we're actually querying the hardware, and since this API is synchronous, this is doing this.

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8381433 [details] [review]
github PR

Some comments on github, but overall is good except some test seems missed. Hope that we can land this patch soon.

Comment 16

[Julien Wajsberg [:julienw]]

I pushed the follow-ups on the github PR :) Now we wait for decisions about:

* should we make "SIM1", "SIM2" localizable?
* should we have "SIM1 - sender" or "(SIM1) sender" (or other) as notification title?

Comment 17

[Carrie Wang [New account: [e-mail]]]

OK, let's make it "(SIM1/2) sender" as the notification for message and "(SIM1/2) Missed call" for Dialer.
I'll update it to my Dialer spec as a reference. Thanks!

Comment 18

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> I pushed the follow-ups on the github PR :) Now we wait for decisions about:
> 
> * should we make "SIM1", "SIM2" localizable?
  - Seems they have no conclusion yet, but we could keep in this patch and refactor later(if necessary)
> * should we have "SIM1 - sender" or "(SIM1) sender" (or other) as
> notification title?
  - Based on Ayman's comment, we will use parentheses instead of dash(Carrie will update the wireframe)

So let's move on!

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8381433 [details] [review]
github PR

Looks good to me with the follow up, and please remember the SIM name display changes, thanks!

Comment 20

[Carrie Wang [New account: [e-mail]]]

Attachment: [1.4 DSDS] Dialer v0.4.pdf

Hi, I've attached the revised spec here. Please refer to p.6-7.
This is the Dialer spec, if you guys need more info about how it will look like in Message, you can refer to Message spec which is produced by Ayman. Thanks!

Comment 21

[Julien Wajsberg [:julienw]]

Great, thanks!

Comment 22

[Carrie Wang [New account: [e-mail]]]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> I pushed the follow-ups on the github PR :) Now we wait for decisions about:
> 
> * should we make "SIM1", "SIM2" localizable?
> * should we have "SIM1 - sender" or "(SIM1) sender" (or other) as
> notification title?

Hi guys, 

About localizing SIM 1 and SIM 2, I'm not sure if "SIM" is an universal term or not. 
Hence, I've sent an email to ask people who work on localization and still waiting for their response.
I'll update to you guys once I receive any related info. Thanks! :)

Comment 23

[Julien Wajsberg [:julienw]]

master: 03ae87c088dc5ac809bfc687e994f5682aee4dbe

Comment 24

[Julien Wajsberg [:julienw]]

note to QA: bug 903291 regressed the display of notifications, so you need to test either with the bug backed out or with a proper patch.

Comment 25

[Paul Yang [:pyang] (away)]

will verify once issue fixed

Comment 26

[Paul Yang [:pyang] (away)]

https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-tag=2579&filter-productversion=164
in-moztrap+

Comment 27

[Julien Wajsberg [:julienw]]

Mmm I think I broke the Service Id switch for retrieving MMS here.

Comment 28

[Joe Cheng [:jcheng] (please needinfo)]

Steve, this seems to be what's not working during the demo today?
do we just need Bug 903291 to land? thanks

Comment 29

[Steve Chung [:steveck]]

I'll verify it with Rik today, so keep the flag until we got the answer. It did work before and I'll create a follow up for it if it's broken.

Comment 30

[Steve Chung [:steveck]]

It's broken right now and sim name didn't show up on DSDS device :( I'll create a follow up for this regression.

Comment 31

[Eric Chang [:ericcc] [:echang]]

Works with v1.4 on Fugu.

GAIA_REV=3534b4880653f24529c973ec2f09ef51a3d6392f
GECKO_REV=b6f99179f940f4e0f9f83a4b09f438618c269330
GAIA_BRANCH=mozillaorg/v1.4
GECKO_BRANCH=mozillaorg/v1.4
BUILD_TAG=jenkins-B2G.v1.4.0.fugu-24
BuildID=20140415041552