element-x-android Clarify model for Event with attachment

Content

Clarify model for Event with attachment. They must have a filename: String and they may have a body :String? (caption).

If filename is missing from the SDK model, the body is used instead and in this case the body is not mapped to our model.

Motivation and context

Cleanup following change in #3567 The changes here should reduce the risk of errors.

Screenshots / GIFs

Tests

Should not have any impact.

Tested devices

[ ] Physical
[x] Emulator
OS version(s):

Checklist

[ ] Changes have been tested on an Android device or Android emulator with API 23
[ ] UI change has been tested on both light and dark themes
[ ] Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
[ ] Pull request is based on the develop branch
[ ] Pull request title will be used in the release note, it clearly define what will change for the user
[ ] Pull request includes screenshots or videos if containing UI changes
[ ] Pull request includes a sign off
[ ] You've made a self review of your PR

Oct 01 '24 12:10 bmarty

:iphone: Scan the QR code below to install the build (arm64 only) for this PR. QR code If you can't scan the QR code you can install the build via this link: https://i.diawi.com/h9Nm3r

Oct 01 '24 12:10 github-actions[bot]

Codecov Report

Attention: Patch coverage is 85.22167% with 30 lines in your changes missing coverage. Please review.

Project coverage is 82.63%. Comparing base (5a4e5d0) to head (6ea11fa). Report is 140 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ctories/event/TimelineItemContentMessageFactory.kt	64.00%	4 Missing and 5 partials :warning:
...mediaviewer/impl/local/AndroidLocalMediaFactory.kt	57.14%	6 Missing :warning:
...timeline/components/event/TimelineItemVideoView.kt	50.00%	3 Missing and 1 partial :warning:
...timeline/components/event/TimelineItemImageView.kt	25.00%	3 Missing :warning:
...eatures/messages/impl/actionlist/ActionListView.kt	80.00%	1 Missing :warning:
...meline/components/event/TimelineItemStickerView.kt	50.00%	0 Missing and 1 partial :warning:
...atter/impl/DefaultPinnedMessagesBannerFormatter.kt	87.50%	0 Missing and 1 partial :warning:
...ies/matrix/api/timeline/item/event/EventContent.kt	66.66%	0 Missing and 1 partial :warning:
...ries/matrix/api/timeline/item/event/MessageType.kt	94.73%	0 Missing and 1 partial :warning:
.../libraries/matrix/test/timeline/TimelineFixture.kt	85.71%	1 Missing :warning:
... and 2 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3574      +/-   ##
===========================================
- Coverage    82.65%   82.63%   -0.03%     
===========================================
  Files         1735     1735              
  Lines        41191    41262      +71     
  Branches      4999     5005       +6     
===========================================
+ Hits         34047    34096      +49     
- Misses        5381     5394      +13     
- Partials      1763     1772       +9

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Oct 02 '24 12:10 codecov[bot]

We are just mapping what the SDK is providing to the application here. There is maybe a problem in the SDK then?

Oct 02 '24 15:10 bmarty

We are just mapping what the SDK is providing to the application here. There is maybe a problem in the SDK then?

Mmm AFAICT, the SDK returns a non-nullable body and a nullable filename?

Oct 02 '24 15:10 ganfra

Yes, I have read it too fast, my bad

The mapping I have done in the EventContentMapper is still correct to me, and the application needs a non-nullable filename.

So we always have a filename, which may be from the body and we have an optional body which is actually either the filename, or the caption. Do you see a problem with this approach?

Oct 02 '24 20:10 bmarty

I think we should keep the mapping as they come from sdk, otherwise it brings confusion. I'd instead offer some methods to get the correct value (or computed val), like computedFilename() and computedCaption()

Oct 03 '24 08:10 ganfra

OK, let's wait for the SDK to change, I think this is discussed in #2570.

Oct 03 '24 09:10 bmarty

Heads up that this is now available in the SDK: https://github.com/matrix-org/matrix-rust-sdk/pull/4081

mediaTimelineItem.filename will always be the filename and mediaTimelineItem.caption/formattedCaption will only exist when there's a caption to be rendered.