compose_box: Use inline syntax on upload for image and audio#2188
compose_box: Use inline syntax on upload for image and audio#2188MritunjayTiwari14 wants to merge 2 commits intozulip:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the compose box’s upload insertion logic to use Zulip’s inline-media Markdown syntax for image/audio uploads, aligning Flutter behavior with recent server/web changes and removing filename captions in rendered media.
Changes:
- Generate
![]()Markdown for image (and audio) uploads in the compose box, including the “Uploading …” placeholder. - Track whether an upload is image/audio so the final replacement uses the correct Markdown syntax.
- Update unit/widget tests to expect the new syntax for images.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/widgets/compose_box.dart | Switch upload placeholder/final insertion to ![]() for image/* and audio/* MIME types. |
| lib/model/compose.dart | Add inlineImageOrAudio helper for generating ![]() syntax. |
| test/widgets/compose_box_test.dart | Update compose-box widget test expectations for image/GIF uploads to ![](). |
| test/model/compose_test.dart | Update model test to cover inlineImageOrAudio. |
Comments suppressed due to low confidence (1)
test/model/compose_test.dart:383
- This test is still named
inlineLinkbut now also asserts behavior ofinlineImageOrAudio, which makes the test intent less clear when it fails. Consider renaming the test (or splitting) so the name matches what it's covering.
test('inlineLink', () {
check(inlineLink('CZO', 'https://chat.zulip.org/')).equals('[CZO](https://chat.zulip.org/)');
check(inlineLink('Uploading file.txt…', '')).equals('[Uploading file.txt…]()');
check(inlineImageOrAudio('IMG_2488.png', '/user_uploads/2/a3/ucEMyjxk90mcNF0y9rmW5XKO/IMG_2488.png'))
.equals('');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
392dcd0 to
934f3c1
Compare
|
From a quick look it looks like this part isn't done:
We should avoid producing this syntax when it'll look broken because the server is too old to support it. (Similarly for audio files; I'm not sure if the feature level threshold is the same.) |
934f3c1 to
32969fe
Compare
32969fe to
7c38f62
Compare
|
Added server-12 TODO's and respective comments with functionality for discarding if the server version is lower than 12, @chrisbobbe PTAL. For why i marked Audio Inline Syntax support from server-12, please see: #mobile > issue mislabled with server-12 than server-11 |
Khader-1
left a comment
There was a problem hiding this comment.
Thanks @MritunjayTiwari14! a few notes bellow. Additionally, I'd add more test cases:
- When the server level is below 467, test that image uploads still use
[]()syntax (not![]()) - Widget tests to cover audio files, not just images
255aef5 to
1a3ec30
Compare
|
Thanks, Khader, for the review. Have pushed some changes. Please take a look whenever you get time. |
There was a problem hiding this comment.
Thanks for the revision @MritunjayTiwari14, code changes LGTM aside from one nit bellow, regarding commits structure, I'd consider squashing both of them into a single commit as I think the first one is overly minimal
Finally, a nit on the commit description:
use Fixes: #[issue number] syntax
compose_box: Use inline syntax on upload for image and audio
Fixes: #2178
Fixes: #2179
Previously, we used standard link text for handling uploaded images
and audio. Zulip Server 12.0 (FL-467) introduced support for the standard
CommonMark syntax for image and audio, we must update the Flutter client to match
and generate the \`![]()\` syntax for these file types.
reference:
https://spec.commonmark.org/0.30/#image-descriptionWith that said, approving, Let's now request your review @rajveermalviya
Thanks for the suggestions 🙂, but since recently we have been using a different "Fixes" format, as mentioned by Greg here. |
|
@rajveermalviya Waiting for your review on this... |
rajveermalviya
left a comment
There was a problem hiding this comment.
Thanks for working on this @MritunjayTiwari14. Comments below.
test/model/compose_test.dart
Outdated
| test('inlineImageOrAudio with FL<467', () { // TODO(server-12): remove this test | ||
| final store = eg.store(account: eg.account(user: eg.selfUser, zulipFeatureLevel: 466), | ||
| initialSnapshot: eg.initialSnapshot(zulipFeatureLevel: 466)); | ||
| check(inlineImageOrAudio('IMG_2488.png', '/user_uploads/2/a3/ucEMyjxk90mcNF0y9rmW5XKO/IMG_2488.png', store: store)) | ||
| .equals('[IMG_2488.png](/user_uploads/2/a3/ucEMyjxk90mcNF0y9rmW5XKO/IMG_2488.png)'); | ||
| check(inlineImageOrAudio('foo_bar.mp3', '/user_uploads/2/a3/ucEMyjxk90mcNF0y9rmW5XKO/foo_bar.mp3', store: store)) | ||
| .equals('[foo_bar.mp3](/user_uploads/2/a3/ucEMyjxk90mcNF0y9rmW5XKO/foo_bar.mp3)'); |
There was a problem hiding this comment.
I don't think it sounds correct to name this function inline when it can generate markdown which will not result in image or audio previews rendered inline.
lib/model/compose.dart
Outdated
| String inlineImageOrAudio(String visibleText, String destination, { | ||
| required PerAccountStore store, | ||
| }) { | ||
| // While the Audio Inline syntax was introduced in server-11(FL-405), | ||
| // it is not recommended for widespread use until 12. Discussion: | ||
| // https://chat.zulip.org/#narrow/channel/48-mobile/topic/issue.20mislabled.20with.20server-12.20than.20server-11/with/2398267 | ||
| // TODO(server-12): simplify and remove comment | ||
| return store.zulipFeatureLevel >= 467 | ||
| ? '' | ||
| : '[$visibleText]($destination)'; |
There was a problem hiding this comment.
By following that thread, I see that inline image links should be generated from feature level 467 (as per this message). But do not see any discussion that indicates inline audio previews should be generated from same feature level. Only relevant indication I see is that #2179 is marked with "server-12" label.
Should both inline image and inline audio link generation handled separately?
There was a problem hiding this comment.
Good question, it might happen that the inline audio syntax was recommended to use from a different feature level. As I too was not able to find the exact FL for that. I have started discussing that on #api documentation > which FL for inline audio syntax.
…e syntax Post FL-467, Zulip recommends using the Inline Markdown syntax (\`\`) for images and audio. We should also add support for that. To generate the correct inline input syntax in compose_box, we have defined a helper function `imageAudioLink`.
Fixes zulip#2178. Fixes zulip#2179. Previously, we used standard link text for handling uploaded images and audio. Zulip Server 12.0 (FL-467) introduced widespread support for the standard CommonMark (inline) syntax for image and audio. We must update the Flutter client to match and generate the \`![]()\` syntax on input for these file types. We used mime type to capture these types of media, and process their input syntax using `imageAudioLink`. note: - Audio player syntax was introduced in server-11 (FL-405), but paired up with image input syntax, it was recommended for widespread use by clients post server-12 (FL-467). For more details on this, See Api Changelog and this: https://chat.zulip.org/#narrow/channel/412-api-documentation/topic/which.20FL.20for.20inline.20audio.20syntax/with/2429954 reference: https://spec.commonmark.org/0.30/#image-description
1a3ec30 to
cc5d483
Compare
|
I have pushed new changes @rajveermalviya, PTAL. Difference made since the last round:
|
Fixes #2178.
Fixes #2179.
Changes
Image Upload
before.webm
after.webm
Audio Upload (No visual changes)
before_audio.webm
after_audio.webm
References