Closed Bug 872543 Opened 6 years ago Closed 6 years ago

Long-form data reporting notification sticks around after visiting settings

Categories

(Firefox Health Report Graveyard :: Client: Android, defect, P2)

ARM
Android
defect

Tracking

(firefox23 fixed, firefox24 fixed)

RESOLVED FIXED
Firefox 24
Tracking Status
firefox23 --- fixed
firefox24 --- fixed

People

(Reporter: rnewman, Assigned: liuche)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Duplicate of this bug: 872815
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
Target Milestone: Firefox 23 → Firefox 24
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.
Attachment #752369 - Attachment is obsolete: true
Attachment #753009 - Flags: review?(nalexander)
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 #753009 - Attachment is obsolete: true
Attachment #753022 - Flags: review?(nalexander)
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
https://hg.mozilla.org/mozilla-central/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.