Join by alias or id #29
Conversation
matrix-api.c
Outdated
| } | ||
|
|
||
| if(errcode != NULL && error != NULL) { | ||
| if(errcode && error) { |
There was a problem hiding this comment.
https://github.com/matrix-org/purple-matrix/pull/27/files#r95143880 referred to this as much as line 70.
I won't insist on you changing it back, but I would prefer to keep the explicit != NULL.
matrix-api.c
Outdated
| @@ -310,8 +314,10 @@ static void matrix_api_complete(PurpleUtilFetchUrlData *url_data, | |||
| } else if(response_code >= 300) { | |||
| purple_debug_info("matrixprpl", "API gave response %i\n", | |||
| response_code); | |||
| (data->bad_response_callback)(data->conn, data->user_data, | |||
| response_code, root); | |||
| if (data && data->conn && data->bad_response_callback) { | |||
There was a problem hiding this comment.
I'm slightly mystified by this change. I don't think there is any way any of these can be null, and it seems an odd place to sanity-check them. Also, it's not obvious that swallowing the error is the correct behaviour if any of them do end up as NULL.
Am I missing something?
matrix-api.c
Outdated
| @@ -538,7 +543,7 @@ static MatrixApiRequestData *matrix_api_start_full(const gchar *url, | |||
| /* we couldn't start the request. In this case, our callback will | |||
| * already have been called, which will have freed data. | |||
| */ | |||
| data = NULL; | |||
| data->purple_data = NULL; | |||
There was a problem hiding this comment.
errrr... either the comment just above is wrong (in which case, it needs fixing, and the condition if(purple_data == NULL) is redundant), or this will segfault. Either way, this is not a good change.
libmatrix.c
Outdated
| matrix_connection_join_room(gc, room, components); | ||
| if (!conv) { | ||
| /* get the room by alias, if possible - join it, otherwise for now, do nothing */ | ||
| matrix_api_get_roomid_by_alias(conn, room_alias, room_join_callback, NULL, NULL, components); |
There was a problem hiding this comment.
why not just try to join the room, rather than doing the lookup first?
matrix-api.h
Outdated
|
|
||
|
|
||
| /** | ||
| * Get roomid by alias |
There was a problem hiding this comment.
This still needs much better documentation.
There was a problem hiding this comment.
Also, I'd suggest you put it among the other functions which hit a specific endpoint. Why not put it after matrix_api_sync, which would be consistent with where you have put it in the C?
matrix-connection.c
Outdated
| while (g_hash_table_iter_next (&iter, &key, &value)) { | ||
| g_hash_table_insert(copy, g_strdup(key), g_strdup(value)); | ||
| } | ||
| /* Assume old components is corrupt */ |
matrix-connection.c
Outdated
| } | ||
| /* Assume old components is corrupt */ | ||
| newcomponents = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, g_free); | ||
| g_hash_table_insert(newcomponents, PRPL_CHAT_INFO_ROOM_ID, g_strdup(room)); |
There was a problem hiding this comment.
I'm still unclear why you want to change this. We used to take a copy of the components table, so that _join_failed could pass it to purple_serv_got_join_chat_failed without worrying about exactly what was supposed to be in there.
Please explain why this is better?
matrix-connection.c
Outdated
| room_id = matrix_json_object_get_string_member(root_obj, "room_id"); | ||
| if (room_id && *room_id) { | ||
| // Now that we have the actual room id, join it, then create conv! | ||
| matrix_api_join_room(conn, room_id, _tell_purple_joined_callback, NULL, NULL, user_data); |
There was a problem hiding this comment.
You're still calling matrix_api_join_room twice. That is not correct.
matrix-connection.c
Outdated
|
|
||
| g_hash_table_destroy(components); | ||
| if (room_id && *room_id) { | ||
| matrix_room_join_conversation(conn->pc, room_id); |
There was a problem hiding this comment.
This should be unnecessary, because the response to /sync will tell us we have joined the room. Why is it necessary?
matrix-room.c
Outdated
| @@ -865,6 +865,35 @@ void matrix_room_handle_timeline_event(PurpleConversation *conv, | |||
| g_free(escaped_body); | |||
| } | |||
|
|
|||
| PurpleConversation *matrix_room_join_conversation( | |||
There was a problem hiding this comment.
As per my comments above, I think this us unnecessary. But even if it's not, most if this is C&Ped from matrix_room_create_conversation. Please try to avoid c&ping code.
Latest commit below c843db3 is a trivial one liner and fixes join by alias / room-id
Signed-off-by: Damien Zammit [email protected]