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

[ECO-5082] feat: presence basic implementation #42

Merged
merged 3 commits into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions chat-android/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ buildConfig {
dependencies {
api(libs.ably.android)
implementation(libs.gson)
implementation(libs.coroutine.core)

testImplementation(libs.junit)
testImplementation(libs.mockk)
Expand Down
96 changes: 69 additions & 27 deletions chat-android/src/main/java/com/ably/chat/Presence.kt
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,18 @@

package com.ably.chat

import android.text.PrecomputedText.Params
import com.google.gson.JsonElement
import com.google.gson.JsonObject
import io.ably.lib.realtime.Channel
import io.ably.lib.realtime.Presence.GET_CLIENTID
import io.ably.lib.realtime.Presence.GET_CONNECTIONID
import io.ably.lib.realtime.Presence.GET_WAITFORSYNC
import io.ably.lib.types.Param
import io.ably.lib.types.PresenceMessage
import io.ably.lib.realtime.Presence as PubSubPresence
import io.ably.lib.realtime.Presence.PresenceListener as PubSubPresenceListener

typealias PresenceData = Any
typealias PresenceData = JsonElement

sacOO7 marked this conversation as resolved.
Show resolved Hide resolved
/**
* This interface is used to interact with presence in a chat room: subscribing to presence events,
Expand All @@ -22,11 +29,12 @@ interface Presence : EmitsDiscontinuities {
val channel: Channel

/**
* Method to get list of the current online users and returns the latest presence messages associated to it.
* @param {Ably.RealtimePresenceParams} params - Parameters that control how the presence set is retrieved.
* @returns {Promise<PresenceMessage[]>} or upon failure, the promise will be rejected with an [[Ably.ErrorInfo]] object which explains the error.
* Method to get list of the current online users and returns the latest presence messages associated to it.
* @param {Ably.RealtimePresenceParams} params - Parameters that control how the presence set is retrieved.
* @throws {@link io.ably.lib.types.AblyException} object which explains the error.
* @returns {List<PresenceMessage>}
*/
suspend fun get(params: List<Params>): List<PresenceMember>
suspend fun get(waitForSync: Boolean = true, clientId: String? = null, connectionId: String? = null): List<PresenceMember>
Comment on lines +32 to +37
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Documentation needs update to match implementation

The documentation refers to Ably.RealtimePresenceParams but the method signature uses individual parameters. Consider using a typed parameter object as suggested in previous reviews for better API clarity and future extensibility.

data class RealtimePresenceParams(
    val waitForSync: Boolean = true,
    val clientId: String? = null,
    val connectionId: String? = null
)

suspend fun get(params: RealtimePresenceParams? = null): List<PresenceMember>


/**
* Method to check if user with supplied clientId is online
Expand All @@ -38,23 +46,23 @@ interface Presence : EmitsDiscontinuities {
/**
* Method to join room presence, will emit an enter event to all subscribers. Repeat calls will trigger more enter events.
* @param {PresenceData} data - The users data, a JSON serializable object that will be sent to all subscribers.
* @returns {Promise<void>} or upon failure, the promise will be rejected with an {@link ErrorInfo} object which explains the error.
* @throws {@link io.ably.lib.types.AblyException} object which explains the error.
*/
suspend fun enter(data: PresenceData?)
suspend fun enter(data: PresenceData? = null)

/**
* Method to update room presence, will emit an update event to all subscribers. If the user is not present, it will be treated as a join event.
* @param {PresenceData} data - The users data, a JSON serializable object that will be sent to all subscribers.
* @returns {Promise<void>} or upon failure, the promise will be rejected with an {@link ErrorInfo} object which explains the error.
* @throws {@link io.ably.lib.types.AblyException} object which explains the error.
*/
suspend fun update(data: PresenceData?)
suspend fun update(data: PresenceData? = null)

/**
* Method to leave room presence, will emit a leave event to all subscribers. If the user is not present, it will be treated as a no-op.
* @param {PresenceData} data - The users data, a JSON serializable object that will be sent to all subscribers.
* @returns {Promise<void>} or upon failure, the promise will be rejected with an {@link ErrorInfo} object which explains the error.
* @throws {@link io.ably.lib.types.AblyException} object which explains the error.
*/
suspend fun leave(data: PresenceData?)
suspend fun leave(data: PresenceData? = null)

/**
* Subscribe the given listener to all presence events.
Expand Down Expand Up @@ -86,7 +94,7 @@ data class PresenceMember(
/**
* The data associated with the presence member.
*/
val data: PresenceData,
val data: PresenceData?,

/**
* The current state of the presence member.
Expand Down Expand Up @@ -121,46 +129,80 @@ data class PresenceEvent(
/**
* The timestamp of the presence event.
*/
val timestamp: Int,
val timestamp: Long,

/**
* The data associated with the presence event.
*/
val data: PresenceData,
val data: PresenceData?,
)

internal class DefaultPresence(
private val messages: Messages,
private val clientId: String,
override val channel: Channel,
private val presence: PubSubPresence,
) : Presence {

override val channel: Channel
get() = messages.channel

override suspend fun get(params: List<Params>): List<PresenceMember> {
TODO("Not yet implemented")
suspend fun get(params: List<Param>): List<PresenceMember> {
val usersOnPresence = presence.getCoroutine(params)
return usersOnPresence.map { user ->
PresenceMember(
clientId = user.clientId,
action = user.action,
data = (user.data as? JsonObject)?.get("userCustomData"),
updatedAt = user.timestamp,
Comment on lines +152 to +153
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add safe casting for JsonObject data extraction

The current implementation uses an unsafe cast that could fail at runtime. Consider using safe cast and proper error handling.

-                data = (user.data as? JsonObject)?.get("userCustomData"),
+                data = runCatching {
+                    (user.data as? JsonObject)?.get("userCustomData")
+                }.getOrNull(),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
data = (user.data as? JsonObject)?.get("userCustomData"),
updatedAt = user.timestamp,
data = runCatching {
(user.data as? JsonObject)?.get("userCustomData")
}.getOrNull(),
updatedAt = user.timestamp,

)
}
}

override suspend fun isUserPresent(clientId: String): Boolean {
TODO("Not yet implemented")
override suspend fun get(waitForSync: Boolean, clientId: String?, connectionId: String?): List<PresenceMember> {
val params = buildList {
if (waitForSync) add(Param(GET_WAITFORSYNC, true))
sacOO7 marked this conversation as resolved.
Show resolved Hide resolved
clientId?.let { add(Param(GET_CLIENTID, it)) }
connectionId?.let { add(Param(GET_CONNECTIONID, it)) }
}
return get(params)
}

override suspend fun isUserPresent(clientId: String): Boolean = presence.getCoroutine(Param(GET_CLIENTID, clientId)).isNotEmpty()
sacOO7 marked this conversation as resolved.
Show resolved Hide resolved

override suspend fun enter(data: PresenceData?) {
TODO("Not yet implemented")
presence.enterClientCoroutine(clientId, wrapInUserCustomData(data))
}

override suspend fun update(data: PresenceData?) {
TODO("Not yet implemented")
presence.updateClientCoroutine(clientId, wrapInUserCustomData(data))
}

override suspend fun leave(data: PresenceData?) {
TODO("Not yet implemented")
presence.leaveClientCoroutine(clientId, wrapInUserCustomData(data))
}

override fun subscribe(listener: Presence.Listener): Subscription {
TODO("Not yet implemented")
val presenceListener = PubSubPresenceListener {
val presenceEvent = PresenceEvent(
action = it.action,
clientId = it.clientId,
timestamp = it.timestamp,
data = (it.data as? JsonObject)?.get("userCustomData"),
)
Comment on lines +187 to +188
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add safe casting in presence event creation

Similar to the get method, this uses an unsafe cast that could fail at runtime.

-                data = (it.data as? JsonObject)?.get("userCustomData"),
+                data = runCatching {
+                    (it.data as? JsonObject)?.get("userCustomData")
+                }.getOrNull(),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
data = (it.data as? JsonObject)?.get("userCustomData"),
)
data = runCatching {
(it.data as? JsonObject)?.get("userCustomData")
}.getOrNull(),

listener.onEvent(presenceEvent)
}

presence.subscribe(presenceListener)

return Subscription {
presence.unsubscribe(presenceListener)
}
}

override fun onDiscontinuity(listener: EmitsDiscontinuities.Listener): Subscription {
TODO("Not yet implemented")
}
Comment on lines 199 to 201
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Implementation needed across multiple components

The onDiscontinuity method is not implemented in multiple core components: Messages, Presence, Typing, RoomReactions, and Occupancy. This appears to be a systematic gap in implementing the EmitsDiscontinuities interface across the codebase. The interface is defined to handle discontinuity events with error information, but none of the implementations are complete.

  • chat-android/src/main/java/com/ably/chat/Messages.kt
  • chat-android/src/main/java/com/ably/chat/Presence.kt
  • chat-android/src/main/java/com/ably/chat/Typing.kt
  • chat-android/src/main/java/com/ably/chat/RoomReactions.kt
  • chat-android/src/main/java/com/ably/chat/Occupancy.kt
🔗 Analysis chain

Implement onDiscontinuity method

The onDiscontinuity method is marked as not implemented. Please clarify if this is intentional or if it should be implemented as part of this PR.

Would you like help implementing this method?


Let me check the implementation in other files to understand the expected behavior.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if onDiscontinuity is used in tests or other files
rg -l "onDiscontinuity"

Length of output: 357


Script:

#!/bin/bash
# Check EmitsDiscontinuities interface definition
cat chat-android/src/main/java/com/ably/chat/EmitsDiscontinuities.kt

# Check implementations in other files
rg "onDiscontinuity.*{" -A 5 --multiline

Length of output: 944


Script:

#!/bin/bash
# Check implementations in other files with fixed regex
rg "override fun onDiscontinuity" -A 5

# Also check if there are any tests for this functionality
fd -e kt -e java test --exec rg -l "onDiscontinuity" {} \;

Length of output: 2233


private fun wrapInUserCustomData(data: PresenceData?) = data?.let {
JsonObject().apply {
add("userCustomData", data)
}
}
}
10 changes: 7 additions & 3 deletions chat-android/src/main/java/com/ably/chat/Room.kt
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,12 @@ interface Room {
internal class DefaultRoom(
override val roomId: String,
override val options: RoomOptions,
realtimeClient: RealtimeClient,
val realtimeClient: RealtimeClient,
ttypic marked this conversation as resolved.
Show resolved Hide resolved
chatApi: ChatApi,
) : Room {

private val clientId get() = realtimeClient.auth.clientId

private val _messages = DefaultMessages(
roomId = roomId,
realtimeChannels = realtimeClient.channels,
Expand All @@ -100,12 +102,14 @@ internal class DefaultRoom(
override val messages: Messages = _messages

override val presence: Presence = DefaultPresence(
messages = messages,
channel = messages.channel,
clientId = clientId,
presence = messages.channel.presence,
)

override val reactions: RoomReactions = DefaultRoomReactions(
roomId = roomId,
clientId = realtimeClient.auth.clientId,
clientId = clientId,
realtimeChannels = realtimeClient.channels,
)

Expand Down
63 changes: 63 additions & 0 deletions chat-android/src/main/java/com/ably/chat/Utils.kt
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
package com.ably.chat

import com.google.gson.JsonElement
import io.ably.lib.realtime.Channel
import io.ably.lib.realtime.CompletionListener
import io.ably.lib.types.AblyException
import io.ably.lib.types.ChannelOptions
import io.ably.lib.types.ErrorInfo
import io.ably.lib.types.Param
import kotlin.coroutines.resume
import kotlin.coroutines.resumeWithException
import kotlin.coroutines.suspendCoroutine
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.suspendCancellableCoroutine
import kotlinx.coroutines.withContext
import io.ably.lib.realtime.Presence as PubSubPresence

const val AGENT_PARAMETER_NAME = "agent"

Expand Down Expand Up @@ -50,6 +56,63 @@ suspend fun Channel.publishCoroutine(message: PubSubMessage) = suspendCoroutine
)
}

suspend fun PubSubPresence.getCoroutine(param: Param) = withContext(Dispatchers.IO) {
Copy link

Choose a reason for hiding this comment

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

We can now safely remove this helper method right

get(param)
}
ttypic marked this conversation as resolved.
Show resolved Hide resolved
sacOO7 marked this conversation as resolved.
Show resolved Hide resolved

@Suppress("SpreadOperator")
suspend fun PubSubPresence.getCoroutine(params: List<Param>) = withContext(Dispatchers.IO) {
sacOO7 marked this conversation as resolved.
Show resolved Hide resolved
get(*params.toTypedArray())
}

suspend fun PubSubPresence.enterClientCoroutine(clientId: String, data: JsonElement?) = suspendCancellableCoroutine { continuation ->
enterClient(
clientId,
data,
object : CompletionListener {
override fun onSuccess() {
continuation.resume(Unit)
}

override fun onError(reason: ErrorInfo?) {
continuation.resumeWithException(AblyException.fromErrorInfo(reason))
}
},
)
}
ttypic marked this conversation as resolved.
Show resolved Hide resolved

suspend fun PubSubPresence.updateClientCoroutine(clientId: String, data: JsonElement?) = suspendCancellableCoroutine { continuation ->
updateClient(
clientId,
data,
object : CompletionListener {
override fun onSuccess() {
continuation.resume(Unit)
}

override fun onError(reason: ErrorInfo?) {
continuation.resumeWithException(AblyException.fromErrorInfo(reason))
}
},
)
}

suspend fun PubSubPresence.leaveClientCoroutine(clientId: String, data: JsonElement?) = suspendCancellableCoroutine { continuation ->
leaveClient(
clientId,
data,
object : CompletionListener {
override fun onSuccess() {
continuation.resume(Unit)
}

override fun onError(reason: ErrorInfo?) {
continuation.resumeWithException(AblyException.fromErrorInfo(reason))
}
},
)
}

@Suppress("FunctionName")
fun ChatChannelOptions(init: (ChannelOptions.() -> Unit)? = null): ChannelOptions {
val options = ChannelOptions()
Expand Down
Loading
Loading