Show file upload progress and placeholder media message in chat. - #6327
Merged
Conversation
mahibi
force-pushed
the
upload-placeholder
branch
from
June 9, 2026 09:41
75e9f18 to
9e857a9
Compare
5 tasks
Contributor
|
APK file: https://github.com/nextcloud/talk-android/actions/runs/27197628252/artifacts/7504764435 |
mahibi
force-pushed
the
upload-placeholder
branch
from
August 7, 2026 09:32
9e857a9 to
43b3d2d
Compare
Collaborator
Author
|
rebased on master and fixed conflicts. Will pick it up whenever time allows |
Contributor
|
APK file: https://github.com/nextcloud/talk-android/actions/runs/31166324927/artifacts/8989488940 |
6 tasks
mahibi
force-pushed
the
upload-placeholder
branch
from
August 8, 2026 23:09
43b3d2d to
361a69c
Compare
Contributor
|
APK file: https://github.com/nextcloud/talk-android/actions/runs/31283341796/artifacts/9029143403 |
mahibi
commented
Aug 12, 2026
| // 3. It is always positive, because getMessagesEqualOrNewerThan expects it to be larger | ||
| // than oldestMessageId | ||
| @Suppress("MagicNumber") | ||
| val placeholderId = (referenceId.hashCode().toLong() and 0x7FFF_FFFFL) |
Collaborator
Author
There was a problem hiding this comment.
uh, this gave me some headace while testing other branches.
I was wondering why enriching notifications always failed for one conversation.
There was always a followup notification with delete=true which removed the notifiaction on server and enriching resulted in 404.
Same symptom as described in #6330
Apparently i tested the current branch a few days ago and my lastReadMessage was set to 1963726147. So all notifications were immediately deleted, of course also when testing other branches.
- This placeholderId must be changed!!
Collaborator
Author
There was a problem hiding this comment.
no offense! Thank you for the contributions ❤️
could have been claude as well as i vibe coded a bit and did not find the time to review yet 🙈
Collaborator
Author
There was a problem hiding this comment.
created #6486 to introduce some gate that rejects to send it to the server if lastReadMessage seems too high.
I might add some more checks for this.
mahibi
force-pushed
the
upload-placeholder
branch
3 times, most recently
from
August 19, 2026 18:17
7f9a0bf to
6144129
Compare
Contributor
|
APK file: https://github.com/nextcloud/talk-android/actions/runs/32286548489/artifacts/9378181088 |
mahibi
force-pushed
the
upload-placeholder
branch
from
August 20, 2026 14:22
6144129 to
e20416a
Compare
mahibi
marked this pull request as ready for review
August 20, 2026 14:23
mahibi
added
3. to review
Contributor
|
APK file: https://github.com/nextcloud/talk-android/actions/runs/32388748534/artifacts/9414583761 |
rapterjet2004
approved these changes
Aug 20, 2026
Signed-off-by: Jens Zalzala <jens@shakingearthdigital.com> # Conflicts: # gradle/verification-keyring.keys
App compiles but the upload progress is buggy Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Bundles the fixes and follow-up polish for the upload-progress/placeholder
feature from the recent merge-conflict cleanup:
Correctness fixes:
- UploadAndShareFilesWorker called shareFile() unconditionally after every
successful upload, even though two other paths already share the file
themselves: ChunkedFileUploader still had a leftover
ShareOperationWorker.shareFile() call, so any chunked upload (files >1MB)
without conversation subfolders posted the attachment twice; and
conversation-subfolder uploads already share via postConversationAttachment,
so the extra call tried to share a path the file was never uploaded to,
failed, and incorrectly marked successful uploads as FAILED.
- uploadUsingConversationSubfolders() sent a freshly generated UUID as the
message's referenceId instead of the placeholder's actual referenceId, so
the server echoed back the wrong id and the temp placeholder could never
be matched against the real incoming message, leaving it stuck forever.
- sendUnsentChatMessages() (resend-on-reconnect) picked up FAILED upload
placeholders and reposted their "{file}" sentinel text as a bogus new
message. Placeholders with a file attachment are now excluded from that
resend path.
- The "upload completed" signal that triggers an immediate message refetch
was commented out, so a successfully uploaded video's placeholder could
spin forever until the chat was closed and reopened.
- Coil's AsyncImage never showed a composable-supplied fallback painter when
passed a pre-built ImageRequest with null data, so previews without a
server URL (e.g. video with no server preview) silently fell back to
Coil's own null-data handling instead of our local first-frame image.
Reliability:
- UploadAndShareFilesWorker now retries transient network failures (socket
resets, timeouts) with backoff and a network-connected constraint instead
of failing immediately, up to a bounded number of attempts.
UI/UX:
- Replaced the linear upload progress bar with a WhatsApp-style circular
spinner overlay (with cancel button) centered on the thumbnail, and fixed
a metadata-layout bug that left a padding gap next to the placeholder.
- Removed the persistent Android notifications duplicating in-chat upload/
compression progress; kept the upload-failed notification.
- Stopped treating a file's name as its caption; only real captions are
shown, matching how sent messages already behave.
- Sized the video upload placeholder to the video's real aspect ratio
(16:9 fallback) instead of collapsing to a small generic icon.
- Added a local-first-frame fallback, cached to disk keyed by referenceId,
for videos whose server preview is unavailable, so they don't show a
generic icon indefinitely.
- The play button overlay now shows on all video messages (not just ones
with a server preview) with a WhatsApp-style semi-transparent dark circle
behind it.
Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…r bug - uploadFile() never called onMessageSent(), so the unread-marker cache could latch onto the sender's own message once the upload's temp placeholder synced to its real (higher) message id - it wasn't excluded the way a regular text send already was. - The real preview replacing the upload placeholder could flash instead of fade: Coil's built-in crossfade doesn't reliably animate from a plain Compose Painter placeholder (as opposed to a Coil-managed Drawable). Replaced with an explicit alpha crossfade so the transition is fully controlled by Compose. - ContentScale.FillWidth left an uneven letterboxing gap whenever the server-reported aspect ratio didn't exactly match the loaded preview's own (confirmed via pixel-level screenshot measurement on a real device) - switched to ContentScale.Crop, and corrected the clip radius to 6dp (bubble's 10dp minus the 4dp inset) so the two corners nest concentrically instead of visibly mismatching. - The media clip shape used a fixed corner pattern regardless of the message's grouping state, unlike the bubble itself - shape() now mirrors the bubble's own per-corner grouping logic (groupedSideTop/groupedSideBottom), with the now-degenerate grouped corner rounded to a small 2dp instead of a jarring hard edge. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Portrait (taller-than-wide) photos/videos filling the full bubble width read as oversized in the chat. Landscape media is already reasonably sized at full width, so only portrait media (aspect ratio < 1) is now capped to 75% width via mediaWidthFraction(), applied consistently to the synced message, the local upload placeholder, and the video-preview placeholder so there's no resize when one becomes the other. This required fixing ChatMessageScaffold's OVERLAY metadata layout mode (used only by media messages) to stop forcing Modifier.fillMaxWidth() on its wrapping Box - that Box's width is what the bubble itself wraps to, so forcing it full-width meant the bubble stayed at full size with empty space beside the now-narrower image instead of shrinking to match. OverlayMetadataBadge's alignment is relative to the Box's own bounds, so it's unaffected either way. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
The attachment preview screen already respects the user's drag-to- reorder choice all the way through to the per-file upload calls, but each file was then enqueued as a fully independent unique WorkManager request (enqueueUniqueWork(fileUri, KEEP, ...)). WorkManager gives no ordering guarantee across independent unique work, so a smaller/faster file queued later could finish its upload+share network calls (and so get a server-assigned message position) before an earlier, larger one - silently reordering the messages in chat regardless of what the user picked in the preview screen. Uploads within the same conversation are now chained under one shared unique-work name via ExistingWorkPolicy.APPEND_OR_REPLACE, so they upload and share strictly in enqueue (i.e. send) order. APPEND_OR_REPLACE rather than APPEND so a cancelled/failed upload starts a fresh chain instead of cascade-failing every file queued behind it. Since the unique-work name is now shared across a conversation's uploads rather than per-file, cancellation switches from cancelUniqueWork(fileUri) (which would now cancel the whole queue) to cancelWorkById() using the upload's own WorkRequest id. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Chat message ordering is "timestamp ASC, id ASC" (ChatMessagesDao), but addUploadPlaceholderMessage's timestamp was truncated to whole seconds - near-guaranteed to tie when several files are sent at once - and the tiebreaker id is a hash of a random referenceId, unrelated to call order. So placeholders for a multi-file send could display in an arbitrary (hash-order) sequence instead of the order they were actually sent in, even before any upload/share completes and regardless of the WorkManager send-order fix already in place. nextPlaceholderTimestampSeconds() tracks the last timestamp handed out and never repeats one, so placeholders created within the same wall-clock second still get strictly increasing values matching call order. Self-corrects once each placeholder is replaced by its server-synced message with the real timestamp. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…oading Chained per-conversation uploads (a prior fix, for send-order correctness) mean a file can take real seconds to actually finish uploading and sharing once its predecessors in the same batch are ahead of it in the queue. By the time its real message syncs in, the server's own timestamp for it can be later than the still-pending, instantly-assigned placeholder timestamps of files queued after it in the same batch - flipping their relative order in chat (the still-pending files would sort before the one that just finished, even though it was sent first). persistChatMessagesAndHandleSystemMessages now looks up the matching local placeholder (if still present) before persisting each incoming real message, and keeps the placeholder's timestamp instead of the server's if it's earlier - preserving the originally-established send order for the rest of the still-in-flight batch. Needed a one-shot (non-Flow) DAO query since the placeholder must be read before upsertChatMessagesAndDeleteTemp overwrites/removes it in the same transaction. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…till uploading" This reverts commit 0ec924cfbcef6034ebc87061ed3154b429e09bc5. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…oading Alternative to the reverted commit 0ec924cfb, which fixed this by overwriting a synced message's server-provided timestamp with its placeholder's - risky since other logic may treat that timestamp as the server's authoritative value (message expiry, etc.). Same root cause: uploads within a conversation are chained sequentially, so a file can take real seconds to actually finish uploading and sharing once its predecessors in the same batch are ahead of it in the queue. By the time its real message syncs in, the server's own timestamp for it can be later than the still-pending, instantly-assigned placeholder timestamps of files queued after it in the same batch, flipping their relative order in the timestamp-sorted list. This time the DB values are left untouched. ChatViewModel now tracks, purely in memory for the lifetime of the screen, the order uploads were started in per referenceId, and reorderKnownSendSequence() corrects just the relative order of messages it has a hint for when building the displayed list - every other message (older history, other users, prior sessions) keeps its exact DB position. Once a whole batch finishes syncing, chaining already guarantees the server's own timestamps land in the right order on their own, so this only matters during the transient partially-synced window. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
… passes
Moved from operating on the mapped List<ChatMessageUi> (returning a
new list) to operating in place on the raw MutableList<ChatMessage>,
the same way applyMessageGrouping()/applySystemMessageGrouping()
already do - all three now run as consistent preprocessing steps
before the single .map { toUiModel(...) } call, instead of the
reorder wrapping that map call separately.
No behavior change, just consistency with the existing pattern.
Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
UploadingMediaMessage's AsyncImage only constrained width, leaving its height to Coil's own intrinsic sizing instead of the already-computed local aspect ratio. MediaMessage (the synced state) and even the sibling UploadingVideoPreview both lock height via Modifier.aspectRatio, so the image case was the odd one out - causing a visible resize once the placeholder was replaced by the final message. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
blur() was applied before padding()/clip() in the modifier chain, so it drew over the whole outer box (including the inset margin) and only got clipped afterwards - letting the blurred edge bleed out to the message bubble's own border. Reordering to padding().clip().blur() confines the blur to the actual clipped image area, matching how the synced MediaMessage's crossfade layers are bounded. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…ight Some incoming file messages never get width/height in their JSON parameters (observed for certain file types). MediaMessage's two crossfade layers both use Modifier.matchParentSize() with no other sized sibling, so without an aspectRatio to size the Box by, it collapses to zero height - hiding the image entirely even though the preview loads successfully (confirmed via Coil logs: MEMORY_CACHE hits for the exact affected fileIds). Fall back to the loaded image's own intrinsic aspect ratio when the server doesn't report one, so the bubble sizes correctly once the preview loads instead of staying permanently collapsed. The server-reported ratio still always takes priority when present, so the normal path is unaffected. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
…t icons Assisted-by: Claude Code:claude-opus-5[1m] Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
The gallery, photo and video tiles in the attachment thumbnail strip used a surfaceVariant background with no explicit icon tint, so the icons fell back to LocalContentColor - onSurface, inherited from the screen's Surface. That is not a matching Material3 pair and left the contrast up to chance. Use the tonal button pairing instead: secondaryContainer for the container and onSecondaryContainer for the icon. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
The screen's previews only covered light and dark mode, so mirrored layout was never visible at design time. Reuse the locale = "ar" form the chat message previews already use. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
…e spec Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
The uploading image placeholder only constrains its height via aspectRatio, which stays absent until the local file's ratio has been read - and reading it copies the whole file out of its content:// uri first, so on a large upload that window covers most of the transfer. With no aspect ratio, the bubble's height came solely from the AsyncImage's intrinsic size, so any moment Coil was not holding a decoded bitmap it collapsed to zero height and the placeholder disappeared from the chat until the real message arrived. Give it the same floor the other media bubbles already have. The uploading video branch falls back to a default aspect ratio and the generic-file branch is a fixed-size icon, so the image branch was the only one without one. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
AndyScherzinger
force-pushed
the
upload-placeholder
branch
from
August 20, 2026 16:53
897ed41 to
347f82d
Compare
AndyScherzinger
approved these changes
Aug 20, 2026
Contributor
|
APK file: https://github.com/nextcloud/talk-android/actions/runs/32394549666/artifacts/9416513478 |
Collaborator
Author
|
Thank you another time for the great initial work @anakin78z 👍 👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolve #2375
copied the PR #6047 from @anakin78z to the nextcloud repo as he won't find the time to continue.
I will
Thank you @anakin78z for this initial code!
🏁 Checklist
/backport to stable-xx.x