Closed
Bug 872543
Opened 10 years ago
Closed 10 years ago
Long-form data reporting notification sticks around after visiting settings
Categories
(Firefox Health Report Graveyard :: Client: Android, defect, P2)
Tracking
(firefox23 fixed, firefox24 fixed)
RESOLVED
FIXED
Firefox 24
People
(Reporter: rnewman, Assigned: liuche)
References
Details
Attachments
(2 files, 2 obsolete files)
5.40 KB,
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
5.40 KB,
patch
|
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
HTC One, 4.1.something, HTC Sense. Two-finger swiping on the notification expands it, which is awesome. You now see a gear icon and "Choose what I share". Tapping that action takes you to settings. It does not dismiss the notification. Tapping the body of the notification takes you to settings and dismisses the notification. I thing the former should also dismiss the notification. I was quite surprised to see it still there.
Assignee | ||
Comment 2•10 years ago
|
||
Seems like setAutoCancel() doesn't work as expected on the big notification action. This probably needs an explicit clearing of the notification when click "action" is called.
Reporter | ||
Comment 3•10 years ago
|
||
Seems like this'll be a quick fix...
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•10 years ago
|
||
This patch explicitly cancels the notification when the notification Action has been selected. This also uses the correct (white) icon in the BigNotification.
Attachment #752369 -
Flags: review?(rnewman)
Reporter | ||
Comment 5•10 years ago
|
||
Comment on attachment 752369 [details] [diff] [review] Patch: Explicitly cancel notification on preferences launch. Review of attachment 752369 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Another option to consider: always attempt to cancel the notification when the user accesses data reporting preferences. Your call. Please land, bake, then flag for uplift.
Attachment #752369 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 6•10 years ago
|
||
I considered that, but adding a single extra key into the bundle is pretty trivial and a one-time-call, and I don't like the idea of sending a notification cancel every single time a settings pane is rendered. Waiting for inbound to re-open...
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b890e8b931d Will bake on Nightly and if no regressions, will submit an aurora patch.
Target Milestone: --- → Firefox 23
Reporter | ||
Updated•10 years ago
|
Target Milestone: Firefox 23 → Firefox 24
Comment 8•10 years ago
|
||
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/9c7c0b3f8c64 for "java-exception | java.lang.NullPointerExceptionat org.mozilla.gecko.GeckoPreferences.onCreate(GeckoPreferences.java:100)" in robocop like https://tbpl.mozilla.org/php/getParsedLog.php?id=23283474&tree=Mozilla-Inbound
Assignee | ||
Comment 9•10 years ago
|
||
Left out a null check for intent extras! Added that in, waiting on Try tests.
Attachment #752369 -
Attachment is obsolete: true
Attachment #753009 -
Flags: review?(nalexander)
Comment 10•10 years ago
|
||
Comment on attachment 753009 [details] [diff] [review] Patch: Explicitly cancel notification on preferences launch v2 Review of attachment 753009 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/DataReportingNotification.java @@ +56,5 @@ > Bundle fragmentArgs = new Bundle(); > fragmentArgs.putString("resource", "preferences_datareporting"); > prefIntent.putExtra(PreferenceActivity.EXTRA_SHOW_FRAGMENT_ARGUMENTS, fragmentArgs); > } > + prefIntent.putExtra(ALERT_NAME_DATAREPORTING_NOTIFICATION, ALERT_NAME_DATAREPORTING_NOTIFICATION.hashCode()); This is weird: you include the notificationId in the extras, but when you actually delete the notification, you ignore it. Either be consistent and cancel the passed in ID, or change the meaning of the extra to be "if this key is present, cancel the one unique notification ID". I think I prefer the latter, since it avoids having to parse the extras bundle.
Attachment #753009 -
Flags: review?(nalexander) → review-
Assignee | ||
Comment 11•10 years ago
|
||
Changed the value from an id to a boolean, but kept the keyname reuse. I kind of want to create another field like IS_DATAREPORTING_NOTIFICATION, because it'd technically be more correct and descriptive, but it would add yet another datareporting-specific single use field.
Attachment #753009 -
Attachment is obsolete: true
Attachment #753022 -
Flags: review?(nalexander)
Updated•10 years ago
|
Attachment #753022 -
Flags: review?(nalexander) → review+
Assignee | ||
Comment 12•10 years ago
|
||
All green on try with android unit-tests with the null check added. https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2178f76d9a
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ae2178f76d9a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•10 years ago
|
||
This has been baking on Nightly for a couple of weeks without problems. Testing on try server against current aurora, then will nom for aurora uplift.
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 758120 [details] [diff] [review] Aurora patch: Clear notification explicitly on tap [Approval Request Comment] Bug caused by (feature/regressing bug #): Follow-up to bug 833625 User impact if declined: Users of Android 4.1+ may see Android system notification for Firefox data collection persist even after attempting to make selections. Testing completed (on m-c, etc.): This has baked in Nightly since 5/23; passes all tests on try: https://tbpl.mozilla.org/?tree=Try&rev=85898e84bc8b and runs correctly Risk to taking this patch (and alternatives if risky): low - added a single additional conditional check for a new flag added in this patch. String or IDL/UUID changes made by this patch: none
Attachment #758120 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #758120 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 16•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f4cb244c7ac7
status-firefox24:
--- → fixed
Updated•4 years ago
|
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•