Remove "X kicked X" terminology when removing people from a room #4911

fedrunov · 2022-01-11T15:59:09Z

there are three separate commits:

replacing "kick" with "remove" in code and resource. String reource's keys are unaffected, @bmarty said that will handle it later
adding alias to commands.
adding aliases which are already exist in web app, taking from https://github.com/matrix-org/matrix-react-sdk/blob/develop/src/SlashCommands.tsx

also there are some small fixes/changes:

case is now ignored (before /Nick returned an error, now works)
fixed typo ("sPlashCommand") in method name and logs (could change in logs somehow affect analytics?)
added strict check for excess params for DISCARD_SESSION command, same as it is for CLEAR_SCALAR_TOKEN
some commands didn't require param, while it looks like they should, so check was addde:
/rainbow - posts empty message
/spoiler - posts empty message
/me - posts message with just user name
/rainbowme - posts message with just user name

fixes #4865

github-actions · 2022-01-11T16:27:43Z

Unit Test Results

  66 files ±0   66 suites ±0 49s ⏱️ -4s
135 tests ±0 135 ✔️ ±0 0 💤 ±0 0 ❌ ±0
418 runs ±0 418 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit d92e0e4. ± Comparison against base commit 67bdf4b.

♻️ This comment has been updated with latest results.

bmarty

Not a lot to say, this is really nice work, thanks!

bmarty · 2022-01-11T17:14:43Z

...k-android/src/main/java/org/matrix/android/sdk/api/session/room/members/MembershipService.kt

@@ -77,7 +77,7 @@ interface MembershipService {
    /**
     * Kick a user from the room
     */
-    suspend fun kick(userId: String, reason: String? = null)
+    suspend fun remove(userId: String, reason: String? = null)


Maybe also update the comment above

bmarty · 2022-01-11T17:16:51Z

...k-android/src/main/java/org/matrix/android/sdk/api/session/room/members/MembershipService.kt

@@ -77,7 +77,7 @@ interface MembershipService {
    /**
     * Kick a user from the room
     */
-    suspend fun kick(userId: String, reason: String? = null)
+    suspend fun remove(userId: String, reason: String? = null)


As it is a change in the SDK api, would be better to keep kick as an alias to remove and mark it as deprecated.
You can write in the interface something like:

@Deprecated("Use remove now") suspend fun kick(userId: String, reason: String? = null) = remove(userId, reason)

bmarty · 2022-01-11T17:22:26Z

vector/src/main/java/im/vector/app/features/command/Command.kt

-    JOIN_SPACE("/joinSpace", "spaceId", R.string.command_description_join_space, true),
-    LEAVE_ROOM("/leave", "<roomId?>", R.string.command_description_leave_room, true),
-    UPGRADE_ROOM("/upgraderoom", "newVersion", R.string.command_description_upgrade_room, true);
+enum class Command(val command: String, val aliases: Array<CharSequence>?, val parameters: String, @StringRes val description: Int, val isDevCommand: Boolean) {


not a strong opinion about it, but why using Array instead of List?

I did it because of spread operation used later here

val allAliases = arrayOf(command, *aliases.orEmpty())

bmarty · 2022-01-11T17:23:48Z

vector/src/main/java/im/vector/app/features/command/CommandParser.kt

@@ -32,12 +32,12 @@ object CommandParser {
     * @param textMessage   the text message
     * @return a parsed slash command (ok or error)
     */
-    fun parseSplashCommand(textMessage: CharSequence): ParsedCommand {
+    fun parseSlashCommand(textMessage: CharSequence): ParsedCommand {


Good catch, nice typo, thanks :)

(my bad eae8f99 :) )

bmarty · 2022-01-11T17:24:27Z

vector/src/main/java/im/vector/app/features/command/CommandParser.kt

        // check if it has the Slash marker
        if (!textMessage.startsWith("/")) {
            return ParsedCommand.ErrorNotACommand
        } else {
-            Timber.v("parseSplashCommand")
+            Timber.v("parseSlashCommand")


Changing the logs does not impact analytics.

bmarty · 2022-01-11T17:29:21Z

vector/src/main/java/im/vector/app/features/command/CommandParser.kt

-                    if (newVersion.isEmpty()) {
-                        ParsedCommand.ErrorSyntax(Command.UPGRADE_ROOM)
+                Command.ADD_TO_SPACE.matches(slashCommand)                 -> {
+                    ParsedCommand.AddToSpace(spaceId = message)


Maybe we miss some validation of message for this command and the one below.

bmarty · 2022-01-11T17:30:18Z

vector/src/main/java/im/vector/app/features/command/CommandParser.kt

+
+    private fun trimParts(message: CharSequence, messageParts: List<String>): String? {
+        val partsSize = messageParts.sumOf { it.length }
+        val gapsNumber = messageParts.size - 1


I am wondering if it still works well if the user adds more space chars between the params

I've checked this case and it worked well. It just trims every space betwen params

bmarty · 2022-01-11T17:31:42Z

vector/src/main/res/values/strings.xml

@@ -25,8 +25,8 @@
    <string name="notice_direct_room_leave_by_you">You left the room</string>
    <string name="notice_room_reject">%1$s rejected the invitation</string>
    <string name="notice_room_reject_by_you">You rejected the invitation</string>
-    <string name="notice_room_kick">%1$s kicked %2$s</string>
-    <string name="notice_room_kick_by_you">You kicked %1$s</string>
+    <string name="notice_room_kick">%1$s removed %2$s</string>


Can you add a TODO in XML comment above so that I will not forget to rename during the next weblate sync. Thanks. (only one comment will be enough)

bmarty · 2022-01-11T17:33:42Z

changelog.d/4865.misc

@@ -0,0 +1 @@
+"/kick" command is replaced with "/remove"


Maybe also mention the replacement in the wordings.

github-actions · 2022-01-13T18:03:18Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
passed="21" failures="0" errors="0" skipped="2"
[org.matrix.android.sdk.account]
passed="5" failures="0" errors="0" skipped="2"
[org.matrix.android.sdk.internal]
passed="1" failures="1" errors="0" skipped="0"
[org.matrix.android.sdk.ordering]
passed="1" failures="1" errors="0" skipped="0"
[org.matrix.android.sdk.PermalinkParserTest]
passed="1" failures="1" errors="0" skipped="0"

bmarty

Thanks for the update!

fedrunov added 3 commits January 11, 2022 16:21

replace "kick" with "remove"

9ec662c

added aliases to commands

2aabbf0

web version aliases

9fa38c5

fedrunov requested a review from bmarty January 11, 2022 15:59

bmarty reviewed Jan 11, 2022

View reviewed changes

code review changes

d92e0e4

bmarty approved these changes Jan 14, 2022

View reviewed changes

bmarty merged commit 31e487b into develop Jan 14, 2022

bmarty deleted the feature/nfe/rename_kick_command branch January 14, 2022 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove "X kicked X" terminology when removing people from a room #4911

Remove "X kicked X" terminology when removing people from a room #4911

fedrunov commented Jan 11, 2022

github-actions bot commented Jan 11, 2022 •

edited

Loading

bmarty left a comment

bmarty Jan 11, 2022

bmarty Jan 11, 2022

bmarty Jan 11, 2022

fedrunov Jan 13, 2022

bmarty Jan 11, 2022

bmarty Jan 11, 2022

bmarty Jan 11, 2022

bmarty Jan 11, 2022

bmarty Jan 11, 2022

fedrunov Jan 13, 2022

bmarty Jan 11, 2022

bmarty Jan 11, 2022

github-actions bot commented Jan 13, 2022

bmarty left a comment

Remove "X kicked X" terminology when removing people from a room #4911

Remove "X kicked X" terminology when removing people from a room #4911

Conversation

fedrunov commented Jan 11, 2022

github-actions bot commented Jan 11, 2022 • edited Loading

Unit Test Results

bmarty 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

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

github-actions bot commented Jan 13, 2022

Matrix SDK

Integration Tests Results:

bmarty left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 11, 2022 •

edited

Loading