Address review comments

src/citra_qt/multiplayer/chat_room.cpp

@@ -2,6 +2,7 @@
 // Licensed under GPLv2 or any later version
 // Refer to the license.txt file included.
 
+#include <array>
 #include <future>
 #include <QColor>
 #include <QImage>
@@ -10,7 +11,6 @@
 #include <QMetaType>
 #include <QTime>
 #include <QtConcurrent/QtConcurrentRun>
-
 #include "citra_qt/game_list_p.h"
 #include "citra_qt/multiplayer/chat_room.h"
 #include "citra_qt/multiplayer/message.h"
@@ -36,10 +36,10 @@ public:
     }
 
 private:
-    ChatMessage() {}
+    static constexpr std::array<const char*, 16> player_color = {
-    const QList<QString> player_color = {
         {"#0000FF", "#FF0000", "#8A2BE2", "#FF69B4", "#1E90FF", "#008000", "#00FF7F", "#B22222",
          "#DAA520", "#FF4500", "#2E8B57", "#5F9EA0", "#D2691E", "#9ACD32", "#FF7F50", "FFFF00"}};
+
     QString timestamp;
     QString nickname;
     QString message;
@@ -49,7 +49,7 @@ class StatusMessage {
 public:
     explicit StatusMessage(const QString& msg, QTime ts = {}) {
         /// Convert the time to their default locale defined format
-        static QLocale locale;
+        QLocale locale;
         timestamp = locale.toString(ts.isValid() ? ts : QTime::currentTime(), QLocale::ShortFormat);
         message = msg;
     }
@@ -65,7 +65,7 @@ private:
     QString message;
 };
 
-ChatRoom::ChatRoom(QWidget* parent) : ui(new Ui::ChatRoom) {
+ChatRoom::ChatRoom(QWidget* parent) : QWidget(parent), ui(new Ui::ChatRoom) {
     ui->setupUi(this);
 
     // set the item_model for player_view
@@ -159,28 +159,29 @@ void ChatRoom::OnChatReceive(const Network::ChatEntry& chat) {
 
 void ChatRoom::OnSendChat() {
     if (auto room = Network::GetRoomMember().lock()) {
-        if (room->GetState() == Network::RoomMember::State::Joined) {
+        if (room->GetState() != Network::RoomMember::State::Joined) {
-            auto message = ui->chat_message->text().toStdString();
+            return;
-            if (!ValidateMessage(message)) {
-                return;
-            }
-            auto nick = room->GetNickname();
-            Network::ChatEntry chat{nick, message};
-
-            auto members = room->GetMemberInformation();
-            auto it = std::find_if(members.begin(), members.end(),
-                                   [&chat](const Network::RoomMember::MemberInformation& member) {
-                                       return member.nickname == chat.nickname;
-                                   });
-            if (it == members.end()) {
-                LOG_INFO(Network, "Chat message received from unknown player");
-            }
-            auto player = std::distance(members.begin(), it);
-            ChatMessage m(chat);
-            room->SendChatMessage(message);
-            AppendChatMessage(m.GetPlayerChatMessage(player));
-            ui->chat_message->clear();
         }
+        auto message = ui->chat_message->text().toStdString();
+        if (!ValidateMessage(message)) {
+            return;
+        }
+        auto nick = room->GetNickname();
+        Network::ChatEntry chat{nick, message};
+
+        auto members = room->GetMemberInformation();
+        auto it = std::find_if(members.begin(), members.end(),
+                               [&chat](const Network::RoomMember::MemberInformation& member) {
+                                   return member.nickname == chat.nickname;
+                               });
+        if (it == members.end()) {
+            LOG_INFO(Network, "Chat message received from unknown player");
+        }
+        auto player = std::distance(members.begin(), it);
+        ChatMessage m(chat);
+        room->SendChatMessage(message);
+        AppendChatMessage(m.GetPlayerChatMessage(player));
+        ui->chat_message->clear();
     }
 }
 
@@ -188,11 +189,11 @@ void ChatRoom::SetPlayerList(const Network::RoomMember::MemberList& member_list)
     // TODO(B3N30): Remember which row is selected
     player_list->removeRows(0, player_list->rowCount());
     for (const auto& member : member_list) {
-        if (member.nickname == "")
+        if (member.nickname.empty())
             continue;
         QList<QStandardItem*> l;
         std::vector<std::string> elements = {member.nickname, member.game_info.name};
-        for (auto& item : elements) {
+        for (const auto& item : elements) {
             QStandardItem* child = new QStandardItem(QString::fromStdString(item));
             child->setEditable(false);
             l.append(child);

src/citra_qt/multiplayer/chat_room.h

@@ -45,7 +45,7 @@ signals:
     void ChatReceived(const Network::ChatEntry&);
 
 private:
-    const u32 max_chat_lines = 1000;
+    static constexpr u32 max_chat_lines = 1000;
     void AppendChatMessage(const QString&);
     bool ValidateMessage(const std::string&);
     QStandardItemModel* player_list;

src/citra_qt/multiplayer/direct_connect.cpp

@@ -19,7 +19,7 @@
 #include "network/network.h"
 #include "ui_direct_connect.h"
 
-enum class ConnectionType : u8 { TRAVERSAL_SERVER, IP };
+enum class ConnectionType : u8 { TraversalServer, IP };
 
 DirectConnectWindow::DirectConnectWindow(QWidget* parent)
     : QDialog(parent, Qt::WindowTitleHint | Qt::WindowCloseButtonHint | Qt::WindowSystemMenuHint),
@@ -44,38 +44,30 @@ DirectConnectWindow::DirectConnectWindow(QWidget* parent)
 }
 
 void DirectConnectWindow::Connect() {
-    ClearAllError();
-    bool isValid = true;
     if (!ui->nickname->hasAcceptableInput()) {
-        isValid = false;
+        NetworkMessage::ShowError(NetworkMessage::USERNAME_NOT_VALID);
-        ShowError(NetworkMessage::USERNAME_NOT_VALID);
+        return;
     }
     if (const auto member = Network::GetRoomMember().lock()) {
-        if (member->IsConnected()) {
+        if (member->IsConnected() && !NetworkMessage::WarnDisconnect()) {
-            if (!NetworkMessage::WarnDisconnect()) {
+            return;
-                return;
-            }
         }
     }
     switch (static_cast<ConnectionType>(ui->connection_type->currentIndex())) {
-    case ConnectionType::TRAVERSAL_SERVER:
+    case ConnectionType::TraversalServer:
         break;
     case ConnectionType::IP:
         if (!ui->ip->hasAcceptableInput()) {
-            isValid = false;
             NetworkMessage::ShowError(NetworkMessage::IP_ADDRESS_NOT_VALID);
+            return;
         }
         if (!ui->port->hasAcceptableInput()) {
-            isValid = false;
             NetworkMessage::ShowError(NetworkMessage::PORT_NOT_VALID);
+            return;
         }
         break;
     }
 
-    if (!isValid) {
-        return;
-    }
-
     // Store settings
     UISettings::values.nickname = ui->nickname->text();
     UISettings::values.ip = ui->ip->text();
@@ -98,8 +90,6 @@ void DirectConnectWindow::Connect() {
     BeginConnecting();
 }
 
-void DirectConnectWindow::ClearAllError() {}
-
 void DirectConnectWindow::BeginConnecting() {
     ui->connect->setEnabled(false);
     ui->connect->setText(tr("Connecting"));

src/citra_qt/multiplayer/direct_connect.h

@@ -30,7 +30,6 @@ private slots:
 
 private:
     void Connect();
-    void ClearAllError();
     void BeginConnecting();
     void EndConnecting();
 

src/citra_qt/multiplayer/lobby_p.h

@@ -4,10 +4,10 @@
 
 #pragma once
 
+#include <utility>
 #include <QPixmap>
 #include <QStandardItem>
 #include <QStandardItemModel>
-
 #include "common/common_types.h"
 
 namespace Column {
@@ -25,7 +25,7 @@ class LobbyItem : public QStandardItem {
 public:
     LobbyItem() = default;
     explicit LobbyItem(const QString& string) : QStandardItem(string) {}
-    virtual ~LobbyItem() override {}
+    virtual ~LobbyItem() override = default;
 };
 
 class LobbyItemName : public LobbyItem {
@@ -62,7 +62,7 @@ public:
     static const int GameIconRole = Qt::UserRole + 3;
 
     LobbyItemGame() = default;
-    explicit LobbyItemGame(u64 title_id, QString game_name, QPixmap smdh_icon) : LobbyItem() {
+    explicit LobbyItemGame(u64 title_id, QString game_name, QPixmap smdh_icon) {
         setData(static_cast<unsigned long long>(title_id), TitleIDRole);
         setData(game_name, GameNameRole);
         if (!smdh_icon.isNull()) {
@@ -97,7 +97,7 @@ public:
     static const int HostPortRole = Qt::UserRole + 3;
 
     LobbyItemHost() = default;
-    explicit LobbyItemHost(QString username, QString ip, u16 port) : LobbyItem() {
+    explicit LobbyItemHost(QString username, QString ip, u16 port) {
         setData(username, HostUsernameRole);
         setData(ip, HostIPRole);
         setData(port, HostPortRole);
@@ -120,13 +120,9 @@ public:
 class LobbyMember {
 public:
     LobbyMember() = default;
-    LobbyMember(const LobbyMember& other) {
+    LobbyMember(const LobbyMember& other) = default;
-        username = other.username;
+    explicit LobbyMember(QString username, u64 title_id, QString game_name)
-        title_id = other.title_id;
+        : username(std::move(username)), title_id(title_id), game_name(std::move(game_name)) {}
-        game_name = other.game_name;
-    }
-    explicit LobbyMember(const QString username, u64 title_id, const QString game_name)
-        : username(username), title_id(title_id), game_name(game_name) {}
     ~LobbyMember() = default;
 
     QString GetUsername() const {
@@ -153,7 +149,7 @@ public:
     static const int MaxPlayerRole = Qt::UserRole + 2;
 
     LobbyItemMemberList() = default;
-    explicit LobbyItemMemberList(QList<QVariant> members, u32 max_players) : LobbyItem() {
+    explicit LobbyItemMemberList(QList<QVariant> members, u32 max_players) {
         setData(members, MemberListRole);
         setData(max_players, MaxPlayerRole);
     }
@@ -183,7 +179,7 @@ public:
     static const int MemberListRole = Qt::UserRole + 1;
 
     LobbyItemExpandedMemberList() = default;
-    explicit LobbyItemExpandedMemberList(QList<QVariant> members) : LobbyItem() {
+    explicit LobbyItemExpandedMemberList(QList<QVariant> members) {
         setData(members, MemberListRole);
     }
 

src/citra_qt/multiplayer/message.h

@@ -4,12 +4,14 @@
 
 #pragma once
 
+#include <utility>
+
 namespace NetworkMessage {
 
 class ConnectionError {
 
 public:
-    explicit ConnectionError(const std::string& str) : err(str) {}
+    explicit ConnectionError(std::string str) : err(std::move(str)) {}
     const std::string& GetString() const {
         return err;
     }

src/citra_qt/multiplayer/state.cpp

@@ -104,11 +104,12 @@ void MultiplayerState::OnCreateRoom() {
 
 void MultiplayerState::OnCloseRoom() {
     if (auto room = Network::GetRoom().lock()) {
-        if (room->GetState() == Network::Room::State::Open) {
+        if (room->GetState() != Network::Room::State::Open) {
-            if (NetworkMessage::WarnCloseRoom()) {
+            return;
-                room->Destroy();
+        }
-                announce_multiplayer_session->Stop();
+        if (NetworkMessage::WarnCloseRoom()) {
-            }
+            room->Destroy();
+            announce_multiplayer_session->Stop();
         }
     }
 }

src/citra_qt/util/clickable_label.cpp

@@ -6,8 +6,6 @@
 
 ClickableLabel::ClickableLabel(QWidget* parent, Qt::WindowFlags f) : QLabel(parent) {}
 
-ClickableLabel::~ClickableLabel() {}
-
 void ClickableLabel::mouseReleaseEvent(QMouseEvent* event) {
     emit clicked();
 }

src/citra_qt/util/clickable_label.h

@@ -6,14 +6,13 @@
 
 #include <QLabel>
 #include <QWidget>
-#include <Qt>
 
 class ClickableLabel : public QLabel {
     Q_OBJECT
 
 public:
-    explicit ClickableLabel(QWidget* parent = Q_NULLPTR, Qt::WindowFlags f = Qt::WindowFlags());
+    explicit ClickableLabel(QWidget* parent = nullptr, Qt::WindowFlags f = Qt::WindowFlags());
-    ~ClickableLabel();
+    ~ClickableLabel() = default;
 
 signals:
     void clicked();