twilio / twilio-chat-demo-android

Chat API Demo Application for Android
MIT License
62 stars 51 forks source link

Question: How to improve code to get list of all channels with metadata #136

Closed emartynov closed 4 years ago

emartynov commented 4 years ago

Hi people,

Here is the code that I wrote to get complete list of user channels with metadata:

    private val channelsSubject = BehaviorSubject.create<List<ChatChannelMetaData>>()

    private val refreshStarted = AtomicBoolean(false)

    override val channels: Observable<List<ChatChannelMetaData>> = channelsSubject.hide()
        get() {
            if (!channelsSubject.hasValue()) {
                refreshChannels()
            }

            return field
        }

    private fun refreshChannels() {
        if (!refreshStarted.compareAndSet(false, true)) return

        val channels = client.channels

        channels.getPublicChannelsList(object : CallbackListener<Paginator<ChannelDescriptor>>() {
            override fun onSuccess(paginator: Paginator<ChannelDescriptor>?) {
                val subscribedChannels = channels.subscribedChannels

                if (subscribedChannels.size == 0) {
                    channelsSubject.onNext(emptyList())
                }

                fetchChannelsInfo(subscribedChannels)
            }
        })
    }

    private fun fetchChannelsInfo(subscribedChannels: List<Channel>) {
        subscribedChannels.forEachIndexed { index, subscribedChannel ->
            val isLastChannel = index == subscribedChannels.size - 1
            if (subscribedChannel.synchronizationStatus == ALL) {
                populatedChannelInfo(subscribedChannel, isLastChannel)
            } else {
                subscribedChannel.addListener(
                        object : TwilioChannelListenerAdapter() {
                            override fun onSynchronizationChanged(channel: Channel) {
                                if (channel.synchronizationStatus == ALL) {
                                    channel.removeListener(this)
                                    populatedChannelInfo(channel, isLastChannel)
                                }
                            }
                        }
                )
            }
        }
    }

    private fun populatedChannelInfo(channel: Channel, isLastChannel: Boolean) {
        val attributes = channel.attributes.toString()
        val metaDataAttribute: ChatChannelAttribute? = try {
            GsonUtil.getInstance().fromJson<ChatChannelAttribute>(
                    attributes,
                    object : TypeToken<ChatChannelAttribute>() {}.type
            )
        } catch (error: Throwable) {
            LogUtil.e("TwilioChat", "Invalid chat channel attributes", error)
            null
        }

        val metaData = ChatChannelMetaData(channel.sid, channel, metaDataAttribute)
        val existingChannels = channelsSubject.value
        val newList = existingChannels?.toMutableList() ?: mutableListOf()
        newList.add(metaData)
        channelsSubject.onNext(newList)

        if (isLastChannel) {
            refreshStarted.set(false)
        }
    }

Can I simplify it? Is there are more elegant way to do it?

I also set listener to client to receive callback when new channel is added/updated/deleted.

I'm afraid what will happen with all this refresh process when one of these callbacks will arrive.

emartynov commented 4 years ago

Sorry for such dummy ticket. But I want to highlight how hard/complex it is in the current API to get list of channels sorted by the last received message.

rusmonster commented 4 years ago

Hi again @emartynov! We appreciate your interest to our product!

  1. In order to get complete list of user channels you don't have to call getPublilcChannelList at all.

There are 3 channel collections exposed by SDK:

  1. As soon as you already use kotlin you can simplify the code a lot using coroutines:

    
    suspend fun getChannelsMetaDataList(): List<ChatChannelMetaData> {
        client.waitForSynchronization()
        client.channels.subscribedChannels.forEach { it.waitForSynchronization() }
        return client.channels.subscribedChannels
                .map { ChatChannelMetaData(it.sid, it, it.attributes.toChatChannelAttribute()) }
    }
    
    fun Attributes.toChatChannelAttribute() = try {
            GsonUtil.getInstance().fromJson<ChatChannelAttribute>(
                    "$this",
                    object : TypeToken<ChatChannelAttribute>() {}.type
            )
        } catch (error: Throwable) {
            LogUtil.e("TwilioChat", "Invalid chat channel attributes", error)
            null
        }
    
    suspend fun ChatClient.waitForSynchronization(
            synchronizationStatus: ChatClient.SynchronizationStatus = ChatClient.SynchronizationStatus.COMPLETED
    ): Unit = suspendCancellableCoroutine { continuation ->
    
        setListener(
                onClientSynchronization = { status ->
                    synchronized(continuation) {
                        if (continuation.isActive && status == synchronizationStatus) {
                            removeListener()
                            continuation.resume(Unit)
                        }
                    }
                }
        )
    }
    
    suspend fun Channel.waitForSynchronization(
            synchronizationStatus: Channel.SynchronizationStatus = Channel.SynchronizationStatus.ALL) {
    
        val complete = CompletableDeferred<Unit>()
    
        val listener = addListener(
                onSynchronizationChanged = { channel ->
                    synchronized<Unit>(complete) {
                        if (complete.isCompleted) return@addListener
    
                        if (channel.synchronizationStatus == Channel.SynchronizationStatus.FAILED) {
                            val errorInfo = ErrorInfo(
                                    CHANNEL_NOT_SYNCHRONIZED, "Channel synchronization failed: ${channel.sid}}")
    
                            complete.completeExceptionally(ChatException(errorInfo))
                        } else if (channel.synchronizationStatus.value >= synchronizationStatus.value) {
                            complete.complete(Unit)
                        }
                    }
                }
        )
    
        try {
            complete.await()
        } finally {
            removeListener(listener)
        }
    }

