Skip to content

Fixing grouped notification causing malformed url #4320

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bmarty merged 3 commits into from
Oct 25, 2021
Merged

Fixing grouped notification causing malformed url #4320

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
06b3cc3
filters the unique notification uris from the link handling
ouchadam Oct 22, 2021
325e781
fixing strange ide extract
ouchadam Oct 25, 2021
55c00a0
adding changelog entry
ouchadam Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions vector/src/main/java/im/vector/app/core/extensions/UriExtensions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright (c) 2021 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package im.vector.app.core.extensions

import android.net.Uri

const val IGNORED_SCHEMA = "ignored"

fun Uri.isIgnored() = scheme == IGNORED_SCHEMA

fun createIgnoredUri(path: String): Uri = Uri.parse("$IGNORED_SCHEMA://$path")
25 changes: 13 additions & 12 deletions vector/src/main/java/im/vector/app/features/notifications/NotificationUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import androidx.core.graphics.drawable.IconCompat
import androidx.fragment.app.Fragment
import im.vector.app.BuildConfig
import im.vector.app.R
import im.vector.app.core.extensions.createIgnoredUri
import im.vector.app.core.resources.StringProvider
import im.vector.app.core.services.CallService
import im.vector.app.core.utils.startNotificationChannelSettingsIntent
Expand Down Expand Up @@ -317,7 +318,7 @@ class NotificationUtils @Inject constructor(private val context: Context,
mode = VectorCallActivity.INCOMING_RINGING
).apply {
flags = Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_SINGLE_TOP
data = Uri.parse("foobar://${call.callId}")
data = createIgnoredUri(call.callId)
}
val contentPendingIntent = PendingIntent.getActivity(context, System.currentTimeMillis().toInt(), contentIntent, 0)

Expand Down Expand Up @@ -378,7 +379,7 @@ class NotificationUtils @Inject constructor(private val context: Context,
call = call,
mode = null).apply {
flags = Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_SINGLE_TOP
data = Uri.parse("foobar://$call.callId")

@ouchadam ouchadam Oct 22, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was a small string interop error here $call.callId instead of ${call.callId}, I wonder if it could have been causing some issues with the outgoing call notification 🤔

@bmarty bmarty Oct 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! IDK about an issue fix here. It's OutgoingRingingCallNotification, @ganfra any idea?

@ganfra ganfra Oct 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think it's actually used

data = createIgnoredUri(call.callId)
}
val contentPendingIntent = PendingIntent.getActivity(context, System.currentTimeMillis().toInt(), contentIntent, 0)

