search

hoehermann / purple-signald

Pidgin libpurple bridge to signald.
GNU General Public License v3.0
153 stars 19 forks source link

Implementation of proper group chat support #8

Closed fancypantalons closed 4 years ago

fancypantalons commented 4 years ago

Hey! I decided to take a crack at completely reworking the way libpurple-signald implements group chat support.

The big observation is that, in the libpurple model, the two major forms of conversation are IMs, which are direct 1:1 conversations between two users, and chats, which map more or less to Signal groups.

The trouble with the current implementation is that the plugin maps both types of Signal traffic to IMs, which doesn't take full advantage of the capabilities libpurple offers for group chats.

So in my groupchat branch I completely rewrote the way Signal groups are handled (and also engaged in a ton of refactoring where needed).

With the plugin installed, with Pidgin first starts up, now any existing Signal groups are turned into open chat windows that work as you'd expect.

In addition, you can:

In addition, there's some other behaviours that I've implemented:

To be clear: I don't think the code is ready yet! I've not made it compatible with both purple2 and purple3. It almost certainly needs some hefty code review for memory leaks, bugs, and so forth. And the testing is... well, it needs more testing. :)

But please take a look at let me know what you think!

hoehermann commented 4 years ago

Nice! :1st_place_medal:

I tried to implement basic group chat features in my devel branch, but Pidgin kept crashing on me for no apparent reason. Glad you figured it out.

From my point of view, you do not need worry about purple3 just yet. I do not use Pidgin 3, hence I do not test with Pidgin 3, thus I do not develop against purple3. One day, when Pidgin 3 is actually released, then I want to look into it. :)

I myself am getting sloppy. Neither libpurple-signald nor purple-gowhatsapp have been checked for memory leaks for months. ;)

The libsignald.c is getting quite long. We should probably start separating the code into files (simple chat / group chat / attachment handling).

fancypantalons commented 4 years ago

The libsignald.c is getting quite long. We should probably start separating the code into files (simple chat / group chat / attachment handling).

I couldn't agree more! In this first implementation the refactorings were focused around the changes needed to support these new capabilities while avoiding duplicating code and so forth. I had to fight the urge to not further reorganize the codebase, as it's definitely gotten unwieldy...

I'm happy to take a run at refactoring the project to break it apart in a way that feels natural. Now that the functional work is done, reorganizing it as part of a few, separate refactoring commits is an easy enough next step.

fancypantalons commented 4 years ago

Alright, the refactoring is done. I... have only done the barest of smoke testing, here, after banging this out in a single 4 hour sitting.

There's two commits in the branch. The first one refactors the messaging stuff further to more cleanly break up the code paths.

The second then splits up libsignald.c.

The group chatting stuff, in particular, needs some regression testing...

fancypantalons commented 4 years ago

Just a quick update, I've been actively using the new code for the last few days (and made a subsequent improvement to make the code work well with Pidgin persistent chats) and haven't spotted any issues so I think it's ready for a PR if you're open to these changes.

So let me know how you'd like to proceed!

hoehermann commented 4 years ago

Awesome! Thank you for the heads-up. I want to go through your changes before I merge them. :) It does not matter to me whether you create a pull-request or not – there are not that many contributors. ;)

hoehermann commented 4 years ago

Merged.

fancypantalons commented 4 years ago

Merged.

BTW, I looked at the history, nice couple of catches on regressions, there! Particularly that send/receive message type issue that crept in with the refactoring...

hoehermann commented 4 years ago

The send/receive had actually been there before. That was not you. Your code is very clean. I like it. :)

When I added handling of sync messages, I daftly assumed "SEND" and "RECEIVE" were mutually exclusive. They are not. Pidgin just happens to play nice with it.

fancypantalons commented 4 years ago

The send/receive had actually been there before. That was not you. Your code is very clean. I like it. :)

Thanks, that's very kind of you!

When I added handling of sync messages, I daftly assumed "SEND" and "RECEIVE" were mutually exclusive. They are not. Pidgin just happens to play nice with it.

Well, funny enough, it looks like the signald_process_group_message() inherited the same bug (you can see it in groups.c:237)...

hoehermann commented 4 years ago

it looks like the signald_process_group_message() inherited the same bug

Whoops. Unfortunately, I just managed to quickly skim the code and must have missed it. I want to do a more thorough review, but I am currently unable to do so. I don't want to keep you waiting, so this is as good as it gets for now.

hoehermann commented 4 years ago

By default, I leave a signal group when closing the group chat window. I can change this behaviour by adding the group to the buddy list. However, upon Pidgin start, I automatically re-join the group chat and the group chat window is opened. Can I somehow disable the latter?

