Dual-Stack AO2 Client to handle both TCP and Websocket connections seemlessly by Salanto · Pull Request #696 · AttorneyOnline/AO2-Client

Salanto · 2022-03-23T13:46:11Z

Is this a breaking change? Depends on how you view it.

If you intentionally sabotaged WebAO usage on your server by either limiting what Clients can join trough the websocket or just disabled them, this PR would break any connectivity we would have to that server till they either update or reenable websockets.

The replacement is a nice-to-have, as it finally removes the need for two ports and different connection medias between the regular AO2-Client and WebAO, giving new servers less of a headache implementing both and allowing already existing servers to streamline their connection code.

Closes #739

stonedDiscord · 2022-03-23T13:49:10Z

you can still sabotage webAO by using the version check packet

github-actions

clang-tidy made some suggestions

github-actions · 2022-03-23T13:51:27Z

include/networkmanager.h

  AOApplication *ao_app;
  QNetworkAccessManager *http;
-  QTcpSocket *server_socket;
+  QWebSocket *server_socket;


warning: member variable 'server_socket' has public visibility [misc-non-private-member-variables-in-classes]

QWebSocket *server_socket; ^

github-actions · 2022-03-23T13:51:27Z

include/networkmanager.h

  QTimer *heartbeat_timer;

-  const QString DEFAULT_MS_BASEURL = "https://servers.aceattorneyonline.com";
+  const QString DEFAULT_MS_BASEURL = "http://servers.aceattorneyonline.com";


warning: member variable 'DEFAULT_MS_BASEURL' has public visibility [misc-non-private-member-variables-in-classes]

const QString DEFAULT_MS_BASEURL = "http://servers.aceattorneyonline.com"; ^

github-actions · 2022-03-23T13:51:27Z

src/networkmanager.cpp

 }

-void NetworkManager::handle_server_packet()
+void NetworkManager::handle_server_packet(QString p_data)


warning: the parameter 'p_data' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

Suggested change

void NetworkManager::handle_server_packet(QString p_data)

void NetworkManager::handle_server_packet(const QString& p_data)

include/networkmanager.h:47:

- void handle_server_packet(QString p_data); + void handle_server_packet(const QString& p_data);

Salanto · 2022-03-23T14:04:02Z

Yeah, but at that point is it even worth it anymore? Just allow WebAO, its not harmful.

Crystalwarrior · 2022-03-23T14:36:59Z

Do not remove tcp support or no one will update because their favorite server breaks or they can't play with friends who are not technically inclined.

Instead make it connect by websockets first, and if it fails fall back to tcp. Add settings that allow you to disable tcp entirely, and make it off by default.

Salanto · 2022-03-23T14:59:18Z

Do not remove TCP support or no one will update because their favourite server breaks or they can't play with friends who are not technically inclined.

Instead make it connect by WebSockets first, and if it fails fall back to TCP. Add settings that allow you to disable TCP entirely, and make it off by default.

Literally what? Name me one of the popular servers that is not CaseCafe or DRO, which has no Websocket support in general and their mostly unique client, that don't have websocket support. This is no harder to set up on the server owner as all major derivatives and servers have WebSocket support which is turned on with a single config change.

Adding Websockets as an addition to TCP only adds more clutter to the AOClient with no tangible benefit besides "we backwards compatible because reasons."

It already filters for websocket enabled servers so there is no "oh no, can't connect to this server on master" moment.

oldmud0

staying compatible with legacy tcp is like 5 lines. i will prove it to you

there are a number of servers that don't have ws forwarded properly or don't want to allow webao for whatever reason. They would not be prepared for this otherwise major change

Salanto · 2022-03-23T17:41:30Z

In the end I can't stop you from doing so, but the endless nemesis of legacy support will eventually make this project less maintainable than it already is, but a long term goal should be to prefer Websocket over TCP and phase the later out.

oldmud0 · 2022-03-24T01:17:44Z

yeah i know, having legacy things around is the rather unfortunate consequence of a game with such a long history

"like 5 lines" yeah probably lost a bet.

github-actions

clang-tidy made some suggestions

github-actions · 2022-03-24T03:20:11Z

include/networkmanager.h

  void connect_to_server(server_type p_server);
+  void disconnect_from_server();

 public slots:


warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]

Suggested change

public slots:

include/networkmanager.h:46: previously declared here

public: ^

github-actions · 2022-03-24T03:20:11Z

src/networkmanager.cpp

+  switch (p_server.socket_type) {
+  case TCP:
+    qInfo() << "using TCP backend";
+    server_socket.tcp = new QTcpSocket(this);


warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

server_socket.tcp = new QTcpSocket(this); ^

github-actions · 2022-03-24T03:20:11Z

src/networkmanager.cpp

+    qInfo() << "using TCP backend";
+    server_socket.tcp = new QTcpSocket(this);
+
+    connect(server_socket.tcp, &QAbstractSocket::connected, this, [] {


warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

connect(server_socket.tcp, &QAbstractSocket::connected, this, [] { ^

github-actions · 2022-03-24T03:20:11Z

src/networkmanager.cpp

+    connect(server_socket.tcp, &QAbstractSocket::connected, this, [] {
+      qDebug() << "established connection to server";
+    });
+    connect(server_socket.tcp, &QIODevice::readyRead, this, [this] {


warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

connect(server_socket.tcp, &QIODevice::readyRead, this, [this] { ^

github-actions · 2022-03-24T03:20:11Z

src/networkmanager.cpp

+      qDebug() << "established connection to server";
+    });
+    connect(server_socket.tcp, &QIODevice::readyRead, this, [this] {
+      handle_server_packet(QString::fromUtf8(server_socket.tcp->readAll()));


warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

handle_server_packet(QString::fromUtf8(server_socket.tcp->readAll())); ^

github-actions · 2022-03-24T03:20:14Z

src/networkmanager.cpp

+    server_socket.tcp->deleteLater();
+    break;
+  case WEBSOCKETS:
+    server_socket.ws->close(QWebSocketProtocol::CloseCodeGoingAway);


warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

server_socket.ws->close(QWebSocketProtocol::CloseCodeGoingAway); ^

github-actions · 2022-03-24T03:20:14Z

src/networkmanager.cpp

+    break;
+  case WEBSOCKETS:
+    server_socket.ws->close(QWebSocketProtocol::CloseCodeGoingAway);
+    server_socket.ws->deleteLater();


warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

server_socket.ws->deleteLater(); ^

github-actions · 2022-03-24T03:20:14Z

src/networkmanager.cpp

-  server_socket->write(p_packet.toUtf8());
+  switch (active_connection_type) {
+  case TCP:
+    server_socket.tcp->write(p_packet.toUtf8());


warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

server_socket.tcp->write(p_packet.toUtf8()); ^

github-actions · 2022-03-24T03:20:15Z

src/networkmanager.cpp

+    server_socket.tcp->write(p_packet.toUtf8());
+    break;
+  case WEBSOCKETS:
+    server_socket.ws->sendTextMessage(p_packet);


warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

server_socket.ws->sendTextMessage(p_packet); ^

src/networkmanager.cpp

oldmud0 · 2022-03-24T03:59:30Z

oh crap, houston we got a big big problem. need to rebuild the windows qt5 builder so that it includes the websockets module. alternatively remove the gitlab CI for windows altogether

Salanto · 2022-03-24T19:53:58Z

Given that Github Actions produce usable clients, we don't need to run the same CI twice, do we? Though I am sure to be corrected because some people still use 32-bit operating systems in 2022, though.

AwesomeAim · 2022-03-25T02:02:14Z

This completely breaks favourites. Also, I can apparently double click on a favourite entry and it stores my double click and then the next time I click on a server that replies it immediately joins it.

include/networkmanager.h

Crystalwarrior · 2022-04-04T11:26:07Z

This completely breaks favourites. Also, I can apparently double click on a favourite entry and it stores my double click and then the next time I click on a server that replies it immediately joins it.

~~Just launched it, favorites seem to work?~~ Wait the favorites tab is not functional on Github Actions but it is functional from qt creator, what the hell?

EDIT: Turns out I've been compiling with minGW while github actions uses VS. stonedDiscord is gonna debug it

Crystalwarrior · 2022-04-04T12:12:03Z

It only uses TCP backend when connecting to Favorites servers when built via minGW

src/networkmanager.cpp

Make "add_favorite_server" remember the socket type

This will keep new entries compatible with 2.9 and prior clients. Makes parsing the list easier too.

src/text_file_functions.cpp

src/aoapplication.cpp

* I have no idea what a lookup table is, but this looks close enough

* Otherwise backend selection behaviour is inverted

* Yet it did not do it. Co-authored-by: oldmud0 <oldmud0@users.noreply.github.com>

Salanto · 2022-05-23T20:22:34Z

Should be good to go now. Tested on MingW 32-bit and the CI action.

oldmud0

this has waited far too long

github-actions

clang-tidy made some suggestions

include/datatypes.h

oldmud0

this is a major feature for 2.10, so let's get this merged

your review sucks jk

src/text_file_functions.cpp

* Fixes an Omni bug where : would split the servername * Utilises internally QSettings properly for low parsing effort and clear structure * Automatically migrates the legacy serverlist.txt to favorite_servers.ini * Pleases my OCD

github-actions

clang-tidy made some suggestions

github-actions · 2022-06-04T20:50:16Z

src/text_file_functions.cpp

+      server_type f_server;
+      l_favorite_ini.beginGroup(fav_index);
+      f_server.ip = l_favorite_ini.value("address", "127.0.0.1").toString();
+      f_server.port = l_favorite_ini.value("port", 27016).toInt();


warning: 27016 is a magic number; consider replacing it with a named constant [readability-magic-numbers]

f_server.port = l_favorite_ini.value("port", 27016).toInt(); ^

was dealt with

oldmud0 · 2022-06-06T18:31:40Z

Cool. Very cool

Replace TCP Serversocket with Websocket

f4aee75

github-actions bot reviewed Mar 23, 2022

View reviewed changes

oldmud0 requested changes Mar 23, 2022

View reviewed changes

Crystalwarrior added this to the 2.10 milestone Mar 24, 2022

Have TCP sockets live harmoniously with WS

90533e9

"like 5 lines" yeah probably lost a bet.

github-actions bot reviewed Mar 24, 2022

View reviewed changes

Update .gitlab-ci.yml

95d29c1

stonedDiscord previously requested changes Mar 25, 2022

View reviewed changes

include/networkmanager.h Show resolved Hide resolved

Crystalwarrior closed this Apr 4, 2022

Crystalwarrior reopened this Apr 4, 2022

hack to fix favorites

3feba26

oldmud0 reviewed Apr 4, 2022

View reviewed changes

src/networkmanager.cpp Outdated Show resolved Hide resolved

Crystalwarrior and others added 2 commits April 6, 2022 14:40

Add support for websockets in the favorites list (serverlist.txt)

1bb37ce

Make "add_favorite_server" remember the socket type

Preserve old serverlist style

866c43e

This will keep new entries compatible with 2.9 and prior clients. Makes parsing the list easier too.

oldmud0 reviewed Apr 8, 2022

View reviewed changes

src/text_file_functions.cpp Outdated Show resolved Hide resolved

src/aoapplication.cpp Outdated Show resolved Hide resolved

Salanto and others added 4 commits May 23, 2022 21:30

Add lookup table and correct write code to use lowercase

17a77ef

* I have no idea what a lookup table is, but this looks close enough

Merge branch 'master' into remove-tcp-add-websocket

60e3aee

Fix lookup table

04e9e15

* Otherwise backend selection behaviour is inverted

clang-tidy had one job

7e8701b

* Yet it did not do it. Co-authored-by: oldmud0 <oldmud0@users.noreply.github.com>

Salanto requested review from oldmud0 and stonedDiscord May 24, 2022 06:59

oldmud0 previously approved these changes May 27, 2022

View reviewed changes

Salanto changed the title ~~Replace TCP Serversocket with Websocket~~ Dual-Stack AO2 Client to handle both TCP and Websocket connections seemlessly May 27, 2022

Merge branch 'master' into remove-tcp-add-websocket

06c32fd

Salanto dismissed oldmud0’s stale review via 06c32fd June 2, 2022 18:35

github-actions bot reviewed Jun 2, 2022

View reviewed changes

include/datatypes.h Show resolved Hide resolved

stonedDiscord added 2 commits June 3, 2022 12:45

const p_data

ef1acd2

Merge branch 'master' into remove-tcp-add-websocket

f87b187

oldmud0 approved these changes Jun 4, 2022

View reviewed changes

oldmud0 previously requested changes Jun 4, 2022

View reviewed changes

src/text_file_functions.cpp Outdated Show resolved Hide resolved

Switch serverlist.txt to an ini format

520a284

* Fixes an Omni bug where : would split the servername * Utilises internally QSettings properly for low parsing effort and clear structure * Automatically migrates the legacy serverlist.txt to favorite_servers.ini * Pleases my OCD

github-actions bot reviewed Jun 4, 2022

View reviewed changes

Salanto requested review from Crystalwarrior and oldmud0 June 4, 2022 20:54

Replace sample serverlist.

8fb120b

stonedDiscord approved these changes Jun 6, 2022

View reviewed changes

stonedDiscord merged commit f0a5e48 into master Jun 6, 2022

stonedDiscord deleted the remove-tcp-add-websocket branch June 7, 2022 11:00

	void NetworkManager::handle_server_packet(QString p_data)
	void NetworkManager::handle_server_packet(const QString& p_data)

Conversation

Salanto commented Mar 23, 2022 • edited by Crystalwarrior Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stonedDiscord commented Mar 23, 2022

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

Salanto commented Mar 23, 2022

Uh oh!

Crystalwarrior commented Mar 23, 2022

Uh oh!

Salanto commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oldmud0 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Salanto commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oldmud0 commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oldmud0 commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Salanto commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AwesomeAim commented Mar 25, 2022

Uh oh!

Uh oh!

Crystalwarrior commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Crystalwarrior commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Salanto commented Mar 23, 2022 •

edited by Crystalwarrior

Loading

Salanto commented Mar 23, 2022 •

edited

Loading

oldmud0 left a comment •

edited

Loading

Salanto commented Mar 23, 2022 •

edited

Loading

oldmud0 commented Mar 24, 2022 •

edited

Loading

oldmud0 commented Mar 24, 2022 •

edited

Loading

Salanto commented Mar 24, 2022 •

edited

Loading

Crystalwarrior commented Apr 4, 2022 •

edited

Loading

Crystalwarrior commented Apr 4, 2022 •

edited

Loading