Refactor notifications #4883
Refactor notifications #4883
Conversation
| // "Post failed" dialog should display in this activity | ||
| draftsAlert.observeInContext(this@MainActivity, true) | ||
| } | ||
|
|
||
| override fun onRequestPermissionsResult(requestCode: Int, permissions: Array<out String>, grantResults: IntArray) { |
There was a problem hiding this comment.
This is the bug fix.
| @@ -75,6 +75,7 @@ class TuskyApplication : Application(), Configuration.Provider { | |||
| NEW_INSTALL_SCHEMA_VERSION | |||
| ) | |||
| if (oldVersion != SCHEMA_VERSION) { | |||
| // TODO SCHEMA_VERSION is outdated / not updated in code | |||
| if (!NotificationHelper.areNotificationsEnabled(context, accountManager)) { | ||
| NotificationHelper.disablePullNotifications(context) | ||
| if (!notificationService.areNotificationsEnabled()) { | ||
| // TODO this is working very wrong |
There was a problem hiding this comment.
That would then be a next step.
There was a problem hiding this comment.
Could you give some more details what the problem is?
There was a problem hiding this comment.
Mostly with the "thinking" behind it (or lack thereof). Earlier I had "worded very wrong" there.
It's "if notifications are disabled then disable them"?
app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationService.kt
Show resolved
Hide resolved
app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationService.kt
Show resolved
Hide resolved
app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationService.kt
Outdated
Show resolved
Hide resolved
|
|
||
| when (notification.type) { | ||
| Notification.Type.FOLLOW, Notification.Type.FOLLOW_REQUEST, Notification.Type.SIGN_UP -> return "@" + notification.account.username | ||
| Notification.Type.MENTION, Notification.Type.FAVOURITE, Notification.Type.REBLOG, Notification.Type.STATUS -> return if (!TextUtils.isEmpty(notification.status!!.spoilerText) && !alwaysOpenSpoiler) { |
There was a problem hiding this comment.
the !! is unnecessary as you checked already for null above
There was a problem hiding this comment.
Well spotted (already removed).
| Log.d(TAG, "Enabled pull checks with " + PeriodicWorkRequest.MIN_PERIODIC_INTERVAL_MILLIS + "ms interval.") | ||
| } | ||
|
|
||
| fun disablePullNotifications(context: Context) { |
There was a problem hiding this comment.
No need for the context parameter, there is already a Context injected
There was a problem hiding this comment.
That is a bit of a question still: Does it matter which context is used (somewhere)?
90% of locations in the old code worked directly or indirectly with application context. But some did and do not.
There was a problem hiding this comment.
It can matter, yes, for example when taking theme attributes from a context, then you need to use the Activity Context as each Activity can have a different theme. But here it shouldn't 🤔
app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationService.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/keylesspalace/tusky/components/systemnotifications/NotificationService.kt
Outdated
Show resolved
Hide resolved
| // "Post failed" dialog should display in this activity | ||
| draftsAlert.observeInContext(this@MainActivity, true) | ||
| } | ||
|
|
||
| override fun onRequestPermissionsResult(requestCode: Int, permissions: Array<out String>, grantResults: IntArray) { |
There was a problem hiding this comment.
Could use the newer ActivityResultContracts api instead. See ViewMediaActivity for an example.
Also fixes #4858.
But apart from that there should be no functional change.
If this is ok I would also move the current push code to the NotificationService in this PR.