From 601fd81d5cbb4608e21df843dbc3db10ae4866a0 Mon Sep 17 00:00:00 2001 From: James Rowe Date: Mon, 9 Apr 2018 11:00:56 -0600 Subject: [PATCH] Address review comments --- src/citra_qt/multiplayer/chat_room.cpp | 57 +++++++++++---------- src/citra_qt/multiplayer/chat_room.h | 2 +- src/citra_qt/multiplayer/direct_connect.cpp | 26 +++------- src/citra_qt/multiplayer/direct_connect.h | 1 - src/citra_qt/multiplayer/lobby_p.h | 22 ++++---- src/citra_qt/multiplayer/message.h | 4 +- src/citra_qt/multiplayer/state.cpp | 11 ++-- src/citra_qt/util/clickable_label.cpp | 2 - src/citra_qt/util/clickable_label.h | 5 +- 9 files changed, 58 insertions(+), 72 deletions(-) diff --git a/src/citra_qt/multiplayer/chat_room.cpp b/src/citra_qt/multiplayer/chat_room.cpp index 46c477eae..d55b39c22 100644 --- a/src/citra_qt/multiplayer/chat_room.cpp +++ b/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 #include #include #include @@ -10,7 +11,6 @@ #include #include #include - #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() {} - const QList player_color = { + static constexpr std::array 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) { - 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(); + if (room->GetState() != Network::RoomMember::State::Joined) { + return; } + 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 l; std::vector 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); diff --git a/src/citra_qt/multiplayer/chat_room.h b/src/citra_qt/multiplayer/chat_room.h index 8c07654cc..c837c24e0 100644 --- a/src/citra_qt/multiplayer/chat_room.h +++ b/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; diff --git a/src/citra_qt/multiplayer/direct_connect.cpp b/src/citra_qt/multiplayer/direct_connect.cpp index 785062d48..c21fe5d75 100644 --- a/src/citra_qt/multiplayer/direct_connect.cpp +++ b/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; - ShowError(NetworkMessage::USERNAME_NOT_VALID); + NetworkMessage::ShowError(NetworkMessage::USERNAME_NOT_VALID); + return; } if (const auto member = Network::GetRoomMember().lock()) { - if (member->IsConnected()) { - if (!NetworkMessage::WarnDisconnect()) { - return; - } + if (member->IsConnected() && !NetworkMessage::WarnDisconnect()) { + return; } } switch (static_cast(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")); diff --git a/src/citra_qt/multiplayer/direct_connect.h b/src/citra_qt/multiplayer/direct_connect.h index 6d9266601..026484394 100644 --- a/src/citra_qt/multiplayer/direct_connect.h +++ b/src/citra_qt/multiplayer/direct_connect.h @@ -30,7 +30,6 @@ private slots: private: void Connect(); - void ClearAllError(); void BeginConnecting(); void EndConnecting(); diff --git a/src/citra_qt/multiplayer/lobby_p.h b/src/citra_qt/multiplayer/lobby_p.h index fa8580349..3773f99de 100644 --- a/src/citra_qt/multiplayer/lobby_p.h +++ b/src/citra_qt/multiplayer/lobby_p.h @@ -4,10 +4,10 @@ #pragma once +#include #include #include #include - #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(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) { - username = other.username; - title_id = other.title_id; - 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(const LobbyMember& other) = default; + explicit LobbyMember(QString username, u64 title_id, QString game_name) + : username(std::move(username)), title_id(title_id), game_name(std::move(game_name)) {} ~LobbyMember() = default; QString GetUsername() const { @@ -153,7 +149,7 @@ public: static const int MaxPlayerRole = Qt::UserRole + 2; LobbyItemMemberList() = default; - explicit LobbyItemMemberList(QList members, u32 max_players) : LobbyItem() { + explicit LobbyItemMemberList(QList 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 members) : LobbyItem() { + explicit LobbyItemExpandedMemberList(QList members) { setData(members, MemberListRole); } diff --git a/src/citra_qt/multiplayer/message.h b/src/citra_qt/multiplayer/message.h index 3a95c1081..3b8613199 100644 --- a/src/citra_qt/multiplayer/message.h +++ b/src/citra_qt/multiplayer/message.h @@ -4,12 +4,14 @@ #pragma once +#include + 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; } diff --git a/src/citra_qt/multiplayer/state.cpp b/src/citra_qt/multiplayer/state.cpp index f0857af52..8882f1c53 100644 --- a/src/citra_qt/multiplayer/state.cpp +++ b/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 (NetworkMessage::WarnCloseRoom()) { - room->Destroy(); - announce_multiplayer_session->Stop(); - } + if (room->GetState() != Network::Room::State::Open) { + return; + } + if (NetworkMessage::WarnCloseRoom()) { + room->Destroy(); + announce_multiplayer_session->Stop(); } } } diff --git a/src/citra_qt/util/clickable_label.cpp b/src/citra_qt/util/clickable_label.cpp index 010413271..e990423a9 100644 --- a/src/citra_qt/util/clickable_label.cpp +++ b/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(); } diff --git a/src/citra_qt/util/clickable_label.h b/src/citra_qt/util/clickable_label.h index 780a01bf6..3c65a74be 100644 --- a/src/citra_qt/util/clickable_label.h +++ b/src/citra_qt/util/clickable_label.h @@ -6,14 +6,13 @@ #include #include -#include class ClickableLabel : public QLabel { Q_OBJECT public: - explicit ClickableLabel(QWidget* parent = Q_NULLPTR, Qt::WindowFlags f = Qt::WindowFlags()); - ~ClickableLabel(); + explicit ClickableLabel(QWidget* parent = nullptr, Qt::WindowFlags f = Qt::WindowFlags()); + ~ClickableLabel() = default; signals: void clicked();