1069322 - [tarako] After deleting a message thread the notification is not be cleared and after unlocking the lockscreen notification will disappear.

Status: RESOLVED WONTFIX

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

Description

[Wei Gao (Spreadtrum)]

OS version
---------------------------------------------
FireFoxOS v1.3t

Reproduce steps:
---------------------------------------------
1) Open sms app and keep the interface on "thread-list"
2) Receive sms message, then there is a notification in status bar
3) Delete the thread message.

Expected result:
---------------------------------------------
The notification will be cleared at the same time.

Actual result:
---------------------------------------------
The notification is still there.

Probability:
---------------------------------------------
Always Recurrence

Comment 1

[Wei Gao (Spreadtrum)]

The next issue:

Reproduce steps:
---------------------------------------------
1) Open sms app and keep the interface on "thread-list"
2) Receive sms message, then there is a notification in status bar
3) lock to lockscreen, and then unlock lockscreen.

Expected result:
---------------------------------------------
The notification is still there.

Actual result:
---------------------------------------------
The notification will disappear.

Probability:
---------------------------------------------
Always Recurrence

Comment 2

[Wei Gao (Spreadtrum)]

I think Bug983631 is same with issue 1, but that patch is not suit for v1.3t.

Dear Steve

Could you help to check this issue?
Thanks a great.

Comment 3

[Julien Wajsberg [:julienw]]

Alexandre, do we have the new notification API on Tarako?

Comment 4

[:gerard-majax]

I'm not 100% sure, but I think so, yes.

Comment 5

[Julien Wajsberg [:julienw]]

The 1.3t flag is missing, I can't request a blocker.
Without a blocker flag there is no chance we'll work on this...

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> Alexandre, do we have the new notification API on Tarako?

The new notification API went live in 1.4, we can't use it on the Tarako. I had to specificall modify a couple of uplifts that were using it for this reason.

Comment 7

[Wei Gao (Spreadtrum)]

