Skip to content

Fix the message edit being lost when attaching media#7090

Merged
bmarty merged 1 commit into
element-hq:developfrom
manfrommedan:fix/keep-edit-when-attaching-media
Jul 8, 2026
Merged

Fix the message edit being lost when attaching media#7090
bmarty merged 1 commit into
element-hq:developfrom
manfrommedan:fix/keep-edit-when-attaching-media

Conversation

@manfrommedan

@manfrommedan manfrommedan commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Content

Starting an edit and then attaching media (photo, video, file) used to drop the edit. The attachment is sent as a new message, which is expected since a text message can't be replaced by media, but the edit itself was lost, so sending afterwards posted the leftover text as another new message instead of editing the original.

handlePickedMedia now leaves the composer mode untouched while you're editing, so the edit stays active and the text still edits the original message on the next send. Reply mode keeps being cleared, since the attachment becomes the reply.

Motivation and context

Fixes #7083.

Screenshots / GIFs

No UI change, this only affects composer state handling.

Tests

  • Edit a message, attach a photo, send the photo, then send the text: the original message is edited instead of a new one being created.
  • Unit tests in MessageComposerPresenterTest cover the edit being preserved across a media pick, and a reply still being cleared.

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 16

Checklist

  • I am aware of the etiquette.
  • Changes have been tested on an Android device or Android emulator with API 24
  • 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 defines what will change for the user
  • Pull request includes screenshots or videos if containing UI changes
  • You've made a self review of your PR

@manfrommedan manfrommedan requested a review from a team as a code owner June 23, 2026 21:15
@manfrommedan manfrommedan requested review from bmarty and removed request for a team June 23, 2026 21:15
@github-actions

Copy link
Copy Markdown
Contributor

Thank you for your contribution! Here are a few things to check in the PR to ensure it's reviewed as quickly as possible:

  • If your pull request adds a feature or modifies the UI, this should have an equivalent pull request in the Element X iOS repo unless it only affects an Android-only behaviour or is behind a disabled feature flag, since we need parity in both clients to consider a feature done. It will also need to be approved by our product and design teams before being merged, so it's usually a good idea to discuss the changes in a Github issue first and then start working on them once the approach has been validated.
  • Your branch should be based on origin/develop, at least when it was created.
  • The title of the PR will be used for release notes, so it needs to describe the change visible to the user.
  • The test pass locally running ./gradlew test.
  • The code quality check suite pass locally running ./gradlew runQualityChecks.
  • If you modified anything related to the UI, including previews, you'll have to run the Record screenshots GH action in your forked repo: that will generate compatible new screenshots. However, given Github Actions limitations, it will prevent the CI from running temporarily, until you upload a new commit after that one. To do so, just pull the latest changes and push an empty commit.

@github-actions github-actions Bot added the Z-Community-PR Issue is solved by a community member's PR label Jun 23, 2026

@bmarty bmarty left a comment

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. This look good to me, but I think that it will have to be updated when #6519 will be merged. I am working on this one now, and I'd prefer to merge #6519 first as it is quite big.

@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.56%. Comparing base (6124d99) to head (55f3a1a).
⚠️ Report is 17 commits behind head on develop.

Files with missing lines Patch % Lines
.../libraries/mediapickers/test/FakePickerProvider.kt 33.33% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #7090   +/-   ##
========================================
  Coverage    80.55%   80.56%           
========================================
  Files         2711     2711           
  Lines        78168    78171    +3     
  Branches     10557    10558    +1     
========================================
+ Hits         62969    62979   +10     
+ Misses       11126    11117    -9     
- Partials      4073     4075    +2     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@manfrommedan

Copy link
Copy Markdown
Contributor Author

Sounds good. One problem worth flagging, and the fix is about 2 minutes.

Problem: #6519's new handlePickedMediaList resets composerMode to Normal unconditionally (it only reads the Reply mode), exactly like the single-pick handlePickedMedia this PR guards. So an in-progress edit is still lost when picking from the gallery, i.e. the #7083 behaviour via multi-pick.

Proposal: the same one-line guard, if (!messageComposerContext.composerMode.isEditing). I'll apply it to both methods when I rebase #7090 on top of #6519, or it's trivial to fold straight into #6519 while you're already there.

@bmarty

bmarty commented Jul 7, 2026

Copy link
Copy Markdown
Member

Hello @manfrommedan , now that #6519 has been merged, can you revisit this PR please?

Picking an attachment reset the composer to Normal, which dropped an
in-progress edit: the attachment went out as a new message and the
leftover edit text then created another one instead of editing the
original.

Route every attach path through a single resetComposerModeAfterAttaching()
helper that clears Reply/Normal but keeps an edit pending, so the typed
text still edits the original on the next send. This also covers the
multi-item gallery picker added in element-hq#6519, which reset the mode
unconditionally.

Reply is still cleared (the attachment becomes the reply).

Fixes element-hq#7083.
@manfrommedan manfrommedan force-pushed the fix/keep-edit-when-attaching-media branch from da26247 to 55f3a1a Compare July 8, 2026 12:33
@manfrommedan

Copy link
Copy Markdown
Contributor Author

Rebased on #6519. Attaching media no longer drops an in-progress edit: all paths (SendUri, single and multi picker) now go through one reset that clears reply/normal but keeps the edit. Added tests for both, all green.

@bmarty bmarty left a comment

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!

@bmarty bmarty merged commit d7a2366 into element-hq:develop Jul 8, 2026
27 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Z-Community-PR Issue is solved by a community member's PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

If you need to edit (add a photo) to your previous message

2 participants