fancypantalons commented 4 years ago

Yeah, I need to look into the proper pattern for handling this in Pidgin. This first cut was the drop dead lazy solution just to get it working.

I feel like maybe the force join should only occur when a group message is received?

And I haven't explored yet is how to autopopulate the group chats into the buddy list or something so they're easy to open if a) they're not already open and b) they haven't been manually added to the buddy list.

demure commented 4 years ago

As a User, my 2 cents are that it is desirable to auto join a group chat. If there is some mechanism exposed so that I can tell bitlbee to look for the chan and auto connect, that is acceptable too (I know bitlbee is capable of this for other protocols). In my usecase I would just like to avoid waiting on another to speak in a group chat for it to appear, as I would be sitting there a very long time waiting.

hoehermann commented 4 years ago

The automatic join is fine by me. Just the conversation window annoys me. I don't actually know whether one can join a group chat without the window popping up. Maybe we can steal from the discord plug-in.

fancypantalons commented 4 years ago

So, this gets back to a debate I had with myself about how to deal with group chats in the first place.

libpurple has two mechanisms. The first is a groupchat, which can appear in the buddylist, and is initiated and closed by the user. In the Pidgin frontend they can be flagged as persistent (adding the gtk-persistent boolean attribute to the chat) so that it doesn't trigger a leave event when the chat is closed.

You could implement this in a way that those buddylist chats are auto-created, add the hack to pre-set the gtk-persistent flag, and then handle things properly in the backend (not worth getting into the details, it suffices to say you can make this work). This isn't how the plugin works today, but it's certainly doable (I think).

The problem is, as far as I'm aware, bitlbee doesn't support listing those buddy list group chat entries as IRC channels. So if the plugin didn't force join, there'd be no way to list them and no way to join them. Boo-urns.

The other mechanism in libpurple is the roomlist. The plugin could populate the roomlist with the set of known group chats, and bitlbee does know how to show that roomlist and let you join them.

The trouble, here, is that as far as I know there's no way, from the client side, to delete a room! They're intended to be a server-side construct. So I'm not sure how you could let the user leave a group chat if you use the room mechanism.

So I'm toying with a solution where the plugin:

  1. Auto-populates group chats into the buddy list with the gtk-persistent flag set as the default (which is a minor hack since it's mucking with the internals in Pidgin) so that folks don't accidentally leave Signal groups when they close the chat window, and then
  2. Includes a setting which controls how group chats behave, and I could see three modes:
    1. Never automatically open.
    2. Automatically open upon receiving a message.
    3. Always automatically open (this is the current behaviour).
hoehermann commented 4 years ago

For me, offering option ii somewhere would be sufficient. The gtk-persistent flag seems a hassle. Unless you really want to, I suggest not spending time on that one.

fancypantalons commented 4 years ago

For me, offering option ii somewhere would be sufficient. The gtk-persistent flag seems a hassle. Unless you really want to, I suggest not spending time on that one.

Well, speaking for myself, before I added support for handling chat persistence, I found myself reflexively closing Pidgin chat windows while testing and accidentally leaving Signal groups.

Now, that'd be fine in another chat protocol, but with Signal, that's an irreversible operation unless someone re-invites you to the chat... I actually ended up leaving some group chats with friends and had to ask them to invite me back in! :(

hoehermann commented 4 years ago

I noticed that, too. Maybe we could just do nothing in signald_chat_leave? By default unless "group leaving" is enabled in the settings?

fancypantalons commented 4 years ago

So if you actually wanted to leave a group chat you'd have to change the account setting, close the chat, then change the setting back? That feels... cumbersome to me.

The nice thing about the persistence flag is it uses an existing, established mechanism in Pidgin to support exactly this idea. i.e., a chat that you're part of, but the chat isn't open all the time, and if you close the window you remain a part of the chat. If you want to actually leave, you edit the chat settings, turn off the "Persistent" flag, then leave the chat and delete it from the buddy list (which... makes me wonder if actually it should be automatically deleted from the buddy list so the user clearly sees that the chat is truly gone from Signal's perspective... but that's a whole other thing).

So it feel like it's the right mechanism, and i don't think it's a huge issue setting that flag programmatically (well... in theory, I haven't actually implemented it yet!) other than it's an internal Pidgin flag that I only know about because I went spelunking in the codebase.

hoehermann commented 4 years ago

So if you actually wanted to leave a group chat you'd have to change the account setting, close the chat, then change the setting back?

Indeed. Though this is 100 % me thinking "that would work for me and is the easiest method I know of". This is also the reason why this project needed your support for the group chats. ;)