Message Draft #24

jguz-pubnub · 2024-07-08T07:43:32Z

No description provided.

wkal-pubnub · 2024-07-08T10:51:14Z

src/commonMain/kotlin/com/pubnub/kmp/message_draft/MessageDraft.kt

+
+        val before = currentText.substring(0, positionInInput)
+        val after = currentText.substring(positionInInput)
+        val newString = before + after.replaceFirst(link, text)


I don't see any replacement in the TypeScript code... your code makes sense to me I think (replace the URL with the link text), but how is it done in TypeScript? does it also happen somewhere?

wkal-pubnub · 2024-07-08T10:52:08Z

src/commonMain/kotlin/com/pubnub/kmp/message_draft/MessageDraft.kt

+    private val userSuggestionLimit: Int = 10,
+    private val shouldTriggerTypingIndicator: Boolean = false
+) {
+    private var currentTextLinkDescriptors: MutableList<TextLinkDescriptor> = emptyList<TextLinkDescriptor>().toMutableList()


always prefer val unless var is required (in all properties in this class)

wkal-pubnub · 2024-07-08T10:53:28Z

src/commonMain/kotlin/com/pubnub/kmp/message_draft/MessageDraft.kt

+        val after = currentText.substring(positionInInput)
+        val newString = before + after.replaceFirst(link, text)
+
+        currentText = newString


I don't know if it will matter in the end, but in TS Chat the onChange is invoked first, then the textLinkDescriptor is added to currentTextLinkDescriptors

wkal-pubnub · 2024-07-08T10:55:03Z

src/commonMain/kotlin/com/pubnub/kmp/message_draft/MessageDraft.kt

+        if (!currentTextLinkDescriptors.removeAll {
+            it.range.contains(positionInInput)
+        }) {
+            println("This operation is noop. There is no link at this position.")


add a TODO to use a logger (once we have it) instead of println

wkal-pubnub · 2024-07-08T11:15:48Z

src/commonMain/kotlin/com/pubnub/kmp/message_draft/MessageDraft.kt

+    private var userPrefix = "@"
+    private var channelPrefix = "#"
+
+    var onSuggestionsChanged: (String, SuggestedMessageDraftMentions) -> Unit = { _, _  -> }


I don't see a similar feature (setting a callback for changing suggestions) in TS?

wkal-pubnub · 2024-07-08T11:17:31Z

src/commonMain/kotlin/com/pubnub/kmp/message_draft/MessageDraft.kt

+    private var getUsersFuture: PNFuture<List<User>> = emptyList<User>().asFuture()
+    private var getChannelsFuture: PNFuture<List<Channel>> = emptyList<Channel>().asFuture()
+
+    private var minNameLength: Int = 3


these could be const val in the file (outside of the class)

wkal-pubnub · 2024-07-08T11:19:11Z

src/commonMain/kotlin/com/pubnub/kmp/message_draft/MessageDraft.kt

+    private var quotedMessage: Message? = null
+    private var mentionedUsers: MutableList<MentionDescriptor<User>> = emptyList<MentionDescriptor<User>>().toMutableList()
+    private var mentionedChannels: MutableList<MentionDescriptor<Channel>> = emptyList<MentionDescriptor<Channel>>().toMutableList()
+    private var getUsersFuture: PNFuture<List<User>> = emptyList<User>().asFuture()


doesn't seem like these futures need to be saved in a property? they're only used in the function body

wkal-pubnub · 2024-07-08T11:26:09Z

src/commonMain/kotlin/com/pubnub/kmp/message_draft/MessageDraft.kt

+        }
+    }
+
+    fun removeMentionedUser(positionInInput: Int) {


my reading of the TS code and documentation is that user and channel mentions don't work on text positions, but on the number of "@" or "#" characters. For example, if there is a string blah @firstmention blah @secondMention blah @third blah, then the mentions have nameOccurrenceIndex (name from TS SDK) 0 for firstMention, 1 for secondMention and 2 for third. And so this and related methods should not operate on positionInInput and text ranges, but rather on the numbered "@" occurences

marcin-cebo · 2024-07-08T10:36:38Z

src/commonMain/kotlin/com/pubnub/kmp/Chat.kt

@@ -159,6 +159,11 @@ interface Chat {
        message: EventContent,
        mergeMessageWith: Map<String, Any>? = null
    ): PNFuture<PNPublishResult>
+
+    fun getChannelSuggestions(


I would place it above section:

/* should be internal */ fun signal(

marcin-cebo · 2024-07-08T10:41:04Z

src/commonMain/kotlin/com/pubnub/kmp/Chat.kt

+
+    fun getChannelSuggestions(
+       filter: String?,
+       limit: Int


What do you think about setting default 10 like and changing return type to PNFuture<Set> like this:
fun getChannelSuggestions(text: String, limit: Int = 10): PNFuture<Set<Channel>>

marcin-cebo · 2024-07-08T10:50:33Z

src/commonMain/kotlin/com/pubnub/kmp/ChatImpl.kt

+            Result.failure(PubNubException(FAILED_TO_GET_CHANNELS.message, exception))
+        }
+    }
+


In Master we already have following code:

override fun getChannelSuggestions(text: String, limit: Int): PNFuture<Set<Channel>> { val cacheKey: String = getPhraseToLookFor(text, "#") ?: return emptySet<Channel>().asFuture() suggestedChannelsCache[cacheKey]?.let { nonNullChannels -> return nonNullChannels.asFuture() } return getChannels(filter = "name LIKE '${cacheKey}*'", limit = limit).then { getChannelsResponse -> val channels: Set<Channel> = getChannelsResponse.channels suggestedChannelsCache[cacheKey] = channels channels } }

That is a bit different with this one. I am not saying that is correct one but in TypeScript code there is concept of
suggestedChannelsCache that I can not see in your implementation. Did you deliberately skip it here? Maybe is in other place that I can not see.

Other thing to be take into consideration is that there is also getUserSuggestions that similarly to getChannelSuggestions uses getPhraseToLookFor method.

marcin-cebo · 2024-07-08T10:59:48Z

src/commonMain/kotlin/com/pubnub/kmp/message_draft/MessageDraft.kt

+    private val userSuggestionLimit: Int = 10,
+    private val shouldTriggerTypingIndicator: Boolean = false
+) {
+    private var currentTextLinkDescriptors: MutableList<TextLinkDescriptor> = emptyList<TextLinkDescriptor>().toMutableList()


Could be also:
private var currentTextLinkDescriptors: MutableList = mutableListOf()

marcin-cebo · 2024-07-08T11:05:19Z

src/commonMain/kotlin/com/pubnub/kmp/message_draft/MessageDraft.kt

+            when (userSuggestionDataSource) {
+                UserSuggestionDataSource.CHANNEL ->
+                    channel.getMembers(
+                        filter = "uuid.name LIKE '${userMention.first}'",


Isn't "*" needed here like this:
filter = "uuid.name LIKE '${userMention.first}*'",
?

marcin-cebo · 2024-07-08T11:07:34Z

src/commonMain/kotlin/com/pubnub/kmp/message_draft/MessageDraft.kt

+    var currentText: String by Delegates.observable("") { _, oldValue, newValue -> onTextChanged(oldValue, newValue) }
+
+    init {
+        require(userSuggestionLimit <= 100) { "Fetching more than 100 users is prohibited" }


Not sure but probably we want to throw PubNubException in such cases.

marcin-cebo · 2024-07-08T11:10:11Z

src/commonMain/kotlin/com/pubnub/kmp/message_draft/MessageDraft.kt

+            link = link
+        )
+
+        if (!link.isValidUrl()) {


Why not to have this validation as a first operation in this method ? In case link is not valid url we wouldn't have to construct textLinkDescriptor?

marcin-cebo · 2024-07-08T12:52:43Z

src/commonTest/kotlin/com/pubnub/kmp/MessageDraftTest.kt

+    }
+
+    override fun getChannelSuggestions(filter: String?, limit: Int): PNFuture<List<Channel>> {
+        TODO("Not yet implemented")


Are you going to handle all TODO in this file?
If not what about sth like this:
return emptyList<Channel>().asFuture()
?
The thing is that IMHO TODO suggests that some action planned in this code.

What do you think?

jguz-pubnub added 13 commits July 1, 2024 20:33

Message draft (WIP)

ee0be0a

Unit tests

bc5b0dc

Added more unit tests

75707ed

Fix for querying items

48bf361

Some minor fixes

2912d7f

Minor fixes for MessageDraft.kt

5374742

Improving unit tests

fea1bbb

Minor fix in ChatImpl.kt

8307360

Minor fix in Chat.kt

fd99b68

Whitespace in Chat.kt

db1b1a3

Removed no longer used range.kt

9c127c3

Whitespace in MessageDraft.kt

c14a041

Whitespace in MessageDraft.kt

2a85e8f

jguz-pubnub requested review from marcin-cebo and wkal-pubnub July 8, 2024 08:48

wkal-pubnub requested changes Jul 8, 2024

View reviewed changes

marcin-cebo reviewed Jul 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Message Draft #24

Message Draft #24

jguz-pubnub commented Jul 8, 2024

wkal-pubnub Jul 8, 2024

wkal-pubnub Jul 8, 2024

wkal-pubnub Jul 8, 2024

wkal-pubnub Jul 8, 2024

wkal-pubnub Jul 8, 2024

wkal-pubnub Jul 8, 2024

wkal-pubnub Jul 8, 2024

wkal-pubnub Jul 8, 2024

marcin-cebo Jul 8, 2024

marcin-cebo Jul 8, 2024

marcin-cebo Jul 8, 2024

marcin-cebo Jul 8, 2024

marcin-cebo Jul 8, 2024

marcin-cebo Jul 8, 2024

marcin-cebo Jul 8, 2024

marcin-cebo Jul 8, 2024

Message Draft #24

Are you sure you want to change the base?

Message Draft #24

Conversation

jguz-pubnub commented Jul 8, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment