Skip to content
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

[Rich text editor] Add ability to insert GIFs from keyboard #8185

Merged
merged 1 commit into from
Mar 2, 2023

Conversation

jonnyandrew
Copy link
Contributor

@jonnyandrew jonnyandrew commented Feb 28, 2023

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Add ability to insert GIFs from keyboard when rich text editor is enabled.

Motivation and context

Closes vector-im/verticals-internal#21

Screenshots / GIFs

gifs.webm

Tests

  • Enable rich text editor in labs
  • Enable Gboard system keyboard
  • Select a GIF from the keyboard
  • Send the GIF to the room
  • Switch to plain text mode
  • Select a GIF from the keyboard
  • Send the GIF to the room

Tested devices

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

Checklist

Closes vector-im/verticals-internal#21
@jonnyandrew jonnyandrew force-pushed the jonny/fix/rich-text-editor-gifs branch from 80e0bdb to f1f4eed Compare February 28, 2023 13:49
@jonnyandrew jonnyandrew requested review from a team and Florian14 and removed request for a team February 28, 2023 14:00
@jonnyandrew jonnyandrew marked this pull request as ready for review February 28, 2023 15:37
@sonarcloud
Copy link

sonarcloud bot commented Feb 28, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one question.

@@ -188,6 +190,16 @@ internal class RichTextComposerLayout @JvmOverloads constructor(
views.plainTextComposerEditText.addTextChangedListener(
TextChangeListener({ callback?.onTextChanged(it) }, { updateTextFieldBorder(isFullScreen) })
)
ViewCompat.setOnReceiveContentListener(
views.richTextComposerEditText,
arrayOf("image/*"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using what was used previously in ComposerEditText?
ViewCompat.getOnReceiveContentMimeTypes(views.richTextComposerEditText) ?: arrayOf("image/*")

Copy link
Contributor Author

@jonnyandrew jonnyandrew Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was a bit confused by this line actually. According to the docs getOnReceiveContentMimeTypes() returns null by default, and otherwise returns any mime types set previously using setOnReceiveContentListener().

So I thought it would be redundant in the rich text editor but didn't want to risk breaking anything by removing it from the old composer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for your investigation.

@@ -188,6 +190,16 @@ internal class RichTextComposerLayout @JvmOverloads constructor(
views.plainTextComposerEditText.addTextChangedListener(
TextChangeListener({ callback?.onTextChanged(it) }, { updateTextFieldBorder(isFullScreen) })
)
ViewCompat.setOnReceiveContentListener(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if we should unregister all of these listeners but I suppose they will be destroyed with the view as they're owned by the view?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was also my understanding that the OnReceiveContentListener was attached to the view.

I had a look at the docs but couldn't see any requirement to unregister the listener. And neither are we unregistering the same type of listener on the existing composer so I think it's safe.

@jonnyandrew jonnyandrew merged commit 7dd15af into develop Mar 2, 2023
@jonnyandrew jonnyandrew deleted the jonny/fix/rich-text-editor-gifs branch March 2, 2023 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants