If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Ambient indicator should be based on a list of notifications and not on a counter

VERIFIED FIXED in Firefox OS v2.1

Status

Firefox OS
Gaia::System
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: albertopq, Assigned: albertopq)

Tracking

unspecified
2.1 S5 (26sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.1+, b2g-v2.1 verified, b2g-v2.2 verified)

Details

(Whiteboard: [systemsfe])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Right now, the ambient indicator size and visibility depends on a counter of unread notifications. That could lead to cases in which an external application removes a notification and that makes the ambient indicator not showing the actual number of unread notifications.

For fixing that, we should keep a list of unread notifications (instead of a counter), and remove them from the list when they are read.
(Assignee)

Updated

3 years ago
Depends on: 1061854
(Assignee)

Comment 1

3 years ago
As an example, here is a use case mhenretty was pointing at:

"If I get a download notification (unread=1), then read it (unread=0), then I get an email notification (unread=1), and then the system removes the download notification (decExternalNotifications, unread=0), the ambient indicator will go away even though I have an unread email notification. Instead of a count, what we need is a list of unread notificationId's. Then when we add or remove notifications, we also add remove from this list, and we use this list for the unread count."
Blocks: 1042713
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Whiteboard: [systemsfe]
(Assignee)

Updated

3 years ago
Assignee: nobody → apastor
(Assignee)

Comment 2

3 years ago
Created attachment 8488450 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23982
Attachment #8488450 - Flags: review?(mhenretty)
Comment on attachment 8488450 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23982

Sweet! Left a small nit on github. And let's add a test to make sure the scenario in comment 1 is addressed.
Attachment #8488450 - Flags: review?(mhenretty) → review+
(Assignee)

Comment 4

3 years ago
master: https://github.com/mozilla-b2g/gaia/commit/b8885f6a209ade6bb722ac474e382058b52b3ca7
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)

Updated

3 years ago
Depends on: 1073032
(Assignee)

Comment 5

3 years ago
[Blocking Requested - why for this release]: This bug could lead to a bad UX, as the ambient indicator could show an incorrect number of notifications (even missing unread notifications)
blocking-b2g: --- → 2.1?
Broken feature
blocking-b2g: 2.1? → 2.1+
Please add a Gaia v2.1 nomination when you get a chance :)
Flags: needinfo?(apastor)
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8488450 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/23982

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: The ambient indicator could show an incorrect number of notifications (even missing unread notifications)
[Testing completed]: Added unit tests. Bug found and fixed on bug 1073032. We should uplift both.
[Risk to taking this patch] (and alternatives if risky): It has been in master for a while, and the only bug found was fixed. I think the risk is low compared to the bad ux this bug adds to the system.
[String changes made]: -
Attachment #8488450 - Flags: approval-gaia-v2.1?(fabrice)
Flags: needinfo?(apastor)
Attachment #8488450 - Flags: approval-gaia-v2.1?(fabrice) → approval-gaia-v2.1+
v2.1: https://github.com/mozilla-b2g/gaia/commit/b7b59f0fd59d9aaeeedb209945865da893b37f3c
status-b2g-v2.1: affected → fixed
Reverted from v2.1 for Gaia unit test failures.
v2.1: https://github.com/mozilla-b2g/gaia/commit/a00d102abfe8ae15c4fd14771efa2335c6d3b8d9

https://tbpl.mozilla.org/php/getParsedLog.php?id=49158087&tree=Mozilla-Aurora
status-b2g-v2.1: fixed → affected
Flags: needinfo?(apastor)
Keywords: branch-patch-needed
(Assignee)

Comment 11

3 years ago
It was all green on master, so I guess another patch is dependent. I'll take a look. Thanks!
Flags: needinfo?(apastor)
(Assignee)

Comment 12

3 years ago
v2.1: https://github.com/mozilla-b2g/gaia/commit/89a13e9325948488fd4ae777a97f489189a7ed26
status-b2g-v2.1: affected → fixed
Keywords: branch-patch-needed
Please verify on 2.1 branch.  when done, flip status-b2g-v2.1: "verified"
Keywords: verifyme

Comment 14

3 years ago
Verified the issue is fixed on Flame 2.2 and 2.1 
The indicator is based on list of notifications and not on a counter

Device: Flame 2.2 KK Master
BuildID: 20141024040202
Gaia: d893a9b971a0f3ee48e5a57dca516837d92cf52b
Gecko: a5ee2769eb27
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 36.0a1 (2.2 Master)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.1 KK
BuildID: 20141024001204
Gaia: 0f76e0baac733cca56d0140e954c5f446ebc061f
Gecko: 7d78ff7d25b6
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified
Flags: needinfo?(ktucker)
Keywords: verifyme
(Assignee)

Comment 15

3 years ago
This will be in-testsuite+ as soon as https://github.com/mozilla-b2g/gaia/pull/25215 lands
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
(Assignee)

Updated

3 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.