[NEW] Add support for initial Rich Messaging #1557

Shailesh351 · 2018-07-30T06:41:02Z

@RocketChat/android

This PR adds support for initail Rich Messaging (ButtonActions as Attachment)

Test it with SDK : RocketChat/Rocket.Chat.Kotlin.SDK#203

And follow this to test it: https://github.com/WideChat/Rocket.Chat.Android/wiki/(WIP)--Rich-Messaging-Acceptance-Test

Spec: https://github.com/WideChat/Rocket.Chat.Android/wiki/Buttons-Inside-Attachment-Object-Spec

Demo : https://photos.app.goo.gl/okir1oP776cAcZ279

CLAassistant · 2018-07-30T06:41:09Z

All committers have signed the CLA.

rafaelks · 2018-07-31T14:11:59Z

@Shailesh351 Could you please sign the CLA so we can review this PR?

rafaelks · 2018-08-07T19:59:55Z

@Shailesh351 Do you mind resolving the conflicts here please?

philipbrito

Avoid using !! in the code. It is a true source of bugs.

philipbrito · 2018-08-07T20:32:53Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ActionsAttachmentViewHolder.kt

+
+    init {
+        with(itemView) {
+            setupActionMenu(actions_attachment_container)


with is useless here since there is only one block being used. Could be itemView.setupActionMenu(actions_attachment_container) instead.

I don't see any issue using with even if it is just for one statement...

You can use it anywhere, anytime, but again: When there is only one block being used it is useless. What about spread with with only one block being used on our entire codebase? It will never been an issue but for sure it will be useless, helping only to increase the code lines.

Btw, do you see any advantage declaring:

init { with(itemView) { setupActionMenu(actions_attachment_container) } }

instead of:

init { itemView.setupActionMenu(actions_attachment_container) }

?

@filipedelimabrito Unable to access setupActionMenu method without with(itemView)

itemView.setupActionMenu(actions_attachment_container) works.

philipbrito · 2018-08-07T20:34:31Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ActionsAttachmentViewHolder.kt

+
+            //TODO
+            if (action.imageUrl != null) {
+                button.visibility = View.GONE


Replace with button.isVisible = false

philipbrito · 2018-08-07T20:34:52Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ActionsAttachmentViewHolder.kt

+            //TODO
+            if (action.imageUrl != null) {
+                button.visibility = View.GONE
+                image.visibility = View.VISIBLE


Replace with image.isVisible = true

philipbrito · 2018-08-07T20:35:23Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ActionsAttachmentViewHolder.kt

+                image.controller = controller
+
+            } else if (action.text != null) {
+                button.visibility = View.VISIBLE


Replace with button.isVisible = true

philipbrito · 2018-08-07T20:35:39Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ActionsAttachmentViewHolder.kt

+
+            } else if (action.text != null) {
+                button.visibility = View.VISIBLE
+                image.visibility = View.GONE


Replace with image.isVisible = false

philipbrito · 2018-08-07T20:37:35Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ChatRoomAdapter.kt

+            if (temp.url != null && temp.isWebView != null) {
+                if (temp.isWebView!!) {
+                    //Open in a configurable sizable webview
+                    Toast.makeText(context, "Open in a configurable sizable webview", Toast.LENGTH_SHORT).show()


Hard coded string. Please, remove and use the string from the string.xml files.

philipbrito · 2018-08-07T20:37:59Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ChatRoomAdapter.kt

+                    }
+                } else {
+                    //Send to bot but not in chat window
+                    Toast.makeText(context, "Send to bot but not in chat window", Toast.LENGTH_SHORT).show()


Hard coded string. Please, remove and use the string from the string.xml files.

luciofm · 2018-08-07T20:50:34Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ActionsAttachmentViewHolder.kt

+
+    init {
+        with(itemView) {
+            setupActionMenu(actions_attachment_container)


I don't see any issue using with even if it is just for one statement...

luciofm · 2018-08-07T21:23:21Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ActionsAttachmentViewHolder.kt

+    fun onActionClicked(action: Action)
+}
+
+class ActionsListAdapter(actions: List<Action>, var actionAttachmentOnClickListener: ActionAttachmentOnClickListener) : RecyclerView.Adapter<ActionsListAdapter.ViewHolder>() {


I think I'd prefer using a new package and multiple files instead of multiple classes and inner classes on the same file

luciofm · 2018-08-07T21:24:56Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ActionsAttachmentViewHolder.kt

+        }
+
+        fun bindAction(action: Action) {
+            TimberLogger.debug("action : $action")


Don't use 'TimberLogger' directly, use Timber.d() or other Timber variants.

luciofm · 2018-08-07T21:26:58Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ActionsAttachmentViewHolder.kt

+    inner class ViewHolder(var layout: View) : RecyclerView.ViewHolder(layout) {
+        lateinit var action: ButtonAction
+
+        var button: MaterialButton = layout.findViewById(R.id.action_button)


No need for findView, can use synthetic accessors from kotlin extensions

luciofm · 2018-08-07T21:30:57Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ChatRoomAdapter.kt

+                } else {
+                    //Open in chrome custom tab
+                    with(this) {
+                        val customTabsBuilder = CustomTabsIntent.Builder()


we have a View.openTabbedUrl() extension function on chat.rocket.android.util.extensions.View.kt

Shailesh351 · 2018-08-23T07:52:34Z

@luciofm @filipedelimabrito I have updated PR with requested changes. Please have a look.

Thank you

philipbrito · 2018-08-23T12:02:07Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ActionsListAdapter.kt

+        }
+
+        init {
+            with(itemView) {


This is a good usage of with with more than one block code being used. Less than that is useless.

👍

philipbrito · 2018-08-23T12:12:58Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ActionsAttachmentViewHolder.kt

+        with(itemView) {
+            title.text = data.title ?: ""
+            actions_list.layoutManager = LinearLayoutManager(itemView.context,
+                    when (alignment) {


You can resume the condition in this way:

if (alignment == "horizontal") { LinearLayoutManager.HORIZONTAL } else { LinearLayoutManager.VERTICAL }

or, if you prefer to use when:

when (alignment) { "horizontal" -> LinearLayoutManager.HORIZONTAL else -> LinearLayoutManager.VERTICAL }

Not needed to repeat LinearLayoutManager.VERTICAL two times.

Merge branch 'temp-button-action' into upstrem-button-action

luciofm

@Shailesh351 the PR is not compiling... And please sync with develop

luciofm · 2018-08-23T15:18:51Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ChatRoomAdapter.kt

+        override fun onActionClicked(view: View, action: Action) {
+            val temp = action as ButtonAction
+            if (temp.url != null && temp.isWebView != null) {
+                if (temp.isWebView!!) {


if you use temp.isWebView == true you don't need to override nullability (!!)

luciofm · 2018-08-23T15:19:03Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ChatRoomAdapter.kt

+                    view.openTabbedUrl(Uri.parse(temp.url))
+                }
+            } else if (temp.message != null && temp.isMessageInChatWindow != null) {
+                if (temp.isMessageInChatWindow!!) {


luciofm · 2018-08-23T15:20:16Z

app/src/main/java/chat/rocket/android/chatroom/adapter/ChatRoomAdapter.kt

+            } else if (temp.message != null && temp.isMessageInChatWindow != null) {
+                if (temp.isMessageInChatWindow!!) {
+                    //Send to chat window
+                    temp.message.run {


use temp.message?.let { message -> that way you don't need to override nullability on temp.message, just use message

luciofm · 2018-08-23T15:20:57Z

app/src/main/java/chat/rocket/android/chatroom/uimodel/ActionsAttachmentUiModel.kt

+import chat.rocket.core.model.attachment.actions.ActionsAttachment
+
+data class ActionsAttachmentUiModel(
+        override val attachmentUrl: String,


use 4 spaces indentation here, please.

luciofm · 2018-08-23T22:00:54Z

app/src/main/java/chat/rocket/android/chatroom/ui/ChatRoomFragment.kt

@@ -339,6 +339,14 @@ class ChatRoomFragment : Fragment(), ChatRoomView, EmojiKeyboardListener, EmojiR
            }

            if (recycler_view.adapter == null) {
+                adapter = ChatRoomAdapter(


The adapter is already initialized on onCreate don't create it twice. Also, this is not compiling...

luciofm

Oops, was meant to request changes...

the PR is not compiling... And please sync with develop

Shailesh351 · 2018-08-24T09:46:50Z

@luciofm Made requested changes and I'm able to successfully compile the PR now. Still latest develop branch is not building successfully so haven't merged it yet.

luciofm · 2018-08-24T11:35:06Z

@Shailesh351 you need to use Android Studio 3.3.0-alpha5 or 3.2.0-beta5, don't use 3.3.0-alpha6 because it has issues.

Shailesh351 · 2018-08-25T07:33:20Z

@luciofm I tried with Android Studio 3.2 RC 1 but develop branch is giving error while building.

Cannot access class 'Lkotlinx.coroutines.experimental.android.HandlerContext;'. Check your module classpath for missing or conflicting dependencies

Shailesh351 · 2018-08-25T08:51:51Z

@luciofm Done!!!. PR is ready.

luciofm · 2018-08-28T16:04:02Z

Merged with #1620

Shailesh351 added 2 commits July 30, 2018 09:24

feat: Add support for bot ButtonAction Attachment

8266b7d

fix: Errors after cherrypick

4000400

Shailesh351 changed the title ~~Upstrem button action~~ [NEW] Add support for initial Rich Messaging(ButtonAction as Attachment) Jul 30, 2018

rafaelks added this to the 2.6.0 milestone Aug 7, 2018

rafaelks added the 🎉 feature label Aug 7, 2018

rafaelks requested review from luciofm and philipbrito August 7, 2018 20:00

philipbrito suggested changes Aug 7, 2018

View reviewed changes

luciofm suggested changes Aug 7, 2018

View reviewed changes

Shailesh351 added 2 commits August 23, 2018 13:16

feat: Add horizontal and vertical buttons

611b008

Merge branch 'develop' into upstrem-button-action

00aa5de

philipbrito reviewed Aug 23, 2018

View reviewed changes

philipbrito suggested changes Aug 23, 2018

View reviewed changes

fix: Small fix

ee657f5

Merge branch 'temp-button-action' into upstrem-button-action

philipbrito changed the title ~~[NEW] Add support for initial Rich Messaging(ButtonAction as Attachment)~~ [NEW] Add support for initial Rich Messaging Aug 23, 2018

luciofm approved these changes Aug 23, 2018

View reviewed changes

luciofm suggested changes Aug 23, 2018

View reviewed changes

fix: Indentation and nullability

894b61e

Merge branch 'develop' into upstrem-button-action

433c545

philipbrito approved these changes Aug 28, 2018

View reviewed changes

luciofm mentioned this pull request Aug 28, 2018

[NEW] Add support for initial Rich Messaging #1620

Merged

luciofm approved these changes Aug 28, 2018

View reviewed changes

luciofm closed this Aug 28, 2018

Shailesh351 deleted the upstrem-button-action branch June 21, 2019 03:03

bart11064 mentioned this pull request Dec 8, 2020

Button on message attachment not showing RocketChat/Rocket.Chat.ReactNative#2684

Closed

[NEW] Add support for initial Rich Messaging #1557

[NEW] Add support for initial Rich Messaging #1557

Conversation

Shailesh351 commented Jul 30, 2018

CLAassistant commented Jul 30, 2018 • edited Loading

rafaelks commented Jul 31, 2018

rafaelks commented Aug 7, 2018

philipbrito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipbrito Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shailesh351 commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luciofm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luciofm left a comment • edited Loading

Choose a reason for hiding this comment

Shailesh351 commented Aug 24, 2018

luciofm commented Aug 24, 2018

Shailesh351 commented Aug 25, 2018

Shailesh351 commented Aug 25, 2018

luciofm commented Aug 28, 2018

CLAassistant commented Jul 30, 2018 •

edited

Loading

philipbrito Aug 8, 2018 •

edited

Loading

luciofm left a comment •

edited

Loading