Clarify model for Event with attachment

[bmarty]

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
• 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

[github-actions]

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/h9Nm3r

[codecov]

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 26 commits behind head on develop.

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ganfra]

I think this is the opposite, from reading the spec?

body | string | Required: If filename is not set or the value of both properties are identical, this is the filename of the original upload. Otherwise, this is a caption for the image.Changed in v1.10: This property can act as a caption for the image.

filename | string | The original filename of the uploaded file.Added in v1.10

The filename is mandatory but the body is not ^

[bmarty]

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

[ganfra]

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?

[bmarty]

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?

[ganfra]

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()

[bmarty]

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

[pixlwave]

Heads up that this is now available in the SDK: matrix-org/matrix-rust-sdk#4081

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

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[bmarty]

@ganfra PR updated with what is provided in matrix-org/matrix-rust-sdk#4081