-
-
Notifications
You must be signed in to change notification settings - Fork 251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
File Captions 🏷️ #3406
File Captions 🏷️ #3406
Conversation
5100228
to
4326c7e
Compare
Hi, I just stumbled across the screenshots that already show a draft for the "multiple images" feature. |
Yeah multiple images are not part of the current scope |
eca3dad
to
7f864ce
Compare
97e9024
to
37e3458
Compare
37e3458
to
ec77c5a
Compare
@@ -49,7 +54,36 @@ public IncomingPreviewMessageViewHolder(View itemView, Object payload) { | |||
@Override | |||
public void onBind(@NonNull ChatMessage message) { | |||
super.onBind(message); | |||
|
|||
if(!message.isVoiceMessage()) { | |||
assert viewThemeUtils != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will crash when not null, so I would suggest to implement it more defensive, as in skip if null, applies also for the below uses of assert
where I would advise against in general.
@@ -48,6 +53,38 @@ public OutcomingPreviewMessageViewHolder(View itemView) { | |||
public void onBind(ChatMessage message) { | |||
super.onBind(message); | |||
|
|||
if(!message.isVoiceMessage()) { | |||
assert viewThemeUtils != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
|
||
</LinearLayout> | ||
|
||
</LinearLayout> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add empty line on the bottom
from a visual perspective it be nice if the image and the caption could be combined so it looks more like a single message. With the current layout it looks like a image message and a text message. |
ec77c5a
to
82952dd
Compare
@rapterjet2004 visual change looks great 👍 smallest papercut, please add some padding to filename and date so it matches the caption text's padding from the bubble. 👍👍👍 |
great 👍 i saw some bugs when testing.
fileCaptionTest1.mp4 |
issue-3290-file-caption-1.webmI think I fixed it, but can you double check? @mahibi |
d94e964
to
76651b1
Compare
76651b1
to
f1bf83f
Compare
|
0aa5ada
to
21b7264
Compare
@mahibi that should have fixed it. I was theming the background, before setting it, which is why the bubble wasn't appearing, and I made it so that the caption and background should be unclickable. |
- Added dialog_file_attachment_preview.xml - Added FileAttachmentPreviewFragment.kt - Edited ChatActivity to add captions to uploaded files, also refactored some code - Edited the Outgoing, Incoming, and Generic Preview holders to have a caption - Fixed some bugs with RemoteFileBrowser Signed-off-by: Julius Linus <[email protected]>
5b54dea
to
5e362ae
Compare
Well done @rapterjet2004 👍 |
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/3406-talk.apk |
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! |
🖼️ Screenshots
Current impl
🚧 TODO
🏁 Checklist
/backport to stable-xx.x