GNOME Bugzilla – Bug 748703
Private chats throw errors
Last modified: 2015-06-02 13:31:51 UTC
This is fallout from bug 710792 - the patches there assume that all channels have group functionality, which is not the case for private conversations: (org.gnome.Polari:31787): Gjs-WARNING **: JS ERROR: TypeError: members is null ChatView<._onChannelChanged@resource:///org/gnome/Polari/js/chatView.js:607 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 _ChatroomManager<._ensureRoomForChannel@resource:///org/gnome/Polari/js/chatroomManager.js:198 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 _ChatroomManager<.handleChannels/<@resource:///org/gnome/Polari/js/chatroomManager.js:249 _ChatroomManager<._processRequest@resource:///org/gnome/Polari/js/chatroomManager.js:213 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 _ChatroomManager<.handleChannels@resource:///org/gnome/Polari/js/chatroomManager.js:245 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 Client<.vfunc_handle_channels@resource:///org/gnome/Polari/js/chatroomManager.js:38 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 main@resource:///org/gnome/Polari/js/main.js:17 run@resource:///org/gnome/gjs/modules/package.js:192 start@resource:///org/gnome/gjs/modules/package.js:176 @/home/fmuellner/opt/gnome/bin/polari:5 (org.gnome.Polari:31787): Gjs-WARNING **: JS ERROR: TypeError: null has no properties ChatView<._insertPendingLogs@resource:///org/gnome/Polari/js/chatView.js:323 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 ChatView<._onLogEventsReady@resource:///org/gnome/Polari/js/chatView.js:280 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 main@resource:///org/gnome/Polari/js/main.js:17 run@resource:///org/gnome/gjs/modules/package.js:192 start@resource:///org/gnome/gjs/modules/package.js:176 @/home/fmuellner/opt/gnome/bin/polari:5
Created attachment 302685 [details] [review] chatView: setNickStatus only for group chats just testing the waters. is this pretty?
(In reply to Bastian from comment #1) > just testing the waters. is this pretty? No. Either check whether the channel has the group interface, or look at the room type. (Also eventually we need to do better than simply ignoring private conversations, see bug 712635)
Created attachment 302765 [details] [review] chatView: setNickStatus only for group chats I am checking the HandleType of the channel now.
Created attachment 302766 [details] [review] chatView: setNickStatus only for group chats woops, looks like i rushed that previous one a bit.
Review of attachment 302766 [details] [review]: Apart from the style nits outlined below, maybe we should try to do a bit better than outright ignoring private conversations - it shouldn't be too hard actually, considering that we *know* the list of members in that case: there are exactly two users in the "room", this._channel.connection.self_contact and this._channel.target_contact ... If we then fix bug 712635 to detect when the target contact disconnects, everything should work as expected. ::: src/chatView.js @@ +323,3 @@ + + let channelHandle = this._channel.get_handle(); + if (channelHandle[1] == Tp.HandleType.ROOM) { PolariRoom duplicates the type property (because we need to differentiate between ROOM and CONTACT even when unconnected, i.e. without a channel), so this can just be if (this._room.type == Tp.HandleType.ROOM) @@ +326,2 @@ let members = this._channel.group_dup_members_contacts(); + for (let j = 0; j < members.length; j++) { Unrelated change
Created attachment 304183 [details] [review] chatView.js: color contact availability in PM rooms The patch now handles the private messaging room case as well. However, Bug 712635 still remains to be solved before coloring would be of any help in there.
Review of attachment 304183 [details] [review]: ::: src/chatView.js @@ +323,3 @@ + + let channelHandle = this._channel.get_handle(); + if (channelHandle[1] == Tp.HandleType.ROOM) { I don't like that stylistically - the meaning of "channelHandle[1]" is totally obscure here. There are two better (IMHO) options: (1) Use: let [handle, type] = this._channel.get_handle(); if (type == ...) (2) Use: if (this._room.type == ...) @@ +614,3 @@ + let channelHandle = this._channel.get_handle(); + if (channelHandle[1] == Tp.HandleType.ROOM) { Dto.
Created attachment 304388 [details] [review] chatView.js: color contact availability in PM rooms
Review of attachment 304388 [details] [review]: Code looks good to me now, thanks. A couple of nicks on the commit message: Prefix without extension please, i.e. "chatView" rather than "chatView.js"; the following subject line should use sentence capitalization. Regarding the body, it's a bit on the vague side as-is - I'd mention that the original commit used API that is only available on MUCs unconditionally, resulting in errors in PM rooms. I'd either leave the last sentence out, or replace it with something like: "We currently don't have a way to check the actual online status of a PM message, so always mark it as available."
Created attachment 304426 [details] [review] chatView: Color contact availability in PM rooms Now reads: "Commit c96929c uses an API which is only available in group chat rooms, resulting in errors in private message rooms. We currently don't have a way to check the actual online status of a PM message, so always mark it as available."
Review of attachment 304426 [details] [review]: (In reply to Bastian from comment #10) > Now reads: > > [...] Fine with me, thanks!
Pushed to master https://git.gnome.org/browse/polari/commit/?id=75ae34c78dcf5f70fc9a2d99147d588d458a1fe3