Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "Direction" column to peers tab #289

Closed
wants to merge 4 commits into from
Closed

Add "Direction" column to peers tab #289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
678b572
gui: add Direction column to peers tab
jonatack Apr 23, 2021
842f4e8
gui: improve connection type translations and translator comments
jonatack Apr 25, 2021
ad92e04
gui: use SPACE constant, QStringBuilder concat in rpcconsole.cpp
jonatack Apr 25, 2021
2bf3f5a
gui: add missing QLatin1String, QStringBuilder headers
jonatack Apr 25, 2021
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
1 change: 1 addition & 0 deletions src/qt/addressbookpage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include <qt/platformstyle.h>

#include <QIcon>
#include <QLatin1String>
jonatack marked this conversation as resolved.
Show resolved Hide resolved
#include <QMenu>
#include <QMessageBox>
#include <QSortFilterProxyModel>
Expand Down
2 changes: 2 additions & 0 deletions src/qt/guiutil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,10 +666,12 @@ QString ConnectionTypeToQString(ConnectionType conn_type, bool prepend_direction
{
QString prefix;
if (prepend_direction) {
//: Connection direction. Also used in CONNECTION_TYPE_DOC and PeerTableModel::data().
prefix = (conn_type == ConnectionType::INBOUND) ? QObject::tr("Inbound") : QObject::tr("Outbound") + " ";
}
switch (conn_type) {
case ConnectionType::INBOUND: return prefix;
//: The connection types (Full Relay, Block Relay, Manual, Feeler, Address Fetch) are also used in CONNECTION_TYPE_DOC.
case ConnectionType::OUTBOUND_FULL_RELAY: return prefix + QObject::tr("Full Relay");
case ConnectionType::BLOCK_RELAY: return prefix + QObject::tr("Block Relay");
case ConnectionType::MANUAL: return prefix + QObject::tr("Manual");
Expand Down
7 changes: 5 additions & 2 deletions src/qt/peertablemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,10 @@ QVariant PeerTableModel::data(const QModelIndex &index, int role) const
case NetNodeId:
return (qint64)rec->nodeStats.nodeid;
case Address:
// prepend to peer address down-arrow symbol for inbound connection and up-arrow for outbound connection
return QString(rec->nodeStats.fInbound ? "↓ " : "↑ ") + QString::fromStdString(rec->nodeStats.addrName);
return QString::fromStdString(rec->nodeStats.addrName);
case Direction:
//: Connection direction. Also used in ConnectionTypeToQString() and CONNECTION_TYPE_DOC.
return QString(rec->nodeStats.fInbound ? tr("Inbound") : tr("Outbound"));
Copy link

Choose a reason for hiding this comment

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

Can we use IN and OUT in caps instead of Inbound and Outbound?

Copy link
Member

Choose a reason for hiding this comment

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

Here is how that would look, not a bad idea. But, a) I think this change is ok as is b) would like to hear other opinions.

Screen Shot 2021-04-30 at 3 09 34 AM

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal is to unify the peers tab Direction and Type columns with the Direction/Type row in the peer details sidebar, so if the side bar displays "Outbound Full Relay", the main table also displays "Outbound Full Relay" with the ability to sort by either Direction or Type.

Copy link
Member Author

Choose a reason for hiding this comment

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

(I also tried "In/Out" while working on this PR, and the horizontal space isn't reduced unless the column header is "Dir" instead of "Direction". But the other headers are full words.)

case ConnectionType:
return GUIUtil::ConnectionTypeToQString(rec->nodeStats.m_conn_type, /* prepend_direction */ false);
case Network:
Expand All @@ -134,6 +136,7 @@ QVariant PeerTableModel::data(const QModelIndex &index, int role) const
case NetNodeId:
case Address:
return {};
case Direction:
case ConnectionType:
case Network:
return QVariant(Qt::AlignCenter);
Expand Down
4 changes: 3 additions & 1 deletion src/qt/peertablemodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class PeerTableModel : public QAbstractTableModel
enum ColumnIndex {
NetNodeId = 0,
Address,
Direction,
ConnectionType,
Network,
Ping,
Expand Down Expand Up @@ -74,7 +75,8 @@ public Q_SLOTS:

private:
interfaces::Node& m_node;
const QStringList columns{tr("Peer Id"), tr("Address"), tr("Type"), tr("Network"), tr("Ping"), tr("Sent"), tr("Received"), tr("User Agent")};
/*: Column titles of the Peers tab window. */
const QStringList columns{tr("Peer Id"), tr("Address"), tr("Direction"), tr("Type"), tr("Network"), tr("Ping"), tr("Sent"), tr("Received"), tr("User Agent")};
std::unique_ptr<PeerTablePriv> priv;
QTimer *timer;
};
Expand Down
2 changes: 2 additions & 0 deletions src/qt/peertablesortproxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ bool PeerTableSortProxy::lessThan(const QModelIndex& left_index, const QModelInd
return left_stats.nodeid < right_stats.nodeid;
case PeerTableModel::Address:
return left_stats.addrName.compare(right_stats.addrName) < 0;
case PeerTableModel::Direction:
return left_stats.fInbound > right_stats.fInbound; // default sort Inbound, then Outbound
case PeerTableModel::ConnectionType:
return left_stats.m_conn_type < right_stats.m_conn_type;
case PeerTableModel::Network:
Expand Down
2 changes: 2 additions & 0 deletions src/qt/psbtoperationsdialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

#include <iostream>

#include <QLatin1String>


PSBTOperationsDialog::PSBTOperationsDialog(
QWidget* parent, WalletModel* wallet_model, ClientModel* client_model) : QDialog(parent, GUIUtil::dialog_flags),
Expand Down
1 change: 1 addition & 0 deletions src/qt/qrimagewidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <QClipboard>
#include <QDrag>
#include <QFontDatabase>
#include <QLatin1String>
#include <QMenu>
#include <QMimeData>
#include <QMouseEvent>
Expand Down
49 changes: 28 additions & 21 deletions src/qt/rpcconsole.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,14 @@
#include <QDateTime>
#include <QFont>
#include <QKeyEvent>
#include <QLatin1String>
#include <QMenu>
#include <QMessageBox>
#include <QScreen>
#include <QScrollBar>
#include <QSettings>
#include <QString>
#include <QStringBuilder>
Copy link

Choose a reason for hiding this comment

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

Not sure if it helps only by including (without using operator %), unless whole bitcoin-qt would be built with QT_USE_QSTRINGBUILDER.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added because this file already uses %.

Copy link

Choose a reason for hiding this comment

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

I see, QLatin1String and QStringBuilder included because where simply missing, not because of new usage. New usage would be nice though, but I don't want to be too-QString-nazi..

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the nudge, @Talkless. Looked up https://wiki.qt.io/Using_QString_Effectively and updated. Please let me know what you think.

#include <QStringList>
#include <QTime>
#include <QTimer>
Expand All @@ -53,6 +55,9 @@ const int INITIAL_TRAFFIC_GRAPH_MINS = 30;
const QSize FONT_RANGE(4, 40);
const char fontSizeSettingsKey[] = "consoleFontSize";

const QLatin1String COLON_SPACE{": "};
Copy link

Choose a reason for hiding this comment

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

Interesting idea. It actually could be constexpr.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was the first thing I tried but the compiler doesn't like it.

Copy link

Choose a reason for hiding this comment

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

That's strange, QLatin1String's constructors are constexpr-marked since at least 5.3: https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/tools/qstring.h?h=5.3#n89

Sure that wasn't type or something? What kind of message was that? And what compiler?

Copy link
Member Author

Choose a reason for hiding this comment

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

This seemed relevant: https://stackoverflow.com/questions/56201976/qt-vs-constexpr-string-literal

Clang 9 and GCC 10.2.1:

  CXX      qt/libbitcoinqt_a-rpcconsole.o
qt/rpcconsole.cpp:58:32: error: constexpr variable 'COLON_SPACE' must be initialized by a constant expression
static constexpr QLatin1String COLON_SPACE{": "};
                               ^~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:91:93: note: non-constexpr function 'strlen' cannot be used in a constant expression
    Q_DECL_CONSTEXPR inline explicit QLatin1String(const char *s) noexcept : m_size(s ? int(strlen(s)) : 0), m_data(s) {}
                                                                                            ^
qt/rpcconsole.cpp:58:32: note: in call to 'QLatin1String(&": "[0])'
static constexpr QLatin1String COLON_SPACE{": "};
                               ^
qt/rpcconsole.cpp:59:32: error: constexpr variable 'SPACE' must be initialized by a constant expression
static constexpr QLatin1String SPACE{" "};
                               ^~~~~~~~~~
/usr/include/x86_64-linux-gnu/qt5/QtCore/qstring.h:91:93: note: non-constexpr function 'strlen' cannot be used in a constant expression
    Q_DECL_CONSTEXPR inline explicit QLatin1String(const char *s) noexcept : m_size(s ? int(strlen(s)) : 0), m_data(s) {}
                                                                                            ^
qt/rpcconsole.cpp:59:32: note: in call to 'QLatin1String(&" "[0])'
static constexpr QLatin1String SPACE{" "};
                               ^
2 errors generated.

const QLatin1String SPACE{" "};

const struct {
const char *url;
const char *source;
Expand Down Expand Up @@ -463,21 +468,23 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty

constexpr QChar nonbreaking_hyphen(8209);
const std::vector<QString> CONNECTION_TYPE_DOC{
tr("Inbound: initiated by peer"),
tr("Outbound Full Relay: default"),
tr("Outbound Block Relay: does not relay transactions or addresses"),
tr("Outbound Manual: added using RPC %1 or %2/%3 configuration options")
//: The connection direction (Inbound/Outbound) is also used in ConnectionTypeToQString() and PeerTableModel::data().
//: The connection types (Full Relay, Block Relay, Manual, Feeler, Address Fetch) are also used in ConnectionTypeToQString().
tr("Inbound") % COLON_SPACE % tr("initiated by peer"),
tr("Outbound") % SPACE % tr("Full Relay") % COLON_SPACE % tr("default"),
tr("Outbound") % SPACE % tr("Block Relay") % COLON_SPACE % tr("does not relay transactions or addresses"),
tr("Outbound") % SPACE % tr("Manual") % COLON_SPACE % tr("added using RPC %1 or %2/%3 configuration options")
.arg("addnode")
.arg(QString(nonbreaking_hyphen) + "addnode")
.arg(QString(nonbreaking_hyphen) + "connect"),
tr("Outbound Feeler: short-lived, for testing addresses"),
tr("Outbound Address Fetch: short-lived, for soliciting addresses")};
const QString list{"<ul><li>" + Join(CONNECTION_TYPE_DOC, QString("</li><li>")) + "</li></ul>"};
.arg(QString(nonbreaking_hyphen) % "addnode")
.arg(QString(nonbreaking_hyphen) % "connect"),
tr("Outbound") % SPACE % tr("Feeler") % COLON_SPACE % tr("short-lived, for testing addresses"),
tr("Outbound") % SPACE % tr("Address Fetch") % COLON_SPACE % tr("short-lived, for soliciting addresses")};
const QString list{"<ul><li>" % Join(CONNECTION_TYPE_DOC, QString("</li><li>")) % "</li></ul>"};
ui->peerConnectionTypeLabel->setToolTip(ui->peerConnectionTypeLabel->toolTip().arg(list));
const QString hb_list{"<ul><li>\""
+ ts.to + "\" – " + tr("we selected the peer for high bandwidth relay") + "</li><li>\""
+ ts.from + "\" – " + tr("the peer selected us for high bandwidth relay") + "</li><li>\""
+ ts.no + "\" – " + tr("no high bandwidth relay selected") + "</li></ul>"};
% ts.to % "\" – " % tr("we selected the peer for high bandwidth relay") % "</li><li>\""
% ts.from % "\" – " % tr("the peer selected us for high bandwidth relay") % "</li><li>\""
% ts.no % "\" – " % tr("no high bandwidth relay selected") % "</li></ul>"};
ui->peerHighBandwidthLabel->setToolTip(ui->peerHighBandwidthLabel->toolTip().arg(hb_list));
ui->dataDir->setToolTip(ui->dataDir->toolTip().arg(QString(nonbreaking_hyphen) + "datadir"));
ui->blocksDir->setToolTip(ui->blocksDir->toolTip().arg(QString(nonbreaking_hyphen) + "blocksdir"));
Expand Down Expand Up @@ -620,10 +627,10 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
// create peer table context menu
peersTableContextMenu = new QMenu(this);
peersTableContextMenu->addAction(tr("Disconnect"), this, &RPCConsole::disconnectSelectedNode);
peersTableContextMenu->addAction(ts.ban_for + " " + tr("1 hour"), [this] { banSelectedNode(60 * 60); });
peersTableContextMenu->addAction(ts.ban_for + " " + tr("1 day"), [this] { banSelectedNode(60 * 60 * 24); });
peersTableContextMenu->addAction(ts.ban_for + " " + tr("1 week"), [this] { banSelectedNode(60 * 60 * 24 * 7); });
peersTableContextMenu->addAction(ts.ban_for + " " + tr("1 year"), [this] { banSelectedNode(60 * 60 * 24 * 365); });
peersTableContextMenu->addAction(ts.ban_for % SPACE % tr("1 hour"), [this] { banSelectedNode(60 * 60); });
peersTableContextMenu->addAction(ts.ban_for % SPACE % tr("1 day"), [this] { banSelectedNode(60 * 60 * 24); });
peersTableContextMenu->addAction(ts.ban_for % SPACE % tr("1 week"), [this] { banSelectedNode(60 * 60 * 24 * 7); });
peersTableContextMenu->addAction(ts.ban_for % SPACE % tr("1 year"), [this] { banSelectedNode(60 * 60 * 24 * 365); });
connect(ui->peerWidget, &QTableView::customContextMenuRequested, this, &RPCConsole::showPeersTableContextMenu);

// peer table signal handling - update peer details when selecting new node
Expand Down Expand Up @@ -835,12 +842,12 @@ void RPCConsole::message(int category, const QString &message, bool html)

void RPCConsole::updateNetworkState()
{
QString connections = QString::number(clientModel->getNumConnections()) + " (";
connections += tr("In:") + " " + QString::number(clientModel->getNumConnections(CONNECTIONS_IN)) + " / ";
connections += tr("Out:") + " " + QString::number(clientModel->getNumConnections(CONNECTIONS_OUT)) + ")";
QString connections = QString::number(clientModel->getNumConnections()) % " (";
connections += tr("In:") % SPACE % QString::number(clientModel->getNumConnections(CONNECTIONS_IN)) % " / ";
connections += tr("Out:") % SPACE % QString::number(clientModel->getNumConnections(CONNECTIONS_OUT)) % ")";

if(!clientModel->node().getNetworkActive()) {
connections += " (" + tr("Network activity disabled") + ")";
connections += " (" % tr("Network activity disabled") % ")";
}

ui->numberOfConnections->setText(connections);
Expand Down Expand Up @@ -1022,7 +1029,7 @@ void RPCConsole::updateDetailWidget()
}
const auto stats = selected_peers.first().data(PeerTableModel::StatsRole).value<CNodeCombinedStats*>();
// update the detail ui with latest node information
QString peerAddrDetails(QString::fromStdString(stats->nodeStats.addrName) + " ");
QString peerAddrDetails(QString::fromStdString(stats->nodeStats.addrName) % SPACE);
peerAddrDetails += tr("(peer id: %1)").arg(QString::number(stats->nodeStats.nodeid));
if (!stats->nodeStats.addrLocal.empty())
peerAddrDetails += "<br />" + tr("via %1").arg(QString::fromStdString(stats->nodeStats.addrLocal));
Expand Down
1 change: 1 addition & 0 deletions src/qt/sendcoinsdialog.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <validation.h>

#include <QFontMetrics>
#include <QLatin1String>
#include <QScrollBar>
#include <QSettings>
#include <QTextDocument>
Expand Down
1 change: 1 addition & 0 deletions src/qt/transactionview.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <QHBoxLayout>
#include <QHeaderView>
#include <QLabel>
#include <QLatin1String>
#include <QLineEdit>
#include <QMenu>
#include <QPoint>
Expand Down
1 change: 1 addition & 0 deletions src/qt/walletview.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <QClipboard>
#include <QFileDialog>
#include <QHBoxLayout>
#include <QLatin1String>
#include <QProgressDialog>
#include <QPushButton>
#include <QVBoxLayout>
Expand Down