In order to save space I haven't provide setListener/addListener implementations here which are trivial and could be done the same way as in android-ktx

  1. I don't see any sorting in your sample code but you can use getSubscribedChannelsSortedBy for that.

  2. It's up to you how to organise your application and how to handle onChannelAdded/Updated/Deleted callbacks. I could recommend the google's Guide to app architecture and use the ChatClient as Remote data source in the Repository

Feel free to ask If you have more questions. We are here to help :)

emartynov commented 4 years ago

Great! Appreciate the answer!

We don't use co-routines, so I can not simplify kotlin code in the proposed way. But it looks much simpler and readable.

Couple of questions:

  1. Am I right that SDK will always try to get all channels synchronized?
  2. Will channel receive channels synchronized event again when new channels will be added/deleted/updated on server?
  3. I don't need to call any load methods on client or channels to initiate the sync process?
  4. Will sorting channels method initiate sync status in sorted order also for every channel?
  5. How to force client to load new channels created on server? How to refresh list of channels from server?
  6. Do you have some documentation about how internals of the library work in terms of the synchronization?

So my logic for loading channels should be next:

  1. Instantiate client and set listener
  2. Wait until client get special sync status channels - I have list of all channels now
  3. For each channel set listener
  4. Wait until every channel gets sync status all - I can read last message now
  5. If received all channels - sort them in chronological order

One API suggestion - having one listener for client is not convenient sometimes - since you wont receive sync status, typing status on the channel object.

rusmonster commented 4 years ago

Hi! Please found inline answers below

  1. Am I right that SDK will always try to get all channels synchronized?

SDK always try to get only USER channels synchronized

  1. Will channel receive channels synchronized event again when new channels will be added/deleted/updated on server?

No, the Client will not receive channels synchronized event again when new channels will be added/deleted/updated on server. It will receive onChannelAdded/Deleted/Updated event instead.

  1. I don't need to call any load methods on client or channels to initiate the sync process?

Correct for Client and user channels, for public channels you have to call getChannel in order to start channel synchronization process.

  1. Will sorting channels method initiate sync status in sorted order also for every channel?

getSubscribedChannelsSortedBy doesn't initialise sync channels, it just returns user channels in their current statuses. Then you can add ChannelListener to each channel in order to monitor synchronisation status updates.

  1. How to force client to load new channels created on server? How to refresh list of channels from server?

No need to force anything. Once new channel created on server client will receive the onChannelAdded callback. The same for onChannelUpdated and onChannelDeleted.

Do you have some documentation about how internals of the library work in terms of the synchronization?

At the moment we have only docs and sample app code in this repo

So my logic for loading channels should be next: Instantiate client and set listener Wait until client get special sync status channels - I have list of all channels now For each channel set listener Wait until every channel gets sync status all - I can read last message now If received all channels - sort them in chronological order

Correct if you are interested in user channels only, for pubilc channels you have to start synchronization by calling getChannel

One API suggestion - having one listener for client is not convenient sometimes - since you wont receive sync status, typing status on the channel object.

Agree that better to be able to add multiple listeners to the client, will add to our backlog. I didn't get what do you mean you wont receive sync status, typing status on the channel object. Please clarify. btw onTypingStarted is ChannelListener method, not the ClientListener. Channels are able to have multiple listeners in the current SDK version.

emartynov commented 4 years ago

Thank you, let me review my code with these comments.