(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> (In reply to Julien Wajsberg [:julienw] from comment #3)
> > Alexandre, do we have the new notification API on Tarako?
> 
> The new notification API went live in 1.4, we can't use it on the Tarako. I
> had to specificall modify a couple of uplifts that were using it for this
> reason.

Thank Gabriele for your attention.

Could you help to give a hand to handle this issue on v1.3t?
It would be much appreciated.
Thanks a great.

Comment 8

[Julien Wajsberg [:julienw]]

Dear Wei, we definitely can't fix it without the new API, that's why I asked this question to Alexandre.

Now, I think I remember we actually uplifted it very late in the game, but I don't remember if it was for Tarako or another version. I remember Michael did it so I'll NI him for the definite answer.

Comment 9

[Michael Henretty [:mikehenrty][:mhenretty]]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> Dear Wei, we definitely can't fix it without the new API, that's why I asked
> this question to Alexandre.
> 
> Now, I think I remember we actually uplifted it very late in the game, but I
> don't remember if it was for Tarako or another version. I remember Michael
> did it so I'll NI him for the definite answer.

We've had the new Notification API since long before 1.3. Notification.get also landed in 1.3. I think the late uplift you are talking about are a bunch of fixes Alexandre did which included not automatically closing notifications from the new API when opening an app (IIRC). We do not have notification persistence on reboot in 1.3.

Now if you are talking about whether the SMS app uses the new API in 1.3t, it does not. It uses the NotificationHelper which in turn uses the old desktop notifications. So, making SMS use the new notification API in 1.3t is probably no small risk.

Comment 10

[Wei Gao (Spreadtrum)]

(In reply to Michael Henretty [:mhenretty] from comment #9)

Ok, thank Michael for your detailed explanation.

Comment 11

[Wei Gao (Spreadtrum)]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> Dear Wei, we definitely can't fix it without the new API, that's why I asked
> this question to Alexandre.
> 
> Now, I think I remember we actually uplifted it very late in the game, but I
> don't remember if it was for Tarako or another version. I remember Michael
> did it so I'll NI him for the definite answer.

Dear Julien

I have tested the new API locally, I find the new notification API such as "new Notification('title', options)" and "Notification.get()" all can work fine.
The only question is we have to change all the old notification mode "NotificationHelper.send()".
So, could we handle this issue now?
Thanks a great.

Comment 12

[Julien Wajsberg [:julienw]]

Ok, I think I remembered about bug 890440 that was due for v1.4 only but was later uplifted to v1.3 and v1.3t.

Wei, the necessary code for SMS app is in bug 855165. I'd be really afraid to uplift this for such a small issue and I'm strongly against it.

Comment 13

[Wei Gao (Spreadtrum)]

Attachment: sms_notification.patch

Dear Julien

I can't understand your strongly against of using new API, could you tell me your reason or the risk of it?
I have made the patch to use the new Notification API on tarako and tested it locally, it can work fine.

Could you give me some suggestion about it?
Thanks so much.

Comment 14

[Julien Wajsberg [:julienw]]

I have several reasons for this opposition:

0. This bug is not a blocker for users. It doesn't block them from doing anything. They can dismiss the notification themselves if they need to.
1. we had regressions when we made this change, that we fixed over time
2. 1.3t is done (we don't even have a flag for it anymore in Bugzilla). I should not work on it anymore.
3. the code that uses the new API had been made for v1.4, so when you uplift this there is the risk of _more_ regressions. Especially with v1.3t where even more modifications were made.
4. We won't be able to catch and fix these regressions in time.
5. I have other work to do, that I think is more important than this.

Every feature that you add will have regressions. It's a fact.
Every fix that you do can have regressions, even if the fix looks harmless.

That's why I don't want to add more features. I've nothing against the new API by itself, I just don't want to risk issues for our users. Especially I don't see why this bug is so important that we need to fix this issue _now_ and risk regressions. If the bug was a real blocking issue (for example: can't see the messages after 1 week, or something), I'd be more than happy to help you fix it. This bug is not a blicking issue as it has workarounds and the user is not blocked using the phone.

I think the current v1.3t code is consistent by itself. Can we make it better? Of course we can, otherwise we can stop developing right now. Should we make it better _now_? I think we should not. It has significant risk.

I don't want to spend my time reviewing this code and testing on my Tarako.
Please raise your concerns to your Mozilla contact. If someone tells me it's the most important thing I need to work on right now, I'll do it of course.

Comment 15

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Needinfo Vance to see if this bug needs to be prioritized.

Comment 16

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Wei Gao (Spreadtrum) from comment #13)
> Created attachment 8494217 [details] [diff] [review]
> sms_notification.patch
> 
> Dear Julien
> 
> I can't understand your strongly against of using new API, could you tell me
> your reason or the risk of it?
> I have made the patch to use the new Notification API on tarako and tested
> it locally, it can work fine.
> 
> Could you give me some suggestion about it?
> Thanks so much.

Hi Wei Gao -

I agree with Julien, really this issue isn't a serious blocker for 1.3T devices. Also the bug has been fixed since 1.4. I don't see the needs to take any risk here simply for fixing this minor issue.

If you think otherwise, or if there is any other strong reason you must have this one fix, please let me know and we can discuss off-line

Thanks

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8494217 [details] [diff] [review]
sms_notification.patch

Clearing the r? request for now.

Comment 18

[Wei Gao (Spreadtrum)]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #16)
> 
> Hi Wei Gao -
> 
> I agree with Julien, really this issue isn't a serious blocker for 1.3T
> devices. Also the bug has been fixed since 1.4. I don't see the needs to
> take any risk here simply for fixing this minor issue.
> 
> If you think otherwise, or if there is any other strong reason you must have
> this one fix, please let me know and we can discuss off-line
> 
> Thanks

Well, ok, that's the point.
Our leader will discuss your suggestions with PM.
Thanks for your kindly attention.

Comment 19

[Julien Wajsberg [:julienw]]

Let's close it now :)

Thanks !