From 4905dbbe6ce81c092f3f05b68fd489694f8ff080 Mon Sep 17 00:00:00 2001 From: Jaidyn Ann Date: Sun, 18 Jul 2021 17:52:36 -0500 Subject: [PATCH] Redesign add-on disconnection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, add-ons are disconnected when ChatProtocol::Shutdown() is called, which the add-on can do by itself― but there is no standard way for add-ons to notify the app about their Shutdown. Because of this, they tend to not call Shutdown()― instead (as in the case of the Jabber add-on), they display a BAlert (IM_ERROR) notifying the user of the connection error, but the account is considered active by Cardie (and its threads are still existant, including its ProtocolLooper). Zombies are bad, so this is redesigned somewhat with this commit: Protocols should no longer call ChatProtocol::Shutdown() themselves, they must send an IM_MESSAGE of IM_PROTOCOL_DISABLE to the app. This will delete its ProtocolLooper, which in turn will send a notification to the user and delete the ChatProtocol, and so calling ChatProtocol::Shutdown(). In the included protocols, an IM_ERROR is sent right before IM_PROTOCOL_DISABLE is sent if due to a connection error. This is not required, but it is courteous to inform your user about the "why." :) --- application/AppMessages.h | 2 +- application/ChatProtocolMessages.h | 13 ++-- application/ProtocolLooper.cpp | 9 +++ application/Server.cpp | 69 +++++++++++-------- application/Server.h | 3 + .../preferences/PreferencesAccounts.cpp | 5 +- application/windows/MainWindow.cpp | 2 +- protocols/irc/IrcProtocol.cpp | 13 +++- protocols/purple/PurpleApp.cpp | 5 +- protocols/purple/PurpleMessages.h | 8 ++- protocols/purple/PurpleProtocol.cpp | 11 ++- protocols/xmpp/JabberHandler.cpp | 7 +- 12 files changed, 97 insertions(+), 50 deletions(-) diff --git a/application/AppMessages.h b/application/AppMessages.h index efba792..dc8d43d 100644 --- a/application/AppMessages.h +++ b/application/AppMessages.h @@ -48,7 +48,7 @@ const uint32 APP_MOVE_UP = 'CYmu'; const uint32 APP_MOVE_DOWN = 'CYmd'; //! Disable a given account -const uint32 APP_DISABLE_ACCOUNT = 'CYda'; +const uint32 APP_ACCOUNT_DISABLED = 'CYda'; //! Request a "help" message const uint32 APP_REQUEST_HELP = 'CYhm'; diff --git a/application/ChatProtocolMessages.h b/application/ChatProtocolMessages.h index ffa8689..658c698 100644 --- a/application/ChatProtocolMessages.h +++ b/application/ChatProtocolMessages.h @@ -412,12 +412,13 @@ enum im_what_code { Should be sent after connection is established, initialization done */ IM_PROTOCOL_READY = 1002, - /*! Account has just been disabled →App - Sent as the account disconnects, and will not be automatically - reconnected on the add-on's side. - Should be sent in two cases: When connection fails due to an error, - or when account has been closed by user (ChatProtocol::Shutdown()) */ - IM_PROTOCOL_DISABLED = 1003 + /*! Deletion of protocol requested →App + This requests that the app delete the ChatProtocol and its + ProtocolLooper― so invoking ChatProtocol::Shutdown(). + This should be sent by the protocol after connection errors or a + disconnect, when the addon doesn't have anything left to do (other + than yearn for the sweet hand of death). */ + IM_PROTOCOL_DISABLE = 1003 }; #endif // _CHAT_PROTOCOL_MESSAGES_H diff --git a/application/ProtocolLooper.cpp b/application/ProtocolLooper.cpp index 9e32291..fab72e2 100644 --- a/application/ProtocolLooper.cpp +++ b/application/ProtocolLooper.cpp @@ -12,9 +12,11 @@ #include "ProtocolLooper.h" +#include #include #include "Account.h" +#include "AppMessages.h" #include "Conversation.h" #include "ConversationAccountItem.h" @@ -42,6 +44,13 @@ ProtocolLooper::ProtocolLooper(ChatProtocol* protocol, int64 instance) ProtocolLooper::~ProtocolLooper() { + BMessage* msg = new BMessage(APP_ACCOUNT_DISABLED); + BBitmap* icon = fProtocol->Icon(); + + icon->Archive(msg); + msg->AddString("name", fProtocol->GetName()); + fProtocol->MessengerInterface()->SendMessage(msg); + fProtocol->Shutdown(); delete fProtocol; } diff --git a/application/Server.cpp b/application/Server.cpp index 3fe4f68..aa26498 100644 --- a/application/Server.cpp +++ b/application/Server.cpp @@ -106,6 +106,12 @@ Server::Filter(BMessage* message, BHandler **target) filter_result result = B_DISPATCH_MESSAGE; switch (message->what) { + case IM_MESSAGE: + result = ImMessage(message); + break; + case IM_ERROR: + ImError(message); + break; case APP_CLOSE_CHAT_WINDOW: { BString id = message->FindString("chat_id"); @@ -116,12 +122,24 @@ Server::Filter(BMessage* message, BHandler **target) result = B_SKIP_MESSAGE; break; } - case IM_MESSAGE: - result = ImMessage(message); - break; - case IM_ERROR: - ImError(message); + case APP_ACCOUNT_DISABLED: + { + BString name; + int64 instance; + if (message->FindInt64("instance", &instance) != B_OK) + return result; + + // "Whoops" notification + if (AppPreferences::Item()->NotifyProtocolStatus == true + && message->FindString("name", &name) == B_OK) { + BBitmap* icon = new BBitmap(message); + BString content("%user% has been disabled!"); + content.ReplaceAll("%user%", name); + + _SendNotification("Disabled", content, icon); + } break; + } case APP_REPLICANT_MESSENGER: { BMessenger* messenger = new BMessenger(); @@ -138,18 +156,6 @@ Server::Filter(BMessage* message, BHandler **target) accountManager->ReplicantStatusNotify(accountManager->Status()); break; } - - case APP_DISABLE_ACCOUNT: - { - int64 instance = 0; - if (message->FindInt64("instance", &instance) != B_OK) { - result = B_SKIP_MESSAGE; - break; - } - RemoveProtocolLooper(instance); - break; - } - case APP_USER_INFO: { User* user = _EnsureUser(message); @@ -159,7 +165,6 @@ Server::Filter(BMessage* message, BHandler **target) } break; } - case APP_REQUEST_HELP: { BString body; @@ -200,12 +205,10 @@ Server::Filter(BMessage* message, BHandler **target) chat->ImMessage(help); break; } - default: // Dispatch not handled messages to main window break; } - return result; } @@ -610,12 +613,14 @@ Server::ImMessage(BMessage* msg) } break; } - case IM_PROTOCOL_DISABLED: + case IM_PROTOCOL_DISABLE: { - // "Whoops" notification - if (AppPreferences::Item()->NotifyProtocolStatus == true) - _ProtocolNotification(_LooperFromMessage(msg), - BString("Disabled"), BString("%user% has been disabled!")); + int64 instance = 0; + if (msg->FindInt64("instance", &instance) != B_OK) { + result = B_SKIP_MESSAGE; + break; + } + RemoveProtocolLooper(instance); break; } default: @@ -983,12 +988,20 @@ Server::_ProtocolNotification(ProtocolLooper* looper, BString title, if (looper == NULL || title.IsEmpty() == true) return; title.ReplaceAll("%user%", looper->Protocol()->GetName()); desc.ReplaceAll("%user%", looper->Protocol()->GetName()); + _SendNotification(title, desc, looper->Protocol()->Icon(), type); +} + +void +Server::_SendNotification(BString title, BString content, BBitmap* icon, + notification_type type) +{ BNotification notification(type); notification.SetGroup(BString(APP_NAME)); notification.SetTitle(title); - if (desc.IsEmpty() == false) - notification.SetContent(desc); - notification.SetIcon(looper->Protocol()->Icon()); + if (content.IsEmpty() == false) + notification.SetContent(content); + if (icon != NULL) + notification.SetIcon(icon); notification.Send(); } diff --git a/application/Server.h b/application/Server.h index 6d63836..ebeea70 100644 --- a/application/Server.h +++ b/application/Server.h @@ -80,6 +80,9 @@ private: void _ProtocolNotification(ProtocolLooper* looper, BString title, BString desc, notification_type type=B_INFORMATION_NOTIFICATION); + void _SendNotification(BString title, BString content, + BBitmap* icon=NULL, + notification_type type=B_INFORMATION_NOTIFICATION); void _ReplicantStatusNotify(UserStatus status); diff --git a/application/preferences/PreferencesAccounts.cpp b/application/preferences/PreferencesAccounts.cpp index aeed3d9..594821a 100644 --- a/application/preferences/PreferencesAccounts.cpp +++ b/application/preferences/PreferencesAccounts.cpp @@ -18,8 +18,8 @@ #include "AccountDialog.h" #include "AccountListItem.h" -#include "AppMessages.h" #include "ChatProtocol.h" +#include "ChatProtocolMessages.h" #include "PreferencesAccounts.h" #include "ProtocolManager.h" #include "ProtocolSettings.h" @@ -197,7 +197,8 @@ PreferencesAccounts::MessageReceived(BMessage* msg) if (found == false) return; - BMessage* remove = new BMessage(APP_DISABLE_ACCOUNT); + BMessage* remove = new BMessage(IM_MESSAGE); + remove->AddInt32("im_what", IM_PROTOCOL_DISABLE); remove->AddInt64("instance", instance); ((TheApp*)be_app)->GetMainWindow()->PostMessage(remove); diff --git a/application/windows/MainWindow.cpp b/application/windows/MainWindow.cpp index 6a6a097..be502a6 100644 --- a/application/windows/MainWindow.cpp +++ b/application/windows/MainWindow.cpp @@ -205,7 +205,7 @@ MainWindow::MessageReceived(BMessage* message) } break; } - case APP_DISABLE_ACCOUNT: + case APP_ACCOUNT_DISABLED: _ToggleMenuItems(); break; case IM_MESSAGE: diff --git a/protocols/irc/IrcProtocol.cpp b/protocols/irc/IrcProtocol.cpp index 3a07bd6..bb6f05b 100644 --- a/protocols/irc/IrcProtocol.cpp +++ b/protocols/irc/IrcProtocol.cpp @@ -58,14 +58,21 @@ connect_thread(void* data) if (irc_connect(session, joinServer.String(), port, password, nick, ident, real_name)) { - printf("Could not connect: %s\n", irc_strerror (irc_errno(session))); + BMessage error(IM_ERROR); + error.AddString("error", "Could not connect"); + error.AddString("details", irc_strerror(irc_errno(session))); + _SendMessage(&error); + _SendMessage(new BMessage(IM_PROTOCOL_DISABLE)); return B_ERROR; } // Start network loop if (irc_run(session)) { - printf("Could not connect or I/O error: %s\n", irc_strerror - (irc_errno(session))); + BMessage error(IM_ERROR); + error.AddString("error", "Connection or I/O error"); + error.AddString("details", irc_strerror(irc_errno(session))); + _SendMessage(&error); + _SendMessage(new BMessage(IM_PROTOCOL_DISABLE)); return B_ERROR; } return B_OK; diff --git a/protocols/purple/PurpleApp.cpp b/protocols/purple/PurpleApp.cpp index 396fb86..e7a8172 100644 --- a/protocols/purple/PurpleApp.cpp +++ b/protocols/purple/PurpleApp.cpp @@ -115,7 +115,7 @@ PurpleApp::MessageReceived(BMessage* msg) SendMessage(thread, _GetCommands(_AccountFromMessage(msg))); break; } - case PURPLE_REQUEST_DISCONNECT: + case PURPLE_DISCONNECT_ACCOUNT: { PurpleAccount* account = _AccountFromMessage(msg); if (account == NULL) @@ -1034,8 +1034,7 @@ signal_account_signed_off(PurpleAccount* account) static void signal_account_disabled(PurpleAccount* account) { - BMessage disabled(IM_MESSAGE); - disabled.AddInt32("im_what", IM_PROTOCOL_DISABLED); + BMessage disabled(PURPLE_SHUTDOWN_ADDON); ((PurpleApp*)be_app)->SendMessage(account, disabled); } diff --git a/protocols/purple/PurpleMessages.h b/protocols/purple/PurpleMessages.h index 0600e27..3810ebb 100644 --- a/protocols/purple/PurpleMessages.h +++ b/protocols/purple/PurpleMessages.h @@ -56,7 +56,13 @@ enum purple_message { /*! Disconnect add-on's account →Server Requires: String account_name */ - PURPLE_REQUEST_DISCONNECT = 'Axwx', + PURPLE_DISCONNECT_ACCOUNT = 'Axwx', + + /*! Shutdown an add-on instance →Protocol + Send from server after an account is disabled + or errors out; sent so an add-on instance + won't run for an unconnected account. */ + PURPLE_SHUTDOWN_ADDON = 'Pxwx', /*! Register chat commands with proto →Protocol */ PURPLE_REGISTER_COMMANDS = 'Scmd', diff --git a/protocols/purple/PurpleProtocol.cpp b/protocols/purple/PurpleProtocol.cpp index 6c56954..547bf97 100644 --- a/protocols/purple/PurpleProtocol.cpp +++ b/protocols/purple/PurpleProtocol.cpp @@ -19,8 +19,6 @@ #include "PurpleProtocol.h" -#include - #include #include #include @@ -124,6 +122,13 @@ connect_thread(void* data) while (true) { BMessage* msg = new BMessage(receive_message()); switch (msg->what) { + case PURPLE_SHUTDOWN_ADDON: { + protocol->Shutdown(); + BMessage* disabled = new BMessage(IM_MESSAGE); + disabled->AddInt32("im_what", IM_PROTOCOL_DISABLE); + protocol->SendMessage(disabled); + break; + } case PURPLE_REGISTER_COMMANDS: protocol->Process(msg); break; @@ -186,7 +191,7 @@ PurpleProtocol::Init(ChatProtocolMessengerInterface* interface) status_t PurpleProtocol::Shutdown() { - BMessage* disconnect = new BMessage(PURPLE_REQUEST_DISCONNECT); + BMessage* disconnect = new BMessage(PURPLE_DISCONNECT_ACCOUNT); _SendPrplMessage(disconnect); kill_thread(fBirdThread); diff --git a/protocols/xmpp/JabberHandler.cpp b/protocols/xmpp/JabberHandler.cpp index c73c5d3..dd081e5 100644 --- a/protocols/xmpp/JabberHandler.cpp +++ b/protocols/xmpp/JabberHandler.cpp @@ -693,9 +693,12 @@ JabberHandler::HandleConnectionError(gloox::ConnectionError& e) } break; } - case gloox::ConnNotConnected: - errMsg.AddString("error", "There is no active connection."); + case gloox::ConnNotConnected: { + BMessage disconnect(IM_MESSAGE); + disconnect.AddInt32("im_what", IM_PROTOCOL_DISABLE); + _SendMessage(&disconnect); break; + } default: break; }