Expand Down Expand Up @@ -584,7 +585,7 @@ class NotificationUtils @Inject constructor(private val context: Context,
// Mark room as read
val markRoomReadIntent = Intent(context, NotificationBroadcastReceiver::class.java)
markRoomReadIntent.action = MARK_ROOM_READ_ACTION
markRoomReadIntent.data = Uri.parse("foobar://${roomInfo.roomId}")
markRoomReadIntent.data = createIgnoredUri(roomInfo.roomId)
markRoomReadIntent.putExtra(NotificationBroadcastReceiver.KEY_ROOM_ID, roomInfo.roomId)
val markRoomReadPendingIntent = PendingIntent.getBroadcast(context, System.currentTimeMillis().toInt(), markRoomReadIntent,
PendingIntent.FLAG_UPDATE_CURRENT)
Expand Down Expand Up @@ -652,7 +653,7 @@ class NotificationUtils @Inject constructor(private val context: Context,
// offer to type a quick reject button
val rejectIntent = Intent(context, NotificationBroadcastReceiver::class.java)
rejectIntent.action = REJECT_ACTION
rejectIntent.data = Uri.parse("foobar://$roomId&$matrixId")
rejectIntent.data = createIgnoredUri("$roomId&$matrixId")
rejectIntent.putExtra(NotificationBroadcastReceiver.KEY_ROOM_ID, roomId)
val rejectIntentPendingIntent = PendingIntent.getBroadcast(context, System.currentTimeMillis().toInt(), rejectIntent,
PendingIntent.FLAG_UPDATE_CURRENT)
Expand All @@ -665,7 +666,7 @@ class NotificationUtils @Inject constructor(private val context: Context,
// offer to type a quick accept button
val joinIntent = Intent(context, NotificationBroadcastReceiver::class.java)
joinIntent.action = JOIN_ACTION
joinIntent.data = Uri.parse("foobar://$roomId&$matrixId")
joinIntent.data = createIgnoredUri("$roomId&$matrixId")
joinIntent.putExtra(NotificationBroadcastReceiver.KEY_ROOM_ID, roomId)
val joinIntentPendingIntent = PendingIntent.getBroadcast(context, System.currentTimeMillis().toInt(), joinIntent,
PendingIntent.FLAG_UPDATE_CURRENT)
Expand All @@ -677,7 +678,7 @@ class NotificationUtils @Inject constructor(private val context: Context,
val contentIntent = HomeActivity.newIntent(context, inviteNotificationRoomId = inviteNotifiableEvent.roomId)
contentIntent.flags = Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_SINGLE_TOP
// pending intent get reused by system, this will mess up the extra params, so put unique info to avoid that
contentIntent.data = Uri.parse("foobar://" + inviteNotifiableEvent.eventId)
contentIntent.data = createIgnoredUri(inviteNotifiableEvent.eventId)
setContentIntent(PendingIntent.getActivity(context, 0, contentIntent, 0))

if (inviteNotifiableEvent.noisy) {
Expand Down Expand Up @@ -716,7 +717,7 @@ class NotificationUtils @Inject constructor(private val context: Context,
val contentIntent = HomeActivity.newIntent(context)
contentIntent.flags = Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_SINGLE_TOP
// pending intent get reused by system, this will mess up the extra params, so put unique info to avoid that
contentIntent.data = Uri.parse("foobar://" + simpleNotifiableEvent.eventId)
contentIntent.data = createIgnoredUri(simpleNotifiableEvent.eventId)
setContentIntent(PendingIntent.getActivity(context, 0, contentIntent, 0))

if (simpleNotifiableEvent.noisy) {
Expand All @@ -738,7 +739,7 @@ class NotificationUtils @Inject constructor(private val context: Context,
val roomIntentTap = RoomDetailActivity.newIntent(context, RoomDetailArgs(roomId))
roomIntentTap.action = TAP_TO_VIEW_ACTION
// pending intent get reused by system, this will mess up the extra params, so put unique info to avoid that
roomIntentTap.data = Uri.parse("foobar://openRoom?$roomId")
roomIntentTap.data = createIgnoredUri("openRoom?$roomId")

// Recreate the back stack
return TaskStackBuilder.create(context)
Expand All @@ -750,7 +751,7 @@ class NotificationUtils @Inject constructor(private val context: Context,
private fun buildOpenHomePendingIntentForSummary(): PendingIntent {
val intent = HomeActivity.newIntent(context, clearNotification = true)
intent.flags = Intent.FLAG_ACTIVITY_CLEAR_TOP or Intent.FLAG_ACTIVITY_SINGLE_TOP
intent.data = Uri.parse("foobar://tapSummary")
intent.data = createIgnoredUri("tapSummary")
return PendingIntent.getActivity(context, Random.nextInt(1000), intent, PendingIntent.FLAG_UPDATE_CURRENT)
}

Expand All @@ -766,7 +767,7 @@ class NotificationUtils @Inject constructor(private val context: Context,
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
intent = Intent(context, NotificationBroadcastReceiver::class.java)
intent.action = SMART_REPLY_ACTION
intent.data = Uri.parse("foobar://$roomId")
intent.data = createIgnoredUri(roomId)
intent.putExtra(NotificationBroadcastReceiver.KEY_ROOM_ID, roomId)
return PendingIntent.getBroadcast(context, System.currentTimeMillis().toInt(), intent,
PendingIntent.FLAG_UPDATE_CURRENT)
Expand All @@ -781,7 +782,7 @@ class NotificationUtils @Inject constructor(private val context: Context,

// the action must be unique else the parameters are ignored
quickReplyIntent.action = QUICK_LAUNCH_ACTION
quickReplyIntent.data = Uri.parse("foobar://$roomId")
quickReplyIntent.data = _root_ide_package_.im.vector.app.core.extensions.createIgnoredUri($roomId")

@bmarty bmarty Oct 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange change in this (commented out) code

@ouchadam ouchadam Oct 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 totally missed that, I guess android studio didn't like auto extracting within the commented code

@bmarty bmarty Oct 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably

return PendingIntent.getActivity(context, 0, quickReplyIntent, 0)
}
*/
Expand Down Expand Up @@ -835,7 +836,7 @@ class NotificationUtils @Inject constructor(private val context: Context,
private fun getDismissSummaryPendingIntent(): PendingIntent {
val intent = Intent(context, NotificationBroadcastReceiver::class.java)
intent.action = DISMISS_SUMMARY_ACTION
intent.data = Uri.parse("foobar://deleteSummary")
intent.data = createIgnoredUri("deleteSummary")
return PendingIntent.getBroadcast(context.applicationContext,
0, intent, PendingIntent.FLAG_UPDATE_CURRENT)
}
Expand Down
25 changes: 15 additions & 10 deletions vector/src/main/java/im/vector/app/features/permalink/PermalinkHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import android.content.Context
import android.net.Uri
import im.vector.app.R
import im.vector.app.core.di.ActiveSessionHolder
import im.vector.app.core.extensions.isIgnored
import im.vector.app.core.utils.toast
import im.vector.app.features.navigation.Navigator
import im.vector.app.features.roomdirectory.roompreview.RoomPreviewData
Expand Down Expand Up @@ -53,15 +54,19 @@ class PermalinkHandler @Inject constructor(private val activeSessionHolder: Acti
navigationInterceptor: NavigationInterceptor? = null,
buildTask: Boolean = false
): Boolean {
if (deepLink == null || !isPermalinkSupported(context, deepLink.toString())) {
return false
}
return tryOrNull {
withContext(Dispatchers.Default) {
val permalinkData = PermalinkParser.parse(deepLink)
handlePermalink(permalinkData, deepLink, context, navigationInterceptor, buildTask)
return when {
deepLink == null -> false
deepLink.isIgnored() -> true

@ouchadam ouchadam Oct 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the fix, we're marking ignored deeplinks as handled to avoid attempting to handle them

@bmarty bmarty Oct 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, not your fault, but this is where a doc would help, to understand what returning false or true means...

@ouchadam ouchadam Oct 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to fix in this PR or a separate one, will also use the opportunity to add some tests

@bmarty bmarty Oct 25, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No hurry, can you just fix the commented out code issue and add a changelog file so I can prepare release 1.3.5. Thanks!

!isPermalinkSupported(context, deepLink.toString()) -> false
else -> {
tryOrNull {
withContext(Dispatchers.Default) {
val permalinkData = PermalinkParser.parse(deepLink)
handlePermalink(permalinkData, deepLink, context, navigationInterceptor, buildTask)
}
} ?: false
}
} ?: false
}
}

private suspend fun handlePermalink(
Expand Down Expand Up @@ -115,8 +120,8 @@ class PermalinkHandler @Inject constructor(private val activeSessionHolder: Acti
private fun isPermalinkSupported(context: Context, url: String): Boolean {
return url.startsWith(PermalinkService.MATRIX_TO_URL_BASE) ||
context.resources.getStringArray(R.array.permalink_supported_hosts).any {
url.startsWith(it)
}
url.startsWith(it)
}
}

private suspend fun PermalinkData.RoomLink.getRoomId(): String? {
Expand Down