Closed
Bug 855165
Opened 11 years ago
Closed 10 years ago
[fugu][Messages] Synchronize the notification with the actual message state
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect, P2)
Tracking
(b2g18?)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
b2g18 | ? | --- |
People
(Reporter: sync-1, Assigned: gerard-majax)
References
Details
(Whiteboard: [u=commsapps-user c=messaging p=0][comms-triaged][mentor=:julienw])
Attachments
(1 file)
AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.19.037 Firefox os v1.0.1 Mozilla build ID: 20130310070203. +++ This bug was initially created as a clone of Bug #427675 +++ [WuYing][6915] DEFECT DESCRIPTION: If the UI stay in SMS message list, then received a new SMS, read the new SMS, but the notification can't disappear REPRODUCING PROCEDURES: 1.the UI stay in SMS message list 2.received a new SMS 3.read this new SMS 4.in this time, the SMS prompt in the notification cannot disappear.---KO EXPECTED BEHAVIOUR: After have readed new SMS, the prompt icon shoud disapper in notification bar. ASSOCIATE SPECIFICATION: TEST PLAN REFERENCE: TOOLS AND PLATFORMS USED: USER IMPACT: medium REPRODUCING RATE: 5/5 For FT PR, Please list reference mobile's behavior: ++++++++++ end of initial bug #427675 description ++++++++++ CONTACT INFO (Name,Phone number): DEFECT DESCRIPTION: REPRODUCING PROCEDURES: EXPECTED BEHAVIOUR: ASSOCIATE SPECIFICATION: TEST PLAN REFERENCE: TOOLS AND PLATFORMS USED: USER IMPACT: REPRODUCING RATE: For FT PR, Please list reference mobile's behavior:
There are two problems: 1.we close the app sms, and then we receive several messages from other people. Then we open the app 'sms' again.All of the messages without read, but their notification is disappear! I think it's much better to clear a 'sms' notification after a end user read it. 2.The event "handleAppopen" will just been triggered when we start the sms app. Imagine this, we are just in this sms application, and we receive a lot short messages. Then we read all of these new messages. All of the notifications are still display on the toorbar Without trigger the event "handleAppopen" although we have read all of them and they are not new any longer.
Comment 2•11 years ago
|
||
We needed Bug 782211 to properly implement the notification handling. It landed 10 days ago in moz-central. So either we uplift this to b2g18 (and I think it will be challenging, there are lots of parts and follow-ups), either we switch back to at least gecko 22. I suspect we'll need code in both System and Sms applications.
tracking-b2g18:
--- → ?
(In reply to Julien Wajsberg [:julienw] from comment #2) > We needed Bug 782211 to properly implement the notification handling. It > landed 10 days ago in moz-central. > > So either we uplift this to b2g18 (and I think it will be challenging, there > are lots of parts and follow-ups), either we switch back to at least gecko > 22. > > I suspect we'll need code in both System and Sms applications. I totally agree with that maybe we need code both in system and sms application. We need system event which can listen the status wheather a message is read or not. Then do something in notification.js such as close a sms notification.
Comment 4•11 years ago
|
||
Based on Julien's comment 2 this looks like it would actually be too risky to take in v1.1 and might have to wait for v1.next when gecko is updated.
Updated•11 years ago
|
blocking-b2g: --- → koi?
Assignee | ||
Comment 5•11 years ago
|
||
That's a bit sad, from a UX point of view, this is one of the biggest feedback/issue I'm getting from real people testing on Geeksphone Keon.
Comment 6•11 years ago
|
||
See Bug 868816 for yet another work around we might get in v1.1.
Comment 7•11 years ago
|
||
the Problem is still existed in AU_LINUX_GECKO_ICS_STRAWBERRY_V1.01.00.01.019.107 Firefox os v1.0.1 Mozilla build ID:20130514070202 Could we fix it in 1.1?
blocking-b2g: koi? → leo?
Comment 8•11 years ago
|
||
See the previous comments, we won't fix it in 1.1, as we don't have the necessary tools yet. See also Bug 868816 comment 11. I agree this is unfortunate but doing it now would be extremely risky and would require a lot of work.
Updated•11 years ago
|
blocking-b2g: leo? → ---
Comment 9•11 years ago
|
||
Nom'ing for Koi given the riskiness of implementing in Leo.
blocking-b2g: --- → koi?
Comment 10•11 years ago
|
||
For koi we should have clear use cases around the notifications so that we don't miss anything.
Comment 12•11 years ago
|
||
Guys, I was studying about the "navigator.mozNotification" and this API doesn't alow us to remove a notification, only create new ones, right? I agree with the idea of an application control its notifications, like the Android API. What do you think about this approach?
Comment 13•11 years ago
|
||
Caio> it's already done (see bug 782211) but just not available in Firefox OS 1.0.1 and 1.1. It will be available in Firefox OS 1.2 though.
Comment 14•11 years ago
|
||
So, We could put this bug as dependent of bug 782211. This way, the solution would be only in the SMS application. Do you think that it's a good idea if I work on this bug as a branch of bug 782211?
Comment 15•11 years ago
|
||
It's definitely dependent on bug 782211, thanks :) You can work on this of course, but I think the whole notification handling should be reworked then. We need to use a real state object instead of using the image url for example.
Depends on: 782211
Comment 16•11 years ago
|
||
> Julien Wajsberg How could I verify if my source tree has the implementations of bug 782211? I'm have some problems because I'm newbie. After verified It, I think i could start work in this bug.
Comment 17•11 years ago
|
||
You just need a mozilla-central tree, but you'll need to apply https://bug782211.bugzilla.mozilla.org/attachment.cgi?id=670640 in gaia too, because this has not landed yet.
Comment 18•11 years ago
|
||
If do I build my gecko as mozilla-central solve this problem? I was looking into my gaia tree and It doesn't have a dom/src/notification/Notification.cpp
Comment 19•11 years ago
|
||
Yep, most of the notification implementation is in gecko. If you know how to build your FxOS build, then just build it out of master. Eg in your B2G directory: BRANCH=master ./config.sh <device> ./build.sh ./flash.sh gecko see https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Preparing_for_your_first_B2G_build#Building_a_branch
Comment 20•11 years ago
|
||
I'm looking the sms app from Gaia source tree, but I didn't find where the notification is trigged when a sms is recieved. The MessageManager has a listener for _mozMobileMessage('recieved'), that is onMessageReceived This method doesn't trigger a notification. Using the informations of this discussion, I guess that this notification is trigged by system app. Could anyone confirm this?
Comment 21•11 years ago
|
||
not really. have a look to the badly-named activity_handler.js file. Here we register a message handler on "sms-received" that is sent by gecko. You'll see that we also register a message handler for "notification", which is currently used when the notification is clicked or dismissed.
Comment 22•11 years ago
|
||
I have already found the code from notification and I've made some tests to validate my guesses. The SMS app uses the shared/NotificationHelper to trigger a new notification. But in current implementation, there is no logic to remove the notification. Analysing the W3C specification(see http://www.w3.org/TR/notifications/) thre is a close event. It is implemented in "dom/Notification.webidl" and in "dom/src/notofication/Notification.cpp" in mozilla-central. However, looking into "system/js/notifications.js" from Gaia's system app, there is no implementation of a listener for "desktop-notification-close". So, I can work to solve this bug, but don't you think that is better create a new bug report to solve this issue in Gaia's system application and after solve this bug from sms? Because it will be a problem to 3rd party application developers too. My blueprint is make the system app listen to notifications' "desktop-notification-close" event and after implement triggers in SMS application to force notifications' close.
Comment 23•11 years ago
|
||
Please read again my comment 17 ;) one of the patches in bug 782211 has not landed. I'll do it myself later today so that we can move forward.
Comment 24•11 years ago
|
||
> Please read again my comment 17 ;) Sorry, I didn't see your comment. So I will apply it in a branch to study the code and try to move forward. See about the https://bugzilla.mozilla.org/show_bug.cgi?id=892528 I was talking with vingtetun on IRC and he/she said me that there is somebody working in this part of API. Are you in this team?
Comment 25•11 years ago
|
||
I have found an important thing in current Notification API. The method "navigator.mozNotification.createNotification" return the notification object implemented by "/dom/src/notification/DesktopNotification.h" and "/dom/src/notification/DesktopNotification.cpp". The "dom/webidl/DesktopNotification.webidl" is the interface to Gaia layer, but it doesn't have a close method defined. I'm thinking in implement a close in this interface, at least as a proof of concept.
Comment 26•11 years ago
|
||
Actually, to use the new Notification API, you must use "new Notification(...)" instead of the "createNotification" method. see http://notifications.spec.whatwg.org/#api for more information.
Comment 27•11 years ago
|
||
Ok then. In this case, I'll focus on change the notification_helper to use this new API, given that it is according with the W3C specification. This is the best way to follow in my mind. I'll try using this object correctly now. Thank you!
Comment 28•11 years ago
|
||
bug 900279 is the new bug for the gaia patch for the System app.
Assignee: nobody → ticaiolima
Depends on: 900279
Comment 29•11 years ago
|
||
Julien, I'm having problems to instance a new Notification. having a litte view in the Notification, I've found that it is PrefEnabled. So, I've added the line prefs.push(["dom.webnotifications.enabled", true]); in /gaia/build/preferences.js. But with this configuration, I'm getting this return from console: "[JavaScript Error: "TypeError: Notification is not a constructor" {file: "app://sms.gaiamobile.org/shared/js/notification_helper.js" line: 55}]" I'm running this build in emulator. have you already used this object?
Comment 30•11 years ago
|
||
To be fair I don't know much more. Have you tried with "mozNotification" ? Gregor, do you have any insight on how the API currently works ?
Flags: needinfo?(anygregor)
Comment 31•11 years ago
|
||
I'm trying to change the "DesktopNotification" now, but having some compilation problems. Sorry for this doubt, but there is a way of partial build of B2G as the Firefox has with "./mach dom", for instance?
Comment 32•11 years ago
|
||
You can still do the same in the gecko tree (see the "gecko" subdirectory in the B2G directory). Or even use an external gecko tree (see https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Customization_with_the_.userconfig_file). But I really think that modifying DesktopNotification is not a good path, rather we should understand how to use the new Notification API.
Comment 33•11 years ago
|
||
Yes, You are right. I'm doing it to try to understand how the biding between C++ and javascript with the webidl works. My guess about the Notification error is because of PrefEnabled modifier. I was looking in a way to see the preferences of My B2G build and set values for them. I've tried adding prefs.push(['dom.notifications.enabled',true]) in "/gaia/build/preferences.js" Using the gdb, I had success in mark a breakpoint into Notification.cpp. in my build steps, the Notification.webidl is processed. A think that the critical point here is know if the Notification binding is being done or how it's being done.
Comment 34•11 years ago
|
||
(In reply to Caio Lima(:caiolima) from comment #33) > Yes, You are right. I'm doing it to try to understand how the biding between > C++ and javascript with the webidl works. > > My guess about the Notification error is because of PrefEnabled modifier. I > was looking in a way to see the preferences of My B2G build and set values > for them. I've tried adding prefs.push(['dom.notifications.enabled',true]) > in "/gaia/build/preferences.js" be careful, it's "dom.webnotifications.enabled". I filed bug 900960 to enable the new Notification object by default, I don't know why it was disabled. To change a pref the best is probably to create a file 'build/custom-prefs.js' with the following content: user_pref('dom.notifications.enabled', true); Then you need to "make reset-gaia" to flash everything otherwise the new prefs are not used. > > Using the gdb, I had success in mark a breakpoint into Notification.cpp. in > my build steps, the Notification.webidl is processed. A think that the > critical point here is know if the Notification binding is being done or how > it's being done. It's all a little magical, but it depends on that pref being set for sure.
Comment 35•11 years ago
|
||
I have another guess now. The gaia/xul-runner-sdk/idl has the nsIDOM*.idl of all objects that we can use in the gaia's dom. I didn't find the nsIDOMNotification.idl yet. In my opinion, The Notification API doesn't have a interface to Gaia's dom yet. I'll see how could I create new dom objects. What do you think about it?
Comment 36•11 years ago
|
||
The fact is that I know I can access a Notification object inside my Firefox Aurora :) So it should work. I'm building a mozilla-central gecko to try this out.
Comment 37•11 years ago
|
||
We have too many open bugs around this :( mhenretty is starting to work on bug 892528 today or on monday. It should solve the persisting, querying and closing problem. Please sync with mike if you already have some code.
Flags: needinfo?(anygregor)
Comment 38•11 years ago
|
||
Gregor: here we're focussing on the Sms implementation, so no work duplication so far :) but we have a hard time just accessing the API
Depends on: 892528
Comment 39•11 years ago
|
||
Julien, I will wait your report about usage of new Notification API. I'm rebuilding my B2G with mozilla-central either.
Comment 40•11 years ago
|
||
So, it "kind of" works. I mean, something happens when I do new Notification("title", { body: "something" }); But it crashes ;) I tried in the browser only, we should try in an app instead. see http://everlong.org/mozilla/notifications/ for my testcase.
Comment 41•11 years ago
|
||
Sorry for ask, but where did you test this app? Emulator, Simulator or device?
Comment 42•11 years ago
|
||
on the device
Comment 43•11 years ago
|
||
Ok. I've applied your patch from 900960. It's in mozilla-central, right? So, I'll rebuild my gecko and see what happens.
Updated•11 years ago
|
Updated•11 years ago
|
Comment 44•11 years ago
|
||
Guys, I've find the solution and started to work in this bug now. To enable the new Notification API, I've used the |BRANCH=master ./config.sh <device>| and built the new source tree. However, I'm with a new problem. There is a global object created in sms/notification.js called Notification. This object is overriding the Notification API. I've renamed the Notification of sms/notification.js and the new Notification API worked well. But I don't know if this is the right implementation, given this object is used by other parts of sms application. Could anyone suggest another way to solve this(if it is possible)?
Comment 45•11 years ago
|
||
(In reply to Caio Lima(:caiolima) from comment #44) > Guys, I've find the solution and started to work in this bug now. To enable > the new Notification API, I've used the |BRANCH=master ./config.sh <device>| > and built the new source tree. > > However, I'm with a new problem. There is a global object created in > sms/notification.js called Notification. This object is overriding the > Notification API. I've renamed the Notification of sms/notification.js and > the new Notification API worked well. > > But I don't know if this is the right implementation, given this object is > used by other parts of sms application. Could anyone suggest another way to > solve this(if it is possible)? That's fine, can you rename it "Notice"? That seems like it will be sufficiently similar enough.
Updated•11 years ago
|
Summary: [Buri][SMS]If the UI stay in SMS message list, then received a new SMS, read the SMS, but the notification can't disappear → [Messages] If the UI stay in SMS message list, then received a new SMS, read the SMS, but the notification can't disappear
Comment 46•11 years ago
|
||
(In reply to Rick Waldron from comment #45) > That's fine, can you rename it "Notice"? That seems like it will be > sufficiently similar enough. Yes, Of course I can! Since it's a kind of prefactor to this bug fix, I guess It will be better to file a new bug to this, because it's an object used by a lot of other soruces in sms . Im my first impact set, there is over 10 files to be changed. I've already filed a bug, could anyone confirm it? So I can work on this! https://bugzilla.mozilla.org/show_bug.cgi?id=906544
Comment 47•11 years ago
|
||
Caio> Thanks, makes sense to have this in another bug :)
Updated•11 years ago
|
Summary: [Messages] If the UI stay in SMS message list, then received a new SMS, read the SMS, but the notification can't disappear → [Messages] Synchronize the notification with the actual message state
Updated•11 years ago
|
Blocks: b2g-notifications
Comment 49•11 years ago
|
||
Dears, as a FFOS phone user, I really confused by the status of messages, I think it shoudn't keep un-synced in the third official release, what do you think?
blocking-b2g: --- → koi?
Comment 50•11 years ago
|
||
I agree but we need all the dependant bugs before doing anything, that's why I think it will be in 1.3 rather than 1.2, because feature freeze is very soon.
Comment 51•11 years ago
|
||
If we rush to have this in 1.2, we will probably have to fix numerous bugs during the convergence phase, and I'd rather do this quietly during the 1.3 work.
Comment 52•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #51) > If we rush to have this in 1.2, we will probably have to fix numerous bugs > during the convergence phase, and I'd rather do this quietly during the 1.3 > work. Yes. Jack Liu, If you have a look on the dependence tree,you will notice that to solve this bug, we are working to make the new Notification API stable on FFOS. It's not being a trivial work. I agree with you, but as Julien said, It's better to do things right. :)
Comment 53•11 years ago
|
||
I think there is still some time before v1.2 shipment, although it's ideal that we have every thing ready at the very beginning but customer won't wait, so we just do as the Chinese saying -- cross the unfamiliar river carefully by exploring the stones under water.
Comment 54•11 years ago
|
||
Actually, I guess that the bug that It's really depending is https://bugzilla.mozilla.org/show_bug.cgi?id=899574 With this bug solved, I can start work to sync the SMS.
Comment 55•11 years ago
|
||
Let's move forward anyway, and I'm confident we'll take it in 1.2 if this is ready and in a good shape.
Comment 56•11 years ago
|
||
comms-triage: dependency with 2 bugs so waiting for them to be solved. Put ni? to engineering team to review the feasibility of taking it for v1.2 once they are solved.
Flags: needinfo?(felash)
Whiteboard: [u=commsapps-user c=messaging p=0] → [u=commsapps-user c=messaging p=0][comms-triaged]
Comment 57•11 years ago
|
||
I'm only afraid of small problems that will get discovered as we move closer and closer to the release, that's why I'd want to do this only after the branching and have this in 1.3 for sure, and backport to 1.2 if we see everything works ok.
Flags: needinfo?(felash)
Updated•11 years ago
|
blocking-b2g: koi? → 1.3?
Updated•11 years ago
|
Whiteboard: [u=commsapps-user c=messaging p=0][comms-triaged] → [u=commsapps-user c=messaging p=0][comms-triaged][mentor=:julienw]
Comment 58•11 years ago
|
||
Guys, I'm waiting for the bug #899574 be fixed to start work on this. But I gues we could start some discussions about the implementation. I have already have some blueprints: 1. My first idea is to store the notification ids "thread-wise". So, when the thread start to be loaded, we can get the notifications and close them. In this case, how the thread_id is generated? 2. Will we think about the notifications when the system restart?
Comment 59•11 years ago
|
||
(In reply to Caio Lima(:caiolima) from comment #58) > Guys, I'm waiting for the bug #899574 be fixed to start work on this. But I > gues we could start some discussions about the implementation. I have > already have some blueprints: > > 1. My first idea is to store the notification ids "thread-wise". So, when > the thread start to be loaded, we can get the notifications and close them. > In this case, how the thread_id is generated? The thread id is automatically generated by the gecko API. > > 2. Will we think about the notifications when the system restart? Yep I think we'll need to store them in an Indexed DB, but maybe not in this bug, as this is not even implemented in the System app yet. (see why I want to avoid async storage for the drafts feature ? ;) or we could make asyncStorage evolve to be able to use different object stores.) But maybe we'll have to store a mapping table anyway for supporting the case where the SMS app restarts. Or abuse the image url again since we don't have bug 899585 yet ?
Comment 60•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] (in Paris-Web conference thursday and friday) from comment #59) > The thread id is automatically generated by the gecko API. Could you point me where it happens on Messaging APP, please? =) > Yep I think we'll need to store them in an Indexed DB, but maybe not in this > bug, as this is not even implemented in the System app yet. Maybe We can do it on this bug. Why not? > (see why I want to avoid async storage for the drafts feature ? ;) or we > could make asyncStorage evolve to be able to use different object stores.) I didn't get your point here. Do you mind clarify it? > But maybe we'll have to store a mapping table anyway for supporting the case > where the SMS app restarts. Or abuse the image url again since we don't have > bug 899585 yet ? Sorry for this, but I don't understand the meaning of "abuse" in this statement. BTW, I prefer to store a mapping table, as you said. Even we can't do it on device restart, we will support the app restarting, as you said and it's not so difficult to implement. What do you think about the idea of store them as a list by Thread ID?
Comment 61•11 years ago
|
||
Not blocking, but we will definitely take a patch on this bug as soon as it is fixed !
Comment 63•11 years ago
|
||
(In reply to Caio Lima(:caiolima) from comment #60) > (In reply to Julien Wajsberg [:julienw] (in Paris-Web conference thursday > and friday) from comment #59) > > The thread id is automatically generated by the gecko API. > > Could you point me where it happens on Messaging APP, please? =) Well, when we send a message or receive a message, we have the threadId as a property of the message. > > > Yep I think we'll need to store them in an Indexed DB, but maybe not in this > > bug, as this is not even implemented in the System app yet. > > Maybe We can do it on this bug. Why not? I like doing features in small steps. Easier to review, easier to find bugs ! > > > (see why I want to avoid async storage for the drafts feature ? ;) or we > > could make asyncStorage evolve to be able to use different object stores.) > > I didn't get your point here. Do you mind clarify it? The current asyncStorage lib uses one db with one objectstore. Could be useful to make it evolve so that we can specify an alternative objectstore if we want to use it. > > > But maybe we'll have to store a mapping table anyway for supporting the case > > where the SMS app restarts. Or abuse the image url again since we don't have > > bug 899585 yet ? > > Sorry for this, but I don't understand the meaning of "abuse" in this > statement. We use the "image url" to put some parameters in the notification, so that we can retrieve them when the user taps the notification. I say "abuse" because the "image url" is not meant to do this, this is not clean. > BTW, I prefer to store a mapping table, as you said. Even we can't do it on > device restart, we will support the app restarting, as you said and it's not > so difficult to implement. What do you think about the idea of store them as > a list by Thread ID? Problem is that I don't know what we can store.
Comment 64•11 years ago
|
||
So, Now we can start work on that. I'll try to implement the ideas presented on comment 60. Maybe it could be more clear. =)
Comment 65•11 years ago
|
||
Note that you'll need a patch for bug 890440 to be able to do all the necessary code here.
Comment 66•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #65) > Note that you'll need a patch for bug 890440 to be able to do all the > necessary code here. Yes. Based on this, this code should depend on the 890440, right? But in the current state, It's the opposite.
Comment 67•11 years ago
|
||
I don't want to land bug 890440 until we do this (and probably other things in other apps), because this would break some use cases, hence this dependency direction. I mean, we can do all the work here and land here, and still not land bug 890440. But you need the patch in bug 890440 to do the work here correctly ;) Is this clear ?
Comment 68•11 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #67) > I don't want to land bug 890440 until we do this (and probably other things > in other apps), because this would break some use cases, hence this > dependency direction. > > I mean, we can do all the work here and land here, and still not land bug > 890440. But you need the patch in bug 890440 to do the work here correctly ;) > > Is this clear ? Yes. I was with this in mind =)
Updated•11 years ago
|
Summary: [Messages] Synchronize the notification with the actual message state → [fugu][Messages] Synchronize the notification with the actual message state
Comment 69•11 years ago
|
||
Is there anyone handling this issue? I would like to arrange someone to steal it.
Updated•11 years ago
|
Flags: needinfo?(ticaiolima)
Comment 70•11 years ago
|
||
(In reply to styang from comment #69) > Is there anyone handling this issue? I would like to arrange someone to > steal it. I'm sorry, I've forgot to unset it. I'm a little busy, so you can steal it with no problem =D
Flags: needinfo?(ticaiolima)
Updated•11 years ago
|
Assignee: ticaiolima → administration
Updated•11 years ago
|
Assignee: administration → nobody
Updated•11 years ago
|
blocking-b2g: - → fugu+
Flags: needinfo?(tzhuang)
Flags: needinfo?(ehung)
Comment 72•11 years ago
|
||
After discussed with our partner, it will not block fugu.
blocking-b2g: fugu+ → ---
Flags: needinfo?(styang)
Updated•10 years ago
|
Flags: needinfo?(ehung)
Comment 74•10 years ago
|
||
Unfortunately this needs all the work in bug 890440 that is in 1.4 only. Seems both difficult and risky to uplift all this to 1.3. But we should have this in 1.4 (at last!).
Assignee | ||
Comment 76•10 years ago
|
||
Looking at this right now. My plan is: 1. Switch to Notification instead of mozNotification ; 2. Make use of Notification.get() when we enter in a thread ; 3. Close the notification that matches the thread.
Depends on: 951150
Assignee | ||
Comment 77•10 years ago
|
||
This is a first pull request. It lacks tests for checking that notifications are closed upon reading of the thread.
Assignee: nobody → lissyx+mozillians
Status: NEW → ASSIGNED
Comment 78•10 years ago
|
||
Comment on attachment 8359816 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/15307 I left a few comments/questions on the PR. Please r=? me when ready
Comment 79•10 years ago
|
||
Dears, nominate to 1.3 because it confuses end user.
blocking-b2g: - → 1.3?
Comment 80•10 years ago
|
||
(In reply to Jack Liu from comment #79) > Dears, nominate to 1.3 because it confuses end user. We understand that, but we can't hold a release commonly on issues that we've already shipped with, unless there's special reason on why we shipped with this in 1.1, but shouldn't ship with it in 1.3.
blocking-b2g: 1.3? → -
Assignee | ||
Comment 81•10 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #80) > (In reply to Jack Liu from comment #79) > > Dears, nominate to 1.3 because it confuses end user. > > We understand that, but we can't hold a release commonly on issues that > we've already shipped with, unless there's special reason on why we shipped > with this in 1.1, but shouldn't ship with it in 1.3. And this depends on other bugs that are not 1.3+ and that I'm not confident to backport at this time.
Assignee | ||
Comment 82•10 years ago
|
||
We need some help for the UX side. What should be the behavior when we receive multiple SMS for one thread ? The way it is currently implemented, we will only update the notification with the latest SMS we receive.
Flags: needinfo?(aymanmaat)
Comment 83•10 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #82) > We need some help for the UX side. What should be the behavior when we > receive multiple SMS for one thread ? > > The way it is currently implemented, we will only update the notification > with the latest SMS we receive. For the sake of space and clarity of communication (that there is more than one new message associated to a single thread) i would be inclined to trade out the preview message text that is associated to the new message in the notification tray with ‘’n’ unread messages’, where ’n’ is the number of new messages associated to that thread. But i don't think this is a blocker because currently the user is notified in the notification tray that there is a new message (which is the most important thing) and will discover the other new messages in the thread when they get to the thread. Therefore number of new messages associated to a single thread in the notification tray is a nice to have.
Flags: needinfo?(aymanmaat)
Assignee | ||
Comment 84•10 years ago
|
||
Thanks Ayman. So if everyone is okay with this, then I propose that the present bug only does migration to the new API as it currently does, and that we open a follow up for 1.4 that we do nice things with those notifications.
Assignee | ||
Comment 85•10 years ago
|
||
Comment on attachment 8359816 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/15307 Travis is green at https://travis-ci.org/mozilla-b2g/gaia/builds/17001317 This PR converts test to the new API and also adds two test cases: - checking that when we switch to a new thread, the Notification.get() is called with the correct tag to filter - checking that when we switch to a new thread, we call the close() method of the notification object As far as I can say those are the only new tests we need.
Attachment #8359816 -
Flags: review?(waldron.rick)
Comment 86•10 years ago
|
||
Comment on attachment 8359816 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/15307 I deleted my old obsolete comments, but several new comments. Please r? me when ready
Attachment #8359816 -
Flags: review?(waldron.rick) → review-
Assignee | ||
Comment 87•10 years ago
|
||
Comment on attachment 8359816 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/15307 Thanks for the comment. All of your previous remarks should be addressed, I agree the code is nicer this way :)
Attachment #8359816 -
Flags: review- → review?(waldron.rick)
Comment 88•10 years ago
|
||
Comment on attachment 8359816 [details] [review] Link to Github https://github.com/mozilla-b2g/gaia/pull/15307 Just the last two things: 1. jshint fixes 2. Update `MockNotification` => `Notification` and then this is r+ me and you can go ahead and land it—Thanks!!
Attachment #8359816 -
Flags: review?(waldron.rick) → review+
Comment 89•10 years ago
|
||
Hi Jason, Per comment#88, seems it can be fixed in v1.3, sorry but I have to nominate again.
blocking-b2g: - → 1.3?
Comment 90•10 years ago
|
||
Jack: no it can't, see comment 74. Sorry (I mean it).
blocking-b2g: 1.3? → ---
Assignee | ||
Comment 91•10 years ago
|
||
(In reply to Rick Waldron [:rwaldron] from comment #88) > Comment on attachment 8359816 [details] [review] > Link to Github https://github.com/mozilla-b2g/gaia/pull/15307 > > Just the last two things: > > 1. jshint fixes > 2. Update `MockNotification` => `Notification` > > and then this is r+ me and you can go ahead and land it—Thanks!! Done. Travis running at https://travis-ci.org/mozilla-b2g/gaia/builds/17049234
Assignee | ||
Comment 92•10 years ago
|
||
Green travis at https://travis-ci.org/mozilla-b2g/gaia/builds/17049234.
Assignee | ||
Comment 93•10 years ago
|
||
https://github.com/mozilla-b2g/gaia/commit/4efcdad4637d6af1654dcd441409b187a00b81ce
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 94•10 years ago
|
||
This is so good, thanks !
Updated•10 years ago
|
Flags: needinfo?(tzhuang)
You need to log in
before you can comment on or make changes to this bug.
Description
•