Add ReorderableLists to MessageFieldView attachments (#1425)#1517
Add ReorderableLists to MessageFieldView attachments (#1425)#1517IluhinDotPro wants to merge 8 commits intomainfrom
MessageFieldView attachments (#1425)#1517Conversation
FCM |
…ments-reordering-in-messagefieldview-by-long-tap
SleepySquash
left a comment
There was a problem hiding this comment.
I've left some notes. Take a look, please!
| MapEntry<GlobalKey<State<StatefulWidget>>, Attachment> entry, | ||
| ) { | ||
| return !_isImageOrVideo(entry); | ||
| } |
There was a problem hiding this comment.
Perhaps a private extension on MapEntry<GlobalKey<State<StatefulWidget>>, Attachment> may be a cleaner thing? Let's try to stick with an extension, please?
| width: | ||
| (attachmentSize + 4) * | ||
| imgVideosAttachments.length, | ||
| child: ReorderableList( |
There was a problem hiding this comment.
Also, how hard would it be to make the list auto scroll itself when reordering is out of viewport?
Screen.Recording.2025-11-24.at.10.12.26.mp4
It would be great to scroll the list horizontally when reordered item gets out of screen. How hard is it? Can you please do a quick look first in order to determine whether this would be a huge refactor, or a small refactor?
There was a problem hiding this comment.
I think the refactor will be a bit complex; some additional logic will be needed to change the sorting within a single attachment list (instead of two, as is currently the case). In that case, we'll get auto-scrolling out of the box.
|
@IluhinDotPro, and please, take a look at your FCM: Take a look at the example FCM, please. All references must be inside the brackets. And PR first. |
SleepySquash
left a comment
There was a problem hiding this comment.
You shouldn't hurry, please. Before sending PR for a review, do a self-check. It's better to take an hour or more just to be extra sure that you're doing what you intended to do.
…1425) - format code and comments with common style
SleepySquash
left a comment
There was a problem hiding this comment.
Few questions - take a look, please
There was a problem hiding this comment.
If that extension belongs to a domain, then why not integrate it directly into /domain/model/attachment.dart file?
There was a problem hiding this comment.
I don't think it's a good idea to add extensions to model files, as they might be replaced with automatic generation later, and we'll lose the code.
There was a problem hiding this comment.
as they might be replaced with automatic generation later
This isn't happening now, and I don't think will happen at all.
Why do you suggest writing an extension at all, if you can integrate those methods directly into the model this way?
…sagefieldview-by-long-tap
…-implement-attachments-reordering-in-messagefieldview-by-long-tap
SleepySquash
left a comment
There was a problem hiding this comment.
Excuse me, have you seen the previous comments I've left in a review last week? #1517 (review)
All the comments are marked as "resolved", and this PR has been sent for a the review, however it seems that you neither did anything told in the comments nor wrote any arguments in those discussions as to why you don't agree with all of this.
Am I missing something? Or why did you resolve all the comments without doing or telling anything?
Resolves #1425
Synopsis
Attachmentsattached cannot be sorted. Also their order is not the order thatChatMessagewill display those attachments in.Solution
Split
attachmentsby to list (image/video and other files).Added ReorderableList to both attachments lists
Modifed
_addAttachmentinMessageFieldControllerfor nore correctly add newAttachmenttoattachments(images/video first, files than)Checklist
k::labels applied