slack-libpurple
slack-libpurple copied to clipboard
dylex
Usernames from shared channels not shown
The usernames from shared channels get shown as something like "U0KDV117F" or "U9E349V2T" and there user list for the shared channel is empty.
I can provide some debug information, if needed.
Can you clarify what you mean by shared channels? Are these just regular public channels? It sounds like the user list isn't getting loaded for some reason. Are there a very large number of users in the slack? Do usernames show up anywhere (like buddy list or IMs)?
@dylex, the shared channels are channels shared between two different workspaces (for example, two companies that work together create one of these to be able to work together).
AFAIK it's still in beta, an explanation can be found at https://get.slack.help/hc/en-us/articles/115004151203-Create-shared-channels-on-a-workspace-beta- .
If you plan on supporting this, I can see to linking two of the workspaces I participate in and provide you with accounts (or I can connect one to one of yours).
I see... They don't seem to have integrated this into their main api docs yet, but I see some mentions. This seems like a large feature, so I can't promise I'll make much progress on supporting it (though I'd happily accept a PR). However, some other things I've been meaning to do to support bots (#34) and unknown IDs (#22) look like they may provide some help with this anyway, at least to display user names. Let's see how things look once I get through with those, if that makes sense.
With the current version of things, shared channels have a significant number of issues:
- The member list is empty.
- Foreign chat members have encoded names rather than display names.
- Direct messages sent by foreign users do not arrive.
I'm happy to investigate and/or test things if you have some idea where to look.
I took the brute force and ignorance approach and did a lookup for each unknown user in a shared channel. Works fine on our relatively small channel but maybe it doesn't scale to bigger numbers of users.
Sorry, I can't be arsed to learn how Git works, so here's an old-fashioned patch.
--- a/slack-channel.c 2020-10-29 09:36:16.000000000 -0400
+++ b/slack-channel.c 2021-02-18 21:51:13.286513747 -0500
@@ -143,6 +143,21 @@
g_free(join);
}
+struct channel_retrieve_user {
+ PurpleConvChat *conv;
+ const char *creator;
+};
+
+static void channel_retrieve_user_cb(SlackAccount *sa, gpointer data, SlackUser *user) {
+ struct channel_retrieve_user *lookup = data;
+ PurpleConvChatBuddyFlags flag = PURPLE_CBFLAGS_VOICE;
+
+ if (slack_object_id_is(user->object.id, lookup->creator))
+ flag |= PURPLE_CBFLAGS_FOUNDER;
+ purple_conv_chat_add_user(lookup->conv, user->object.name, NULL, flag, FALSE);
+ g_free(lookup);
+}
+
static gboolean channels_info_cb(SlackAccount *sa, gpointer data, json_value *json, const char *error) {
SlackChannelType type = GPOINTER_TO_INT(data);
json = json_get_prop_type(json, type >= SLACK_CHANNEL_GROUP ? "group" : "channel", object);
@@ -169,10 +184,20 @@
json_value *members = json_get_prop_type(json, "members", array);
if (members) {
GList *users = NULL, *flags = NULL;
+ gboolean is_shared = json_get_prop_boolean(json, "is_shared", FALSE);
for (unsigned i = members->u.array.length; i; i --) {
- SlackUser *user = (SlackUser*)slack_object_hash_table_lookup(sa->users, json_get_strptr(members->u.array.values[i-1]));
- if (!user)
+ const char *user_id = json_get_strptr(members->u.array.values[i-1]);
+ SlackUser *user = (SlackUser*)slack_object_hash_table_lookup(sa->users, user_id);
+ if (!user) {
+ /* on a shared channel there may be users from other teams we haven't seen yet */
+ if (is_shared) {
+ struct channel_retrieve_user *lookup = g_new(struct channel_retrieve_user, 1);
+ lookup->conv = conv;
+ lookup->creator = creator;
+ slack_user_retrieve(sa, user_id, channel_retrieve_user_cb, lookup);
+ }
continue;
+ }
users = g_list_prepend(users, user->object.name);
PurpleConvChatBuddyFlags flag = PURPLE_CBFLAGS_VOICE;
if (slack_object_id_is(user->object.id, creator))
Now handles member joins after a channel join.
--- a/slack-channel.c 2020-10-29 09:36:16.000000000 -0400
+++ b/slack-channel.c 2021-02-20 14:42:39.674321135 -0500
@@ -143,6 +143,21 @@
g_free(join);
}
+struct channel_retrieve_user {
+ PurpleConvChat *conv;
+ const char *creator;
+};
+
+static void channel_retrieve_user_cb(SlackAccount *sa, gpointer data, SlackUser *user) {
+ struct channel_retrieve_user *lookup = data;
+ PurpleConvChatBuddyFlags flag = PURPLE_CBFLAGS_VOICE;
+
+ if (slack_object_id_is(user->object.id, lookup->creator))
+ flag |= PURPLE_CBFLAGS_FOUNDER;
+ purple_conv_chat_add_user(lookup->conv, user->object.name, NULL, flag, FALSE);
+ g_free(lookup);
+}
+
static gboolean channels_info_cb(SlackAccount *sa, gpointer data, json_value *json, const char *error) {
SlackChannelType type = GPOINTER_TO_INT(data);
json = json_get_prop_type(json, type >= SLACK_CHANNEL_GROUP ? "group" : "channel", object);
@@ -169,10 +184,20 @@
json_value *members = json_get_prop_type(json, "members", array);
if (members) {
GList *users = NULL, *flags = NULL;
+ gboolean is_shared = json_get_prop_boolean(json, "is_shared", FALSE);
for (unsigned i = members->u.array.length; i; i --) {
- SlackUser *user = (SlackUser*)slack_object_hash_table_lookup(sa->users, json_get_strptr(members->u.array.values[i-1]));
- if (!user)
+ const char *user_id = json_get_strptr(members->u.array.values[i-1]);
+ SlackUser *user = (SlackUser*)slack_object_hash_table_lookup(sa->users, user_id);
+ if (!user) {
+ /* on a shared channel there may be users from other teams we haven't seen yet */
+ if (is_shared) {
+ struct channel_retrieve_user *lookup = g_new(struct channel_retrieve_user, 1);
+ lookup->conv = conv;
+ lookup->creator = creator;
+ slack_user_retrieve(sa, user_id, channel_retrieve_user_cb, lookup);
+ }
continue;
+ }
users = g_list_prepend(users, user->object.name);
PurpleConvChatBuddyFlags flag = PURPLE_CBFLAGS_VOICE;
if (slack_object_id_is(user->object.id, creator))
@@ -334,12 +359,21 @@
if (!conv)
return;
+ const char *id = json_get_prop_strptr(json, "team");
const char *user_id = json_get_prop_strptr(json, "user");
SlackUser *user = (SlackUser*)slack_object_hash_table_lookup(sa->users, user_id);
if (joined) {
- PurpleConvChatBuddyFlags flag = PURPLE_CBFLAGS_VOICE;
- /* TODO we don't know creator here */
- purple_conv_chat_add_user(conv, user ? user->object.name : user_id, NULL, flag, TRUE);
+ if (!user && g_strcmp0(sa->team.id, id)) {
+ /* look up unknown user from a different team */
+ struct channel_retrieve_user *lookup = g_new(struct channel_retrieve_user, 1);
+ lookup->conv = conv;
+ lookup->creator = NULL;
+ slack_user_retrieve(sa, user_id, channel_retrieve_user_cb, lookup);
+ } else {
+ PurpleConvChatBuddyFlags flag = PURPLE_CBFLAGS_VOICE;
+ /* TODO we don't know creator here */
+ purple_conv_chat_add_user(conv, user ? user->object.name : user_id, NULL, flag, TRUE);
+ }
} else
purple_conv_chat_remove_user(conv, user ? user->object.name : user_id, NULL);
}
This is great. The only issue is that it may generate a number of requests in parallel, especially for channels with lots of external users, which the slack API doesn't like (and starts rejecting requests).
This comes up often enough I'm wondering if we should solve it at a lower level, having the slack api layer serialize requests itself, which would make it easier to write code like this. I'll try to take a look and think about doing this.
You also need to strdup creator, as it's part of the response, so will be invalid when the response comes back. (Or just leave this out -- its seems unlikely to me that the creator will be an external user, and if so, maybe we don't care anyway.) Also probably need to make sure the conversation is still valid somehow...
Yes, it definitely makes a lot of user lookup calls-- hence my comment about it probably not scaling. The shared channel I'm now on (which prompted this patch) only has about three dozen "foreign" users, so this does fix my immediate problem.
Good catch on forgetting to g_strdup the creator-- fixed. Turns out the creator on my shared channel is external, so it is actually useful. :) Probably not worth figuring out the creator on a channel join as it seems unlikely a creator would rejoin their own channel. Anyway, I think they will show up as the creator again if the chat window is closed and reopened.
This patch now passes the channel id and looks up the conversation from that instead of passing the conversation directly, which I think addresses the "conversation is still valid" problem. It also prints a "@XXXXXXX is now foo" message in the chat window on a join so you know who the heck it was.
Finally, the stanza at the top (rather clumsily) fixes a crash in slack_blist_uncache() when the user removes a chat from the buddy list using the GUI and then leaves the channel via the Slack web interface. Possibly not worth fixing, but after the second time I crashed Pidgin by doing that...
--- slack-channel.c.orig 2020-10-29 09:36:16.000000000 -0400
+++ slack-channel.c 2021-02-21 17:51:11.987996428 -0500
@@ -40,11 +40,18 @@
g_hash_table_remove(sa->channel_cids, GUINT_TO_POINTER(chan->cid));
chan->cid = 0;
}
- if (chan->object.buddy) {
+ /* ensure user hasn't already removed chat from buddy list via the UI */
+ PurpleBlistNode *node;
+ for (node = purple_blist_get_root(); node; node = purple_blist_node_next(node, TRUE)) {
+ if (chan->object.buddy == node)
+ break;
+ }
+ if (node) {
+ /* XXX we leak cache if the user already removed the chat */
slack_blist_uncache(sa, chan->object.buddy);
purple_blist_remove_chat(channel_buddy(chan));
- chan->object.buddy = NULL;
}
+ chan->object.buddy = NULL;
}
SlackChannel *slack_channel_set(SlackAccount *sa, json_value *json, SlackChannelType type) {
@@ -143,6 +150,36 @@
g_free(join);
}
+struct channel_retrieve_user {
+ slack_object_id cid;
+ gchar *creator;
+ gboolean joined;
+};
+
+static void channel_retrieve_user_cb(SlackAccount *sa, gpointer data, SlackUser *user) {
+ struct channel_retrieve_user *lookup = data;
+ SlackChannel *chan = g_hash_table_lookup(sa->channels, lookup->cid);
+ PurpleConvChat *conv = NULL;
+
+ if (chan)
+ conv = slack_channel_get_conversation(sa, chan);
+ if (conv) {
+ PurpleConvChatBuddyFlags flag = PURPLE_CBFLAGS_VOICE;
+ if (slack_object_id_is(user->object.id, lookup->creator))
+ flag |= PURPLE_CBFLAGS_FOUNDER;
+ purple_conv_chat_add_user(conv, user->object.name, NULL, flag, FALSE);
+ if (lookup->joined) {
+ char *msg = g_strdup_printf("@%s is now %s", user->object.id, user->object.name);
+ purple_conversation_write(conv->conv, NULL, msg, PURPLE_MESSAGE_SYSTEM | PURPLE_MESSAGE_NO_LINKIFY, time(NULL));
+ purple_debug_info("slack", "%s\n", msg);
+ g_free(msg);
+ }
+ }
+ if (lookup->creator)
+ g_free(lookup->creator);
+ g_free(lookup);
+}
+
static gboolean channels_info_cb(SlackAccount *sa, gpointer data, json_value *json, const char *error) {
SlackChannelType type = GPOINTER_TO_INT(data);
json = json_get_prop_type(json, type >= SLACK_CHANNEL_GROUP ? "group" : "channel", object);
@@ -169,10 +206,21 @@
json_value *members = json_get_prop_type(json, "members", array);
if (members) {
GList *users = NULL, *flags = NULL;
+ gboolean is_shared = json_get_prop_boolean(json, "is_shared", FALSE);
for (unsigned i = members->u.array.length; i; i --) {
- SlackUser *user = (SlackUser*)slack_object_hash_table_lookup(sa->users, json_get_strptr(members->u.array.values[i-1]));
- if (!user)
+ const char *user_id = json_get_strptr(members->u.array.values[i-1]);
+ SlackUser *user = (SlackUser*)slack_object_hash_table_lookup(sa->users, user_id);
+ if (!user) {
+ /* on a shared channel there may be users from other teams we haven't seen yet */
+ if (is_shared) {
+ struct channel_retrieve_user *lookup = g_new(struct channel_retrieve_user, 1);
+ slack_object_id_copy(lookup->cid, chan->object.id);
+ lookup->creator = g_strdup(creator);
+ lookup->joined = FALSE;
+ slack_user_retrieve(sa, user_id, channel_retrieve_user_cb, lookup);
+ }
continue;
+ }
users = g_list_prepend(users, user->object.name);
PurpleConvChatBuddyFlags flag = PURPLE_CBFLAGS_VOICE;
if (slack_object_id_is(user->object.id, creator))
@@ -334,12 +382,22 @@
if (!conv)
return;
+ const char *id = json_get_prop_strptr(json, "team");
const char *user_id = json_get_prop_strptr(json, "user");
SlackUser *user = (SlackUser*)slack_object_hash_table_lookup(sa->users, user_id);
if (joined) {
- PurpleConvChatBuddyFlags flag = PURPLE_CBFLAGS_VOICE;
/* TODO we don't know creator here */
- purple_conv_chat_add_user(conv, user ? user->object.name : user_id, NULL, flag, TRUE);
+ if (!user && g_strcmp0(sa->team.id, id)) {
+ /* look up unknown user from a different team */
+ struct channel_retrieve_user *lookup = g_new(struct channel_retrieve_user, 1);
+ slack_object_id_copy(lookup->cid, chan->object.id);
+ lookup->creator = NULL;
+ lookup->joined = TRUE;
+ slack_user_retrieve(sa, user_id, channel_retrieve_user_cb, lookup);
+ } else {
+ PurpleConvChatBuddyFlags flag = PURPLE_CBFLAGS_VOICE;
+ purple_conv_chat_add_user(conv, user ? user->object.name : user_id, NULL, flag, TRUE);
+ }
} else
purple_conv_chat_remove_user(conv, user ? user->object.name : user_id, NULL);
}
On the serialize branch, I've changed the api interface to only do one request at a time, so sending out many requests like this should now be safe. I'll merge this in after a bit more testing, but feel free to use it now (it shouldn't conflict with any of this).