Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
209 changes: 193 additions & 16 deletions src/platform/qt/BattleChipView.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include "GBAApp.h"
#include "ShortcutController.h"
#include "Window.h"
#include <mgba/internal/gba/gba.h>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be a blank line between the C++ header and the C header include blocks

#include <mgba/internal/gba/memory.h>

#include <QtAlgorithms>
#include <QFileInfo>
Expand All @@ -20,6 +22,10 @@
#include <QSettings>
#include <QStringList>

#include <QTcpServer>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be merged and sorted into the include block above.

#include <QTcpSocket>
#include <QHostAddress>

using namespace QGBA;

BattleChipView::BattleChipView(std::shared_ptr<CoreController> controller, Window* window, QWidget* parent)
Expand Down Expand Up @@ -48,22 +54,23 @@ BattleChipView::BattleChipView(std::shared_ptr<CoreController> controller, Windo
m_ui.chipList->setGridSize(m_ui.chipList->gridSize() * size);
m_model.setScale(size);

connect(m_ui.chipId, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), m_ui.inserted, [this]() {
m_ui.inserted->setChecked(false);
});
connect(m_ui.chipName, static_cast<void (QComboBox::*)(int)>(&QComboBox::currentIndexChanged), m_ui.chipId, [this](int id) {
if (id < 0) {
return;
connect(m_ui.chipName, static_cast<void (QComboBox::*)(int)>(&QComboBox::currentIndexChanged), this, [this](int idx) {
if (idx < 0) return;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the existing style guide--no single line blocks.

const auto keys = m_model.chipNames().keys();
if (idx >= 0 && idx < keys.size()) {
bool blocked = m_ui.chipId->blockSignals(true);
m_ui.chipId->setValue(keys[idx]);
m_ui.chipId->blockSignals(blocked);
Comment on lines +58 to +63
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified:

Suggested change
if (idx < 0) return;
const auto keys = m_model.chipNames().keys();
if (idx >= 0 && idx < keys.size()) {
bool blocked = m_ui.chipId->blockSignals(true);
m_ui.chipId->setValue(keys[idx]);
m_ui.chipId->blockSignals(blocked);
auto keys = m_model.chipNames().keys();
if (idx < 0 || idx >= keys.size()) {
return;
}
QSignalBlocker blocker(m_ui.chipId);
m_ui.chipId->setValue(keys[idx]);

Though if I may ask, why are we blocking signals here now?

}
m_ui.chipId->setValue(m_model.chipNames().keys()[id]);
});

connect(m_ui.inserted, &QAbstractButton::toggled, this, &BattleChipView::insertChip);
connect(m_ui.insert, &QAbstractButton::clicked, this, &BattleChipView::reinsert);
connect(m_ui.add, &QAbstractButton::clicked, this, &BattleChipView::addChip);
connect(m_ui.remove, &QAbstractButton::clicked, this, &BattleChipView::removeChip);
connect(m_ui.save, &QAbstractButton::clicked, this, &BattleChipView::saveDeck);
connect(m_ui.load, &QAbstractButton::clicked, this, &BattleChipView::loadDeck);
connect(m_ui.chipId, static_cast<void (QSpinBox::*)(int)>(&QSpinBox::valueChanged), this, &BattleChipView::onChipIdChanged);
connect(m_ui.insert, &QAbstractButton::clicked, this, &BattleChipView::reinsert);
connect(m_ui.add, &QAbstractButton::clicked, this, &BattleChipView::addChip);
connect(m_ui.remove, &QAbstractButton::clicked, this, &BattleChipView::removeChip);
connect(m_ui.save, &QAbstractButton::clicked, this, &BattleChipView::saveDeck);
connect(m_ui.load, &QAbstractButton::clicked, this, &BattleChipView::loadDeck);
Comment on lines +68 to +73
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow existing convention. Do not do inline alignment, and especially don't change existing code just to match a convention you added.

connect(m_ui.updateData, &QAbstractButton::clicked, this, &BattleChipView::updateData);
connect(m_ui.buttonBox->button(QDialogButtonBox::Reset), &QAbstractButton::clicked, &m_model, &BattleChipModel::clear);

Expand Down Expand Up @@ -119,9 +126,23 @@ BattleChipView::BattleChipView(std::shared_ptr<CoreController> controller, Windo
connect(download, &QDialog::accepted, this, &BattleChipView::updateData);
download->show();
}

if (m_ui.netGateEnable && m_ui.netGatePort && m_ui.netGateBind && m_ui.netGateStatus) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will never be false.

connect(m_ui.netGateEnable, &QCheckBox::toggled, this, &BattleChipView::netGateToggled);
connect(m_ui.netGatePort, &QLineEdit::textChanged, this, &BattleChipView::netGatePortChanged);
connect(m_ui.netGateBind, &QLineEdit::textChanged, this, &BattleChipView::netGateBindChanged);
Comment on lines +131 to +133
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above about style.


bool ok = false;
quint32 p = m_ui.netGatePort->text().toUInt(&ok);
if (ok && p > 0 && p <= 65535) {
m_netPort = static_cast<quint16>(p);
}
m_netBindStr = m_ui.netGateBind->text().trimmed();
Comment on lines +135 to +140
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the constructor. You need to set default values.

}
}

BattleChipView::~BattleChipView() {
netGateStop();
m_controller->detachBattleChipGate();
}

Expand All @@ -133,9 +154,6 @@ void BattleChipView::setFlavor(int flavor) {
}

void BattleChipView::insertChip(bool inserted) {
bool blocked = m_ui.inserted->blockSignals(true);
m_ui.inserted->setChecked(inserted);
m_ui.inserted->blockSignals(blocked);
Comment on lines -136 to -138
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this removed?

if (inserted) {
m_controller->setBattleChipId(m_ui.chipId->value());
} else {
Expand All @@ -144,12 +162,15 @@ void BattleChipView::insertChip(bool inserted) {
}

void BattleChipView::reinsert() {
if (m_ui.inserted->isChecked()) {
const bool keepInserted = m_ui.inserted->isChecked();
if (keepInserted) {
Comment on lines -147 to +166
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has no functional change. Please revert this.

insertChip(false);
m_next = true;
m_frameCounter = UNINSERTED_TIME;
} else {
insertChip(true);
m_next = false;
m_frameCounter = UNINSERTED_TIME;
Comment on lines +172 to +173
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this added? If it's needed then the if statement can be removed entirely as both branches are the same, just with the parameters truth values swapped based on condition.

}
m_window->setWindowState(m_window->windowState() & ~Qt::WindowActive);
m_window->setWindowState(m_window->windowState() | Qt::WindowActive);
Expand Down Expand Up @@ -261,3 +282,159 @@ void BattleChipView::updateData() {
});
m_updater->downloadUpdate();
}

void BattleChipView::netGateToggled(bool on) {
if (on) {
netGateStart();
} else {
netGateStop();
}
}

void BattleChipView::netGatePortChanged(const QString& s) {
bool ok = false;
quint32 p = s.toUInt(&ok);
if (ok && p > 0 && p <= 65535) {
m_netPort = static_cast<quint16>(p);
if (m_netGateServer && m_netGateServer->isListening()) {
netGateStop();
netGateStart();
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified. An early return reduces the indent level and the explicit cast is unnecessary.

Suggested change
}
if (!ok || p == 0 || p > 65535) {
return;
}
m_netPort = p;
if (m_netGateServer && m_netGateServer->isListening()) {
netGateStop();
netGateStart();
}

}

void BattleChipView::netGateBindChanged(const QString& s) {
m_netBindStr = s.trimmed();
if (m_netGateServer && m_netGateServer->isListening()) {
netGateStop();
netGateStart();
}
}

void BattleChipView::netGateStart() {
if (!m_ui.netGateEnable || !m_ui.netGatePort || !m_ui.netGateBind || !m_ui.netGateStatus) {
return;
}
Comment on lines +315 to +317
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is always false.


if (!m_netPort) {
netGateSetStatus(tr("Failed: invalid port"));
m_ui.netGateEnable->setChecked(false);
return;
}
if (!m_netGateServer) {
m_netGateServer = new QTcpServer(this);
connect(m_netGateServer, &QTcpServer::newConnection, this, &BattleChipView::netGateNewConnection);
}
QHostAddress bindAddr;
if (!bindAddr.setAddress(m_netBindStr)) {
netGateSetStatus(tr("Failed: invalid bind address"));
m_ui.netGateEnable->setChecked(false);
return;
}
if (!m_netGateServer->listen(bindAddr, m_netPort)) {
netGateSetStatus(tr("Failed: %1").arg(m_netGateServer->errorString()));
m_ui.netGateEnable->setChecked(false);
return;
}
netGateDisableFields(true);
netGateSetStatus(tr("Listening on %1:%2").arg(bindAddr.toString()).arg(m_netPort));
}

void BattleChipView::netGateStop() {
for (QTcpSocket* c : m_netClients) {
if (!c) continue;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No single line blocks.

c->disconnect(this);
c->close();
c->deleteLater();
}
m_netClients.clear();
m_netBufs.clear();

if (m_netGateServer) {
m_netGateServer->close();
}

netGateDisableFields(false);
netGateSetStatus(tr("Stopped"));
}

void BattleChipView::netGateNewConnection() {
if (!m_netGateServer) return;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No single line blocks.

while (m_netGateServer->hasPendingConnections()) {
QTcpSocket* c = m_netGateServer->nextPendingConnection();
m_netClients.append(c);
m_netBufs.insert(c, QByteArray());
connect(c, &QTcpSocket::readyRead, this, [this, c]() { netGateReadyRead(c); });
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No single line blocks.

connect(c, &QTcpSocket::disconnected, this, [this, c]() {
m_netBufs.remove(c);
m_netClients.removeAll(c);
c->deleteLater();
});
}
}

void BattleChipView::netGateReadyRead(QTcpSocket* sock) {
if (!sock) return;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No single line blocks.

QByteArray& buf = m_netBufs[sock];
buf += sock->readAll();

for (;;) {
int hdr = buf.indexOf(char(0x80));
if (hdr < 0) { buf.clear(); return; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No single line blocks.

if (buf.size() - hdr < 3) {
if (hdr > 0) buf.remove(0, hdr);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No single line blocks.

return;
}
const unsigned char b0 = (unsigned char)buf.at(hdr + 0);
const unsigned char b1 = (unsigned char)buf.at(hdr + 1);
const unsigned char b2 = (unsigned char)buf.at(hdr + 2);
Comment on lines +388 to +390
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const unsigned char b0 = (unsigned char)buf.at(hdr + 0);
const unsigned char b1 = (unsigned char)buf.at(hdr + 1);
const unsigned char b2 = (unsigned char)buf.at(hdr + 2);
uint8_t b0 = buf.at(hdr + 0);
uint8_t b1 = buf.at(hdr + 1);
uint8_t b2 = buf.at(hdr + 2);

This is theoretically undefined behavior but mGBA is compiled with -fwrapv, which defines it. It's required for the core.

buf.remove(0, hdr + 3);
if (b0 != 0x80) continue;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No single line blocks.

const quint16 chipId = (quint16(b1) << 8) | quint16(b2);

netGateApplyChip(chipId);
}
}

void BattleChipView::netGateApplyChip(quint16 chipId) {
bool blocked = m_ui.chipId->blockSignals(true);
m_ui.chipId->setValue(chipId);
m_ui.chipId->blockSignals(blocked);
onChipIdChanged(chipId);

const bool keepInserted = m_ui.inserted->isChecked();
if (keepInserted) {
insertChip(false);
m_next = true;
m_frameCounter = UNINSERTED_TIME;
} else {
insertChip(true);
m_next = false;
m_frameCounter = UNINSERTED_TIME;
}
Comment on lines +405 to +414
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is reused code. Please factor it out into a method that's called in both places--after you explain why you changed it above to be the same.

}

void BattleChipView::netGateSetStatus(const QString& text) {
if (m_ui.netGateStatus) {
m_ui.netGateStatus->setText(text);
}
}
Comment on lines +417 to +421
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a single line inside of a block that's always true. Please just inline the callsites.


void BattleChipView::netGateDisableFields(bool disable) {
if (!m_ui.netGatePort || !m_ui.netGateBind) return;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No single line blocks.

m_ui.netGatePort->setEnabled(!disable);
m_ui.netGateBind->setEnabled(!disable);
}

void BattleChipView::onChipIdChanged(int id) {
const auto names = m_model.chipNames();
const QString name = names.value(id);
if (!name.isEmpty()) {
int idx = m_ui.chipName->findText(name);
if (idx >= 0) {
bool blocked = m_ui.chipName->blockSignals(true);
m_ui.chipName->setCurrentIndex(idx);
m_ui.chipName->blockSignals(blocked);
}
}
Comment on lines +430 to +439
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be slightly simplified:

Suggested change
const auto names = m_model.chipNames();
const QString name = names.value(id);
if (!name.isEmpty()) {
int idx = m_ui.chipName->findText(name);
if (idx >= 0) {
bool blocked = m_ui.chipName->blockSignals(true);
m_ui.chipName->setCurrentIndex(idx);
m_ui.chipName->blockSignals(blocked);
}
}
auto names = m_model.chipNames();
QString name = names.value(id);
if (name.isEmpty()) {
return;
}
int idx = m_ui.chipName->findText(name);
if (idx < 0) {
return;
}
QSignalBlocker blocker(m_ui.chipName);
m_ui.chipName->setCurrentIndex(idx);

}
27 changes: 27 additions & 0 deletions src/platform/qt/BattleChipView.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,20 @@
#include "BattleChipModel.h"

#include <QDialog>
#include <QByteArray>
#include <QString>
#include <QList>
#include <QHash>
Comment on lines 10 to +14
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep this sorted


#include <memory>

#include <mgba/core/interface.h>

#include "ui_BattleChipView.h"

class QTcpServer;
class QTcpSocket;

namespace QGBA {

class CoreController;
Expand Down Expand Up @@ -43,6 +50,14 @@ private slots:

void updateData();

void onChipIdChanged(int id);

void netGateToggled(bool on);
void netGatePortChanged(const QString& s);
void netGateBindChanged(const QString& s);
void netGateNewConnection();
void netGateReadyRead(QTcpSocket* sock);

private:
static const int UNINSERTED_TIME = 10;

Expand All @@ -59,6 +74,18 @@ private slots:
Window* m_window;

BattleChipUpdater* m_updater = nullptr;

QTcpServer* m_netGateServer = nullptr;
QList<QTcpSocket*> m_netClients;
QHash<QTcpSocket*, QByteArray> m_netBufs;
quint16 m_netPort;
QString m_netBindStr;

void netGateStart();
void netGateStop();
void netGateSetStatus(const QString& text);
void netGateDisableFields(bool disable);
void netGateApplyChip(quint16 chipId);
};

}
Loading