Baseed on message app WF spec(https://bug919977.bugzilla.mozilla.org/attachment.cgi?id=822328) p.19, we need to implement a proactively informed notification to notify user about the report status. There has some necessary work in gaia to handle system message event and showing the correct notification content.

Hi Gene, this feature will need gecko's help to dispatch a system message while message report return. Do you have gecko bug for message report system message?

Gecko bugs opened.

1.3+ for a committed 1.3 user story

Just a note to inform that we currently have bug 930296 that makes message handlers not working properly.

This is not a 1.3 feature anymore.

Hi Ayman, based on WF v8.0, p.23 and p24, the delivery(or read) notification layout would be: --------------------------------- Message received by just now receiver name --------------------------------- but the original message notification layout is : --------------------------------- sender name just now message content --------------------------------- The position of the name and message is switched in different case, did we design this differentiation on purpose?

Hi Julien, this patch is about the delivery/read success notification and it should works on current device. I know you already worked on activity handler part(seems a lot conflicts whould happen) and some unit tests is not ready yet, any feedback about the implementation is welcome.(BTW I really think we should refactor activity handler in the future...)

Comment on attachment 8379650 [details] [diff] [review] Bug-933133.patch Review of attachment 8379650 [details] [diff] [review]: ----------------------------------------------------------------- About the activity handler: I think your proposed change makes the code quite difficult to read, because we have lots of "if" everywhere. What you tried to do is keep the current structure, with one big function receiving options, and "if" blocks to do different cases. So I propose you to try something different: extract everything that is common to the 3 cases to different methods (wakelock management, notification handling, MMS parsing). The good thing is that some of it is already in different functions, we just need to promote them to actual methods, or use a closure to have global methods. Then you can just call these common methods from your onSmsReceived, onSmsDeliverySuccess, etc functions. I think that in the end everything will be more linear, easier to read, easier to follow, and easier to test. ::: apps/sms/js/activity_handler.js @@ +22,4 @@ > > // We want to register the handler only when we're on the launch path > if (!window.location.hash.length) { > + ['sms-received', 'sms-delivery-success', 'sms-read-success', It's strange that sms-read-success is named like that, although read reports comes only for MMS... @@ +25,5 @@ > + ['sms-received', 'sms-delivery-success', 'sms-read-success', > + 'notification'].forEach(function(key){ > + var handler = Utils.camelCase('on-' + key); > + window.navigator.mozSetMessageHandler(key, this[handler].bind(this)); > + }, this); suggestion: maybe we can put all message handlers in a "messageHandlers" object, with the keys being "sms-received", "sms-delivery-success", etc. ::: apps/sms/locales/sms.en-US.properties @@ +97,5 @@ > +receiver[one] = {{name}} and {{n}} other receiver > +receiver[two] = {{name}} and {{n}} other receivers > +receiver[few] = {{name}} and {{n}} other receivers > +receiver[many] = {{name}} and {{n}} other receivers > +receiver[other] = {{name}} and {{n}} other receivers this looks quite long, not sure this fits in a notification

In this patch, don't we want to update a message report too? I'm fine with leaving this for a future bug too.

Triage: nominating to v1.5?, pending functionality coming from v1.3 version

to backlog

Ni? Jenny for the question in comment 6(the original spec is in comment 0)

Hi Julien, I decompose the huge message receiving into smaller parts in this patch, and the multipule name in notification is replaced with the latest receiver. It's still a WIP because we might need more test in dispatchNotification part, but any quick feedback would be a great help, thanks!

About comment 6: note that the position of text could have a semantic meaning in the future. Currently it does not have a semantic meaning in markup (it's just plain div) but since the first line is considered to be the title, I think we need to keep this in mind and really use a title text for this.

See https://bugzilla.mozilla.org/show_bug.cgi?id=989182 for more information about how notifications should look and replace themselves; so we need to use the tag appropriately.

Comment on attachment 8456855 [details] Link to github As I said before, I'd prefer to have all common code to separate functions, and that you call them directly in onSmsDeliverySuccess, ... . Now, I think it would be a lot easier to do it if more of our asynchronous functions used promises. So in the end, we have 2 choices: * convert the asynchronous functions we use here to promises (for some of them, like the one in contacts, we can still support both callback and promises, so that you don't need to change all the code now), and then chain them correctly directly in onSmsDeliverySuccess and other functions, and in the end get rid of some if-blocks * keep the approach you have here for now. My preference is clearly the first choice, because this would be in the direction of better and more maintenable code. But I admit this is more work too... So up to you :)

I am not convinced we need notification for message report!

Hello all, UX will not be supporting this feature to show notifications for delivery/read report. We are already showing "message delivered" and "message read" icon in thread view if user turns on the feature in settings (the string in setting needs to be modified accordingly though), I don't see the need to send extra notifications. Note that this is how it's done in most texting apps like whatsapp or line, user goes to the app and see if the message has been delivered/read, not waiting for a notification. Let me know if there's any concern, thanks!

(In reply to Wilfred Mathanaraj [:WDM] from comment #19) > I am not convinced we need notification for message report! I agree that this feature is that important comparing to others(that's why it's pending from 1.3 until now). But one important reason that we still implemented it because some partners did request this feature in the past(like bug 1019341). Could you confirm the request from partner is still existed? Except for the partner request, user could still benefit from the notification that they don't have to turn on the message app for the outgoing message status. I would suggest the we still keep the logic and introduce a flag to control the report notification. We can set the flag to "off" if it's annoying in UX POV, and turn on for partner if they really want this feature.

Jenny - what do you think of the flag? I still would like this to happen later than 2.1. We need to be focused on 2.1 on the critical items that need to be done.

(In reply to Wilfred Mathanaraj [:WDM] from comment #22) > Jenny - what do you think of the flag? I still would like this to happen > later than 2.1. We need to be focused on 2.1 on the critical items that need > to be done. Sure, let's use the flag and set it to off, see if we want to revisit the feature in the future. Thanks Steve!

I personally feel that if we don't want the feature we should just not do it instead of committing some code that is pref-ed off and will inevitably break.

not blocking feature, and this is not must-do for 2.1

I think that what we _really_ miss here is error notifications. We don't really care about success notifications. Do we receive system messages for errors?

Bevis, can we receive system messages for delivery errors? This is the number 1 issue I have while dogfooding.

(In reply to Julien Wajsberg [:julienw] from comment #27) > Bevis, can we receive system messages for delivery errors? > This is the number 1 issue I have while dogfooding. Bug 1138264 has been filed to address the support of this feature.

Bug 1138264 just landed, now we can resume work on this ! Wondering how this works if we can just bind sms-failed on a new very simple html document that would just send the notification.

Un-assign myself first and let the product team to decide when we can place this feature in the roadmap.

In this bug we especially want to send a notification when a sms failed sending. I think this is actually a quite important feature we miss here. When we have this we'll be able to remove the full-screen dialog we display in the application itself as well. Alexandre Lissy will be happier.

Clearing product flag for this bug. We won't be addressing this.

Mass closing of Gaia::SMS bugs. End of an era :(

Mass closing of Gaia::SMS bugs. End of an era :(