-
Notifications
You must be signed in to change notification settings - Fork 155
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.
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
Iteration on caption #3816
Iteration on caption #3816
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Quality Gate passedIssues Measures |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3816 +/- ##
===========================================
- Coverage 83.02% 83.01% -0.01%
===========================================
Files 1772 1772
Lines 44617 44636 +19
Branches 5237 5244 +7
===========================================
+ Hits 37042 37054 +12
- Misses 5745 5746 +1
- Partials 1830 1836 +6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
// We cannot use Uri? type here, as that could trigger a | ||
// NotSerializableException when persisting this to storage | ||
val imageUriString: String?, | ||
private val imageUriString: String?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed for this PR, but since we're no longer persisting these (I think) we might be able to use Uri again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A yes, true!
@@ -316,6 +317,17 @@ class DefaultNotifiableEventResolver @Inject constructor( | |||
} | |||
.getOrNull() | |||
} | |||
|
|||
private suspend fun NotificationContent.MessageLike.RoomMessage.getImageMimetype(): String? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't this be reused in the previous function (fetchImageIfPresent
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, maybe. Also I do not see a better way to avoid the code duplication here. I do not like using Pair
.
Content
This PR is following #3803 .
Motivation and context
Polish the application
Screenshots / GIFs
Tests
Tested devices
Checklist