Integrate Send Tab and Push components #865
Conversation
jonalmeida
force-pushed
the
integrate-push-796
branch
2 times, most recently
from
0af1898 to
12b2ddc
Compare
|
|
||
| // Pick a random ID for this notification so that different tabs do not clash. | ||
| @SuppressWarnings("MagicNumber") | ||
| val notificationId = (Math.random() * 100).toInt() |
There was a problem hiding this comment.
Should we use NotificationIds?
There was a problem hiding this comment.
Wouldn't this give me the same id all the time? I don't have different tags, but I need new IDs generated for each tab sent.
There was a problem hiding this comment.
Ah, yeah, that is tricky. You could still create a different tag based on the URL every time. But that is also wasteful.
Anyhow, using random will just randomly replace other notifications and is not a good choice.
There was a problem hiding this comment.
I took the NotificationManager from Fenix, so I wonder how that's working 🤔 .
I guess I should store the IDs in a map to avoid collisions.
There was a problem hiding this comment.
Since we're using a unique tag for send tab, I added a simple notification ID counter that would always increment up. That should avoid notification ID collisions.
app/src/main/java/org/mozilla/reference/browser/NotificationManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/mozilla/reference/browser/NotificationManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/mozilla/reference/browser/NotificationManager.kt
Outdated
Show resolved
Hide resolved
|
|
||
| // Pick a random ID for this notification so that different tabs do not clash. | ||
| @SuppressWarnings("MagicNumber") | ||
| val notificationId = (Math.random() * 100).toInt() |
There was a problem hiding this comment.
Ah, yeah, that is tricky. You could still create a different tag based on the URL every time. But that is also wasteful.
Anyhow, using random will just randomly replace other notifications and is not a good choice.
jonalmeida
force-pushed
the
integrate-push-796
branch
from
12b2ddc to
46ffb59
Compare
app/src/main/java/org/mozilla/reference/browser/NotificationManager.kt
Outdated
Show resolved
Hide resolved
| /** | ||
| * Manages notification channels and allows displaying different types of notifications. | ||
| */ | ||
| class NotificationManager(private val context: Context) { |
There was a problem hiding this comment.
Since we're creating an NotificationManager, should DataReportingNotification also use this?
There was a problem hiding this comment.
Oh wow, when I rebased I didn't see this. I think it's fair to merge them together.
| } | ||
|
|
||
| // Use an incrementing notification ID since they have the same tag. | ||
| var sendTabNotificationIdCount = 0 |
There was a problem hiding this comment.
Maybe more generic name if we're using this class for all notifications
jonalmeida
force-pushed
the
integrate-push-796
branch
from
46ffb59 to
33c395e
Compare
4 participants
Needs some love from releng to get us the Firebase keys into r-b, but it should work after that. 🎉
Pull Request checklist
Before merging checklist