[DO NOT MERGE] add an option to use display names in chatrooms #28

gordon-quad · 2019-01-21T01:08:02Z

Added nick command to change your display name
Added an option to use display names as nicks in chatrooms
Users data structures refactored to accept user_id as key since there could be multiple users with same display name.

Spec recommends to use format "displayname (user id)" to remove ambiguity. In my case I used color for that because weechat nick column can be quite limited in width.

Depends on matrix-nio/matrix-nio#2

gordon-quad · 2019-01-21T01:11:00Z

matrix/buffer.py

-        # TODO make this configurable
-        if not short_name or short_name in self.displayed_nicks.values():
-            # Use the full user id, but don't include the @
-            nick = user_id[1:]


Could not quite wrap my head around this if branch.
Was this an workaround to remote ambiguity? I did not notice that this branch is ever executed.

Short name can be an empty string here if someone has a user id of the format "@:example.org", someone in Matrix HQ had such an user id.
Short name can already be in displayed_nicks if two people have the same user part of the user_id. For example "@alice.example1.org" and "@alice.example2.org".

gordon-quad · 2019-01-21T12:21:36Z

So it seems that weechat's nicklist can support somehow addition of same nick multiple times, it returns different nick_ptr every time and you can use it to delete the nick, but it displays only one nick in the nicklist pane. I kinda have intention to find a way to disambiguate nicks in nicklist's pane but keep short displayname in rooms log using different colors to distinguish one from the other. While I'm trying to find approach to this I think PR is still reviewable since I don't think it's going to be changed much.

poljar · 2019-01-21T11:26:54Z

matrix/buffer.py

-        # TODO make this configurable
-        if not short_name or short_name in self.displayed_nicks.values():
-            # Use the full user id, but don't include the @
-            nick = user_id[1:]


Short name can be an empty string here if someone has a user id of the format "@:example.org", someone in Matrix HQ had such an user id.
Short name can already be in displayed_nicks if two people have the same user part of the user_id. For example "@alice.example1.org" and "@alice.example2.org".

poljar · 2019-01-21T11:30:03Z

matrix/buffer.py

@@ -227,6 +230,7 @@ class WeechatChannelBuffer(object):
        "kick": [SCRIPT_NAME + "_kick", "log4"],
        "invite": [SCRIPT_NAME + "_invite", "log4"],
        "topic": [SCRIPT_NAME + "_topic", "log3"],
+        "change": [SCRIPT_NAME + "_change", "log4"],


I used the same tags that the irc plugin uses since some plugins can reuse some known tags, also for consistency sake.
The irc plugin uses irc_nick for nick changes.

poljar · 2019-01-21T11:41:54Z

matrix/buffer.py

@@ -491,13 +495,13 @@ def _message_tags(self, user, message_type):

        return tags

-    def _get_user(self, nick):
+    def _get_user(self, user_id):


The WeechatChannelBuffer class was supposed to be protocol agnostic, that is it should not know what an user id is.
The idea was to make a higher level abstraction around a plain weechat buffer that implements actions like printing various message types, handling user joins, smart filtering, etc.
Other protocol scripts could reuse this then.

If we are going to introduce user ids into the WeechatChannelBuffer class then the separation between WeechatChannelBuffer and RoomBuffer doesn't make any sense anymore and they should be merged into a single class.
I prefer to leave the separation as it is but I'm also fine with a merged classes scenario, just not a combination of both.

Fyi, if you chose to leave the separation, the user_id -> nick mapping is handled in this hashmap https://github.com/poljar/weechat-matrix/blob/olm-command/matrix/buffer.py#L849

Do you have any other such abstract separations in you code somewhere?
If that's the only one with WeechatBuffer/RoomBuffer separation I would rather merge it if you don't mind.

There is in config.py.

If you're going to merge the classes please separate this into separate commits to ease the reviewing.

poljar · 2019-01-21T12:01:53Z

matrix/buffer.py

+            msg = self._nick_change_message(user, old_nick, new_nick)
+
+            # TODO add a option to disable smart filters
+            tags.append(SCRIPT_NAME + "_smart_filter")


The irc plugin adds two more tags for nick changes: irc_nick1 and irc_nick2.
Those tags contain the old and new nick and as far as i remember the smart filtering algorithm needs them to unmask lines that belong to the old nick.
More info here.

interesting, so i guess i cannot use spaces or special symbols in tags there during irc_nick1_%s construction. Will those tags serve any purpose for weechat if I strip displayname of whitespace, unicode and/or special symbols?

Oh right, display names can contain spaces.

I guess we'll have to change the smart filter algorithm to just use the user_id, and put the user_id into the tags of every message.

poljar · 2019-01-21T12:24:14Z

matrix/buffer.py

+            tags.append(SCRIPT_NAME + "_smart_filter")
+
+            self.print_date_tags(msg, date, tags)
+            self.add_smart_filtered_nick(user.nick)


You need to remove the old nick using remove_smart_filtered_nick(). The smart filter algorithm should then pick the old nick up from the tags just like it does in the irc plugin.

You don't need to make the smart filter algorithm work with nick changes for now. Just make sure to add the tags and remove the old nick here.

poljar · 2019-01-21T12:46:23Z

matrix/commands.py

@@ -178,6 +178,12 @@ def upload(args):
        parser.add_argument("file")
        return WeechatCommandParser._run_parser(parser, args)

+    @staticmethod


Please put the /nick command addition into a separate commit.

poljar · 2019-01-21T12:48:34Z

matrix/buffer.py

@@ -640,22 +644,19 @@ def _get_prefix_color(prefix):

    def _add_user_to_nicklist(self, user):
        # type: (WeechatUser) -> None
-        nick_pointer = W.nicklist_search_nick(self._ptr, "", user.nick)
-
-        if not nick_pointer:


Why did you remove the conditional here?

oops, will bring it back

it is not required in new commit since nick_ptrs and duplicate nicks handled in separately

gordon-quad · 2019-01-24T18:11:16Z

I did not fix issue with WeechatChannelBuffer/RoomBuffer yet, but it would be nice if you can check if you're happy with my solution to nick disambiguation and smart filtering.

It breaks nick autocompletion unfortunately but could be addressed with custom autocompletion for /input hook I guess (unless it also breaks something else).

poljar · 2019-01-25T10:00:53Z

matrix/buffer.py

@@ -685,10 +718,29 @@ def _membership_message(self, user, message_type):

        return message

+    def _nick_change_message(self, user, old_nick, new_nick):
+        action_color = "purple"


Purple is not a valid weechat color, you can see the available colors with the /color command after inputing e in the newly created buffer.

poljar · 2019-01-25T10:05:29Z

matrix/buffer.py

+        action_color = "purple"
+        prefix = "network"
+
+        message = (


This doesn't print out the disambiguated nicks which can lead to confusion when we have duplicates, maybe it'll make sense to always print out the user id alongside the first nick?

Also if you want to color the message, please make it configurable. The irc nick change message uses the default color, I would stick to that as a default as well.

poljar · 2019-01-25T10:11:47Z

matrix/buffer.py

@@ -640,22 +652,43 @@ def _get_prefix_color(prefix):

    def _add_user_to_nicklist(self, user):
        # type: (WeechatUser) -> None
-        nick_pointer = W.nicklist_search_nick(self._ptr, "", user.nick)
+        if user.nick in self.duplicate_nicks:


The disambiguation algorithm works well, great work. Sadly we forgot a crucial feature about it, the room display name depends on user display names if the room is unnamed.

Therefore the algorithm should go into nio, and weechat should ask nio when adding, removing, and changing nicks if there are conflicts. After a user is added, removed or a nick change is performed weechat needs to ask nio for the new room display name and set the buffers short_name property to that (it already does that to an extent).

do you want nio to handle disambiguation in full or just put get_disambiguated_nick into nio? I kinda have a feeling that nio is a bit more low-level to handle that...
Also I still feel it as a bit of controversy, I know that it is recommended by spec but I really want to avoid using fully disambiguated nicks in weechat room buffer (not in nicklist) since it makes output unwieldy or nick get cut by weechat.look.prefix_align_max.

I'm not sure what you mean by full. Anything that is needed to calculate a disambiguated display name and the room name should go into nio. The client of course can pick and chose what it will display to the user. But the client should be able to ask nio for a disambiguated name.

So weechat remembers what it has displayed (it already does that), upon a join/leave or display name change it checks if it would have a conflict and if so asks nio for the disambiguated names.

The room name calculation is inside of nio, I don't see how room member name calculation is any higher or lower level than this. Especially since correct room name calculation depends on the room member names.

If you want to mix and match what gets displayed please make it configurable, I have a feeling that people will have different opinions on that. So 3 modes I guess, what we currently have (user ids without the host part), full display names, and your mixed mode.

Sorry if it wasn't so clear, by full I mean that nio should track collisions and return disambiguated name only in case when there is a clash in a single the room. I understand that for room names this is kinda required.

Frankly speaking if matrix-nio/matrix-nio#4 going to be merged current mode of using part of user_id without host for nicks would be inconsistent with automatic room names. Do you have a suggestion how to make automatic room names configurable in matrix-nio?

Maybe I'm missing something... Could you elaborate on why it would be inconsistent?

In case if you have a chat with account with display name "Foo" and user id @bar:example.org and if matrix-nio/matrix-nio#4 would be merged named_room_name is going to return room name as "Foo" but weechat nicklist going to contain "bar" instead of "Foo" which can be confusing.

The room names are already inconsistent since they use the full user id and the nicklist the short/local part of the user id.

That being said a function that uses user ids instead of display names for room name calculation can be added to nio and the old behavior can be preserved that way. Another function that gives a room name consisting of only the local part of the user id is fine as well.

@gordon-quad me? what?

sorry, github falsly triggered on part of matrix user id @bar:example.org

poljar · 2019-01-25T10:17:29Z

matrix/buffer.py

        tags_type = "message_private" if self.type == "private" else "message"
        tags = self._message_tags(user, tags_type) + (extra_tags or [])
        self._print_message(user, message, date, tags, extra_prefix)

        user.update_speaking_time(date)
-        self.unmask_smart_filtered_nick(nick)
+        if user.host:
+           self.unmask_smart_filtered_nick(underscore_nonalnum(user.host))


You don't need to filter the user_id here, the spec disallows white-space, more info here. nio should take care of events that contain invalid user_ids.

i am more worried about other symbols, like '=' or '@'. Couldn't find in weechat docs if those are allowed so decided not to risk it.

I believe they are fine since the IRC plugin tends to put host names into tags. Any UTF-8 string is fine but the tags get separated by a space, so that's why display names are problematic.

poljar · 2019-01-25T10:21:13Z

matrix/buffer.py

@@ -491,13 +495,13 @@ def _message_tags(self, user, message_type):

        return tags

-    def _get_user(self, nick):
+    def _get_user(self, user_id):


If you're going to merge the classes please separate this into separate commits to ease the reviewing.

poljar · 2019-01-25T10:35:35Z

matrix/buffer.py

@@ -347,6 +355,7 @@ def __init__(self, name, server_name, user):

        self.name = ""
        self.users = {}  # type: Dict[str, WeechatUser]
+        self.duplicate_nicks = {} # type: Dict[str, List[WeechatUser]]


You can use a defaultdict here, that way we can always use duplicate_nicks[nick].append().

gordon-quad · 2019-02-27T10:22:51Z

Slowly trying to finish this PR. Issue is that keeping track of user display name in matrix-nio doesn't help with it in client since if I understand correctly events received during sync are processed in matrix-nio internally (and propagated to the state of matrix-nio room) and only then processed by the client. In this case if client will mindlessly pull display names from matrix-nio room and multiple events of change nick and/or messages received during one sync - this can generate inconsistent logs, for example if following event sequence will be received in one sync:

Alice change display name to Alice1
Alice1 sends message "foobar"
Alice1 change display name to Alice

room.user_name("@alice:example.org") is going to return "Alice" despite that message was sent when Alice had display name "Alice1".

In this case it seems to me that it is still inevitable to for client to keep track of display name changes, unless there is a possibility to process events one-by-one both in matrix-nio and client.

I am not very familiar with the codebase so any advise here would be really helpful.
@poljar

poljar · 2019-02-27T15:30:26Z

There is no way to process events one-by-one.

The way this problem could be solved is for nio to remember the room state transitions for the current sync, then let the user fetch the snapshot of the state with a event_id or a event object.

So for example:

Alice changes the display name to Alice1
Weechat asks nio for the user name for alice via room.user_name("alice:example.org", )

After a new sync is received the old transitions are cleared.

Either that or some amount of duplication inside of Weechat would be fine as well.

Keeping only the latest state in nio is still very useful because the majority of "joins" (m.room.member event with a join membership state) will happen when we login for the first time, that is the joins will be contained in the state part of the sync instead of the timeline. So the state transitions are only important for us if the event is part of the timeline.

poljar · 2019-04-23T16:29:30Z

Ok, a little bit of an less crazy idea landed in nio here. Now we can go through events at the same pace nio does.

But this will probably end up requiring a complete rewrite of the RoomBuffer class.

If you're still interested in this please let me know, otherwise I'll go forward with this in the next month(s).

pinusc · 2019-06-03T15:36:26Z

Hi, what's the state of this pr? I'd be happy to help if necessary, I really want this feature.

poljar · 2019-06-03T16:26:12Z

It's waiting for someone to pick it up and finish the Weechat side of things.

Weechat currently goes through events after nio (the underlying matrix library) does.

Weechat and its RoomBuffer class would need to be modified or even rewritten to utilize nio event callbacks. This way Weechat and nio go through events in sync and we can ask nio for the display name of a user if we need to do so. We would still cache currently used display names in Weechat inside the nicklist.

We would ideally separate the event formatting code into a separate class so that it can be reused in different parts of Weechat.

gordon-quad commented Jan 21, 2019

View reviewed changes

gordon-quad changed the title ~~add an option to use display names in chatrooms~~ [DO NOT MERGE] add an option to use display names in chatrooms Jan 21, 2019

poljar requested changes Jan 21, 2019

View reviewed changes

Gordon Quad added 2 commits January 24, 2019 14:44

/nick command added to change display name

16137df

support for display names instead of accounts in nicklists

c886174

gordon-quad force-pushed the displaynames branch from b05c6a0 to c886174 Compare January 24, 2019 18:09

poljar requested changes Jan 25, 2019

View reviewed changes

poljar reviewed Jan 25, 2019

View reviewed changes

ptman mentioned this pull request Mar 6, 2019

Whatsapp bridge display names #59

Closed

poljar mentioned this pull request Sep 13, 2019

Displayname support #118

Open

poljar mentioned this pull request May 12, 2020

Add an option to show display names of users #189

Open

[DO NOT MERGE] add an option to use display names in chatrooms #28

Are you sure you want to change the base?

[DO NOT MERGE] add an option to use display names in chatrooms #28

Conversation

gordon-quad commented Jan 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gordon-quad commented Jan 21, 2019

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

gordon-quad commented Jan 24, 2019 • 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

gordon-quad Feb 8, 2019 • 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

gordon-quad commented Feb 27, 2019

poljar commented Feb 27, 2019

poljar commented Apr 23, 2019

pinusc commented Jun 3, 2019

poljar commented Jun 3, 2019

gordon-quad commented Jan 24, 2019 •

edited

Loading

gordon-quad Feb 8, 2019 •

edited

Loading