Supporting images in notifications #4402
Conversation
There was a problem hiding this comment.
One remark, plus:
- You want to merge that PR into
feature/adm/single-transaction-push(and not develop)? - Changelog please :)
BigPictureStyledoes not need to be used (see Render image in notification #1697)?
| private suspend fun TimelineEvent.fetchImageIfPresent(session: Session): Uri? { | ||
| return when { | ||
| root.isEncrypted() && root.mxDecryptionResult == null -> null | ||
| root.getClearType() == EventType.MESSAGE -> downloadAndExportImage(session) |
There was a problem hiding this comment.
I think we should filter on msgtype here, since the attachment is not necessarily an image, be can be anything else. Event.isImageMessage() can help to do that.
It would be great to merge this after
👍 will add (was originally planning to add as part of the feature PR
correct, not needed because we're already using the
|
|
Thanks for the explanations about I still think there is something strange in your branches since I see this on the other PR: |
|
the branches are
merging #4401 will automatically update #4402 to point to the feature branch and when #4402 is merged I'll raise the feature PR against develop with the idea that this larger PR doesn't need a code review as it's the combination of the reviewed sub PRs the main aim is to keep the PR diffs more contained and (hopefully 🤞 ) easier to review I might create a label for |
ouchadam
added
the
Z-Depends-on-PR
|
Ah there are 3 branches, my bad |
- downloads and exports any images whilst resolving the notification event
…ion image, will allow us to continue to show the notifications
…r non image based attachments
ouchadam
force-pushed
the
feature/adm/notification-images
branch
from
73a224e to
1eb544e
Compare
ouchadam
removed
the
Z-Depends-on-PR
Fixes #1697, relies on #4401 refactor
Adds image support to the message notifications
FileProvider Uriin order to allow the notifications to read and display the image