Closed Bug 947180 Opened 6 years ago Closed 6 years ago

[DSDS][Message] Message notification informs of SIM

Categories

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

x86
macOS
defect

Tracking

(Not tracked)

VERIFIED FIXED
1.4 S2 (28feb)

People

(Reporter: wmathanaraj, Assigned: julienw)

References

Details

(Whiteboard: [ucid:, 1.4, ft:comms])

Attachments

(3 files, 1 obsolete file)

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
Summary: [DSDS] Message notification informs of SIM → [DSDS][Message] Message notification informs of SIM
No longer blocks: 1.4-comms-committed
blocking-b2g: 1.4+ → 1.4?
Priority: P1 → P2
blocking-b2g: 1.4? → 1.4+
Priority: P2 → P1
Flags: in-moztrap?(pyang)
Whiteboard: [needs final text]
features should not block release, remove blocking flag
blocking-b2g: 1.4+ → ---
Whiteboard: [needs final text] → [ucid:, 1.4, ft:comms][needs final text]
ni? ayman for final text
Assignee: nobody → felash
Flags: needinfo?(aymanmaat)
Target Milestone: --- → 1.4 S2 (28feb)
Ayman, I just want to know if the text you've put in the wireframes page 12 is final?
(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 ;)
Flags: needinfo?(aymanmaat)
Thanks, then it's workable!
Whiteboard: [ucid:, 1.4, ft:comms][needs final text] → [ucid:, 1.4, ft:comms]
Attached file Github PR (obsolete) —
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.
Attachment #8378423 - Flags: feedback?(schung)
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!
Attachment #8378423 - Flags: feedback?(schung) → feedback+
Depends on: 975356
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 - +33123456789". This is really not pretty.

Discussing with Ayman, it seems that using "(SIM1) +33123456789" is a possible solution, but we need your solution, because I understand this needs to be consistent accross apps.
Flags: needinfo?(cawang)
as an FYI Carrie '(SIM1) +33123456789' uses exactly the same amount of space as 'SIM1 - +33123456789' 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.
Attached file 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(-)
Attachment #8378423 - Attachment is obsolete: true
Attachment #8381433 - Flags: review?(schung)
(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.
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!
Flags: needinfo?(cawang) → needinfo?(anthony)

(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 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.
Attachment #8381433 - Flags: review?(schung)
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?
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!
Flags: needinfo?(anthony)
(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 on attachment 8381433 [details] [review]
github PR

Looks good to me with the follow up, and please remember the SIM name display changes, thanks!
Attachment #8381433 - Flags: review+
Flags: in-testsuite+
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!
Great, thanks!
(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! :)
master: 03ae87c088dc5ac809bfc687e994f5682aee4dbe
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
will verify once issue fixed
Mmm I think I broke the Service Id switch for retrieving MMS here.
Depends on: 981711
Steve, this seems to be what's not working during the demo today?
do we just need Bug 903291 to land? thanks
Flags: needinfo?(schung)
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.
Flags: needinfo?(schung)
Flags: needinfo?(schung)
It's broken right now and sim name didn't show up on DSDS device :( I'll create a follow up for this regression.
Flags: needinfo?(schung)
Blocks: 984820
Depends on: 985273
No longer blocks: 984820
Depends on: 984820
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.