1022 cachedNodeids.clear();
1023- for(int i = 0; i < selected.size(); i++)
1024- {
1025- const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(selected.at(i).row());
1026+ for (const auto& peer : GUIUtil::getEntryData(ui->peerWidget, PeerTableModel::NetNodeId)) {
1027+ const auto stats = peer.data(PeerTableModel::StatsRole).value<CNodeCombinedStats*>();
6e27ee8b6 To me at least, this would be easier to understand what is going on (and not using auto allows uniform initialization). The same motivation for the other suggestions below.
0- for (const auto& peer : GUIUtil::getEntryData(ui->peerWidget, PeerTableModel::NetNodeId)) {
1- const auto stats = peer.data(PeerTableModel::StatsRole).value<CNodeCombinedStats*>();
2+ for (const QModelIndex& peer : GUIUtil::getEntryData(ui->peerWidget, PeerTableModel::NetNodeId)) {
3+ const CNodeCombinedStats* stats{peer.data(PeerTableModel::StatsRole).value<CNodeCombinedStats*>()};
AAA Style (Almost Always Auto)
… easier to understand what is going on
Maybe GUIUtil::getEntryData requires renaming? GetModelIndexesByColumn?
@promag What do you think?
Side node, I’d just ditch getEntryData and hasEntryData, doesn’t improve that much over Qt API. If we keep them then maybe reword to GetSelectedModelIndex or something like that with “selection”.
In this particular case I think peer should have the type, and leave auto for stats.
auto only when the type is obvious or when it can avoid a complicated, verbose type. Anyway, just a minor suggestion, feel free to ignore.
1078@@ -1081,15 +1079,13 @@ void RPCConsole::peerLayoutChanged()
1079
1080 void RPCConsole::updateDetailWidget()
1081 {
1082- QModelIndexList selected_rows;
1083- auto selection_model = ui->peerWidget->selectionModel();
1084- if (selection_model) selected_rows = selection_model->selectedRows();
1085- if (!clientModel || !clientModel->getPeerTableModel() || selected_rows.size() != 1) {
1086+ const auto selected_peers = GUIUtil::getEntryData(ui->peerWidget, PeerTableModel::NetNodeId);
6e27ee8b6
0 const QList<QModelIndex> selected_peers{GUIUtil::getEntryData(ui->peerWidget, PeerTableModel::NetNodeId)};
1088 ui->detailWidget->hide();
1089 ui->peerHeading->setText(tr("Select a peer to view detailed information."));
1090 return;
1091 }
1092- const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(selected_rows.first().row());
1093+ const auto stats = selected_peers.first().data(PeerTableModel::StatsRole).value<CNodeCombinedStats*>();
6e27ee8
0 const CNodeCombinedStats* stats{selected_peers.first().data(PeerTableModel::StatsRole).value<CNodeCombinedStats*>()};
.value<>(), IIRC auto can be used when the type is clear from the statement.
1209- if (detailNodeRow < 0) return;
1210-
1211+ for (const auto& peer : GUIUtil::getEntryData(ui->peerWidget, PeerTableModel::NetNodeId)) {
1212 // Find possible nodes, ban it and clear the selected node
1213- const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(detailNodeRow);
1214+ const auto stats = peer.data(PeerTableModel::StatsRole).value<CNodeCombinedStats*>();
6e27ee8b67a8d8a81038b58e8503a3ab0d22f881
0- for (const auto& peer : GUIUtil::getEntryData(ui->peerWidget, PeerTableModel::NetNodeId)) {
1+ for (const QModelIndex& peer : GUIUtil::getEntryData(ui->peerWidget, PeerTableModel::NetNodeId)) {
2 // Find possible nodes, ban it and clear the selected node
3- const auto stats = peer.data(PeerTableModel::StatsRole).value<CNodeCombinedStats*>();
4+ const CNodeCombinedStats* stats{peer.data(PeerTableModel::StatsRole).value<CNodeCombinedStats*>()};
65@@ -66,6 +66,10 @@ class PeerTableModel : public QAbstractTableModel
66 Subversion = 5
67 };
68
69+ enum {
70+ StatsRole = Qt::UserRole
71+ };
As an alternative, a simple constexpr could be used (it seems better for me), but here I use the way how we already did such things:
https://github.com/bitcoin-core/gui/blob/f1f26b8d5baec4a45a3a9ba0440cd4eff7af8407/src/qt/addresstablemodel.h#L36-L38
https://github.com/bitcoin-core/gui/blob/f1f26b8d5baec4a45a3a9ba0440cd4eff7af8407/src/qt/transactiontablemodel.h#L46 @promag What do you think?
Right, this follow Qt approach, I guess they didn’t want to manually assign a value to each role.
With multiple roles constexpr looks weird because you’d have to manually increment for each custom role.
… this follow Qt approach…
In the recent example they use
0const int IdRole = Qt::UserRole;
With multiple roles
constexprlooks weird because you’d have to manually increment for each custom role.
Agree.
This change allows access to CNodeCombinedStats instance directly from
any view object.
This change prevents direct calls to the PeerTableModel object that is a
layer violation.
Tested re-ACK b3e9bcaac85a64a1b41d534b28c8cfb1f08e14e5 per git range-diff ae8f797 4c05fe0 b3e9bca
Thanks for updating.
Code review ACK b3e9bcaac85a64a1b41d534b28c8cfb1f08e14e5.
As follow ups, maybe it could make sense to split each member of
0struct CNodeCombinedStats {
1 CNodeStats nodeStats;
2 CNodeStateStats nodeStateStats;
3 bool fNodeStateStatsAvailable;
4};
on a dedicated role.
Or just add some other roles like:
0- const auto stats = peer.data(PeerTableModel::StatsRole).value<CNodeCombinedStats*>();
1- cachedNodeids.append(stats->nodeStats.nodeid);
2+ cachedNodeids.append(peer.data(PeerTableModel::NodeIdRole).toInt());
