Closed Bug 872543 Opened 6 years ago Closed 6 years ago
Long-form data reporting notification sticks around after visiting settings
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.
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.
Seems like this'll be a quick fix...
Assignee: nobody → liuche
Status: NEW → ASSIGNED
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)
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+
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...
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
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
Left out a null check for intent extras! Added that in, waiting on Try tests.
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-
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 #753022 - Flags: review?(nalexander) → review+
All green on try with android unit-tests with the null check added. https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2178f76d9a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
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?
Attachment #758120 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.