Hide peer detail view if multiple are selected #13

pull promag wants to merge 1 commits into bitcoin-core:master from promag:2020-06-peer-detail-view changing 2 files +16 −29

promag commented at 10:51 AM on June 27, 2020: contributor

Currently if multiple peers are selected the peer detail view shows the first new selected peer.

With this PR the peer detail view is hidden when multiple peers are selected. It is also a slight refactor to simplify and remove duplicate code.
promag commented at 10:51 AM on June 27, 2020: contributor

This was done while reviewing #6, @hebasto maybe you could take a look too.
hebasto commented at 11:12 AM on June 27, 2020: member

Concept ACK.
MarcoFalke renamed this:
~~qt: Hide peer detail view if multiple are selected~~
Hide peer detail view if multiple are selected
on Jun 27, 2020

in src/qt/rpcconsole.cpp:1087 in cb262e7680 outdated

1084 | -void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats)
1085 | +void RPCConsole::updateDetailWidget()
1086 |  {
1087 | +    auto selected_rows = ui->peerWidget->selectionModel()->selectedRows();
1088 | +    if (!clientModel || !clientModel->getPeerTableModel() || selected_rows.size() != 1) {
1089 | +        ui->detailWidget->hide();

Saibato commented at 7:05 PM on June 27, 2020:

Nit check for access to clientModel ? Had some sporadic segfaults, with check patch so far none hmm.

if (!clientModel) return;
    auto selected_rows = ui->peerWidget->selectionModel()->selectedRows();

hebasto commented at 5:57 AM on June 29, 2020: member

Tested cb262e7680b274ff8f0c500965cfe4873108cbd1 on Linux Mint 20 (x86_64, Qt 5.12.8):

$ lldb ./src/qt/bitcoin-qt -- -testnet
(lldb) target create "./src/qt/bitcoin-qt"
Current executable set to '/home/hebasto/GitHub/gui/src/qt/bitcoin-qt' (x86_64).
(lldb) settings set -- target.run-args  "-testnet"
(lldb) run
Process 20230 launched: '/home/hebasto/GitHub/gui/src/qt/bitcoin-qt' (x86_64)
Process 20230 stopped
* thread [#1](/bitcoin-core-gui/1/), name = 'bitcoin-qt', stop reason = signal SIGSEGV: invalid address (fault address: 0x8)
    frame [#0](/bitcoin-core-gui/0/): 0x00007ffff6d50711 libQt5Core.so.5`QItemSelectionModel::selection() const + 17
libQt5Core.so.5`QItemSelectionModel::selection:
->  0x7ffff6d50711 <+17>: movq   0x8(%rsi), %rbx
    0x7ffff6d50715 <+21>: movq   %fs:0x28, %rax
    0x7ffff6d5071e <+30>: movq   %rax, 0x28(%rsp)
    0x7ffff6d50723 <+35>: xorl   %eax, %eax
(lldb) bt
* thread [#1](/bitcoin-core-gui/1/), name = 'bitcoin-qt', stop reason = signal SIGSEGV: invalid address (fault address: 0x8)
  * frame [#0](/bitcoin-core-gui/0/): 0x00007ffff6d50711 libQt5Core.so.5`QItemSelectionModel::selection() const + 17
    frame [#1](/bitcoin-core-gui/1/): 0x00007ffff6d5088e libQt5Core.so.5`QItemSelectionModel::selectedRows(int) const + 94
    frame [#2](/bitcoin-core-gui/2/): 0x000055555562468f bitcoin-qt`RPCConsole::updateDetailWidget(this=0x0000555556514ff0) at rpcconsole.cpp:1083:60
    frame [#3](/bitcoin-core-gui/3/): 0x0000555555622cc7 bitcoin-qt`RPCConsole::RPCConsole(this=0x0000555556514ff0, node=<unavailable>, _platformStyle=<unavailable>, parent=<unavailable>) at rpcconsole.cpp:495:5
    frame [#4](/bitcoin-core-gui/4/): 0x00005555555dc3d3 bitcoin-qt`BitcoinGUI::BitcoinGUI(this=0x0000555556598400, node=0x0000555556369b40, _platformStyle=0x00005555564fa900, networkStyle=<unavailable>, parent=<unavailable>) at bitcoingui.cpp:97:22
    frame [#5](/bitcoin-core-gui/5/): 0x00005555555d37b2 bitcoin-qt`BitcoinApplication::createWindow(this=0x00007fffffffdbf8, networkStyle=<unavailable>) at bitcoin.cpp:252:18
    frame [#6](/bitcoin-core-gui/6/): 0x00005555555d6247 bitcoin-qt`GuiMain(argc=<unavailable>, argv=<unavailable>) at bitcoin.cpp:583:13
    frame [#7](/bitcoin-core-gui/7/): 0x00005555555d22e3 bitcoin-qt`main(argc=<unavailable>, argv=<unavailable>) at main.cpp:21:43
    frame [#8](/bitcoin-core-gui/8/): 0x00007ffff63020b3 libc.so.6`__libc_start_main(main=(bitcoin-qt`main at main.cpp:21), argc=2, argv=0x00007fffffffdd98, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007fffffffdd88) at libc-start.c:308:16
    frame [#9](/bitcoin-core-gui/9/): 0x00005555555d220e bitcoin-qt`_start + 46

promag commented at 8:35 AM on June 29, 2020: contributor

Strange, in macos that didn't happen. Was that on startup? On opening peers tab?
hebasto commented at 8:36 AM on June 29, 2020: member

Strange, in macos that didn't happen. Was that on startup? On opening peers tab?

On startup.
promag force-pushed on Jun 29, 2020
promag commented at 3:23 PM on June 29, 2020: contributor

@hebasto should be fixed now, selectionModel() can return nullptr.
hebasto commented at 5:29 PM on June 29, 2020: member

@hebasto should be fixed now...

Testing right now.

selectionModel() can return nullptr.

The Qt docs keep silence about nullptr :)
promag commented at 5:33 PM on June 29, 2020: contributor

Right, I saw qabstractitemview.cpp where it also has the check.

hebasto commented at 5:35 PM on June 29, 2020: member

@promag How about the following patch which fixes the problem described in the OP:

--- a/src/qt/rpcconsole.cpp
+++ b/src/qt/rpcconsole.cpp
@@ -593,7 +593,7 @@ void RPCConsole::setClientModel(ClientModel *model)
         ui->peerWidget->verticalHeader()->hide();
         ui->peerWidget->setEditTriggers(QAbstractItemView::NoEditTriggers);
         ui->peerWidget->setSelectionBehavior(QAbstractItemView::SelectRows);
-        ui->peerWidget->setSelectionMode(QAbstractItemView::ExtendedSelection);
+        ui->peerWidget->setSelectionMode(QAbstractItemView::SingleSelection);
         ui->peerWidget->setContextMenuPolicy(Qt::CustomContextMenu);
         ui->peerWidget->setColumnWidth(PeerTableModel::Address, ADDRESS_COLUMN_WIDTH);
         ui->peerWidget->setColumnWidth(PeerTableModel::Subversion, SUBVERSION_COLUMN_WIDTH);

promag commented at 5:51 PM on June 29, 2020: contributor

Multiple selection is enabled and used when you use the context menu.
qt: Hide peer detail view if multiple are selected 76277cc77d

in src/qt/rpcconsole.cpp:1081 in df2987a0bd outdated

1062 | @@ -1078,7 +1063,6 @@ void RPCConsole::peerLayoutChanged()
1063 |          }
1064 |  
1065 |          // get fresh stats on the detail node.
1066 | -        stats = clientModel->getPeerTableModel()->getNodeStats(detailNodeRow);

hebasto commented at 6:13 PM on June 29, 2020:

nit: not sure about the two lines above?

promag commented at 7:32 PM on June 29, 2020:

Removed those.

promag force-pushed on Jun 29, 2020

in src/qt/rpcconsole.cpp:1091 in 76277cc77d

1089 | +    if (!clientModel || !clientModel->getPeerTableModel() || selected_rows.size() != 1) {
1090 | +        ui->detailWidget->hide();
1091 | +        ui->peerHeading->setText(tr("Select a peer to view detailed information."));
1092 | +        return;
1093 | +    }
1094 | +    const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(selected_rows.first().row());

hebasto commented at 6:48 AM on June 30, 2020:

nanonit:


    const CNodeCombinedStats* stats = clientModel->getPeerTableModel()->getNodeStats(selected_rows.first().row());

or even use const auto (that seems preferable to me).

in src/qt/rpcconsole.cpp:1084 in 76277cc77d

1082 |  
1083 | -void RPCConsole::updateNodeDetail(const CNodeCombinedStats *stats)
1084 | +void RPCConsole::updateDetailWidget()
1085 |  {
1086 | +    QModelIndexList selected_rows;
1087 | +    auto selection_model = ui->peerWidget->selectionModel();

hebasto commented at 6:53 AM on June 30, 2020:

nit:

    const auto selection_model = ui->peerWidget->selectionModel();

hebasto approved
hebasto commented at 6:56 AM on June 30, 2020: member

ACK 76277cc77dea39b53e09ee1c440cd37270826201, tested on Linux Mint 20 (Qt 5.12.8).
Saibato commented at 4:12 PM on July 1, 2020: contributor

tested https://github.com/bitcoin-core/gui/commit/76277cc77dea39b53e09ee1c440cd37270826201 on debian and lubuntu VM's on top of master and other changes so far LGTM
hebasto commented at 5:38 PM on July 1, 2020: member

@promag Due to this connection https://github.com/bitcoin-core/gui/blob/76277cc77dea39b53e09ee1c440cd37270826201/src/qt/rpcconsole.cpp#L626

not sure if added in the latest push updateDetailWidget(); is required at all.
promag commented at 12:21 AM on July 7, 2020: contributor

@hebasto right, I've added it just to be sure view is updated. I also thought of blocking signals to prevent unnecessary updates because of that signal. Another thing we could do is to add const CNodeCombinedStats* m_current_node_stats, this way we can skip unnecessary work in updateDetailWidget.
promag closed this on Jul 7, 2020
promag reopened this on Jul 7, 2020
luke-jr commented at 7:40 PM on July 31, 2020: member

Weak Concept NACK

My mail program also shows the most-recently-touched message when selecting multiple, so the current behaviour isn't too weird. I'm not sure there's a benefit from hiding it?
hebasto commented at 10:53 AM on August 1, 2020: member

@luke-jr

Weak Concept NACK

My mail program also shows the most-recently-touched message when selecting multiple, so the current behaviour isn't too weird. I'm not sure there's a benefit from hiding it?

But the GUI shows not the most-recently-touched peer details rather the first-touched one's. Therefore, this pull is an improvement, I think.
promag commented at 4:14 PM on August 1, 2020: contributor

Either hide or show last selected looks good to me.
jonasschnelli commented at 9:18 AM on October 23, 2020: contributor

Tested ACK 76277cc77dea39b53e09ee1c440cd37270826201. Should this be considered a bugfix?
laanwj referenced this in commit 924a4ff7eb on Oct 29, 2020
jonasschnelli merged this on Nov 20, 2020
jonasschnelli closed this on Nov 20, 2020
promag deleted the branch on Nov 20, 2020
promag commented at 9:35 AM on November 20, 2020: contributor

Thanks.
sidhujag referenced this in commit 45a8ec42fe on Nov 20, 2020
apoelstra referenced this in commit 94091e2872 on Dec 3, 2020
gwillen referenced this in commit fbac33bbcc on Mar 23, 2021
MarcoFalke referenced this in commit bce09da122 on Apr 28, 2021
fanquake referenced this in commit fa00bb2c5c on Apr 29, 2021
MarcoFalke referenced this in commit eb9a1fe037 on May 7, 2021
MarcoFalke referenced this in commit c857148636 on May 15, 2021
Fabcien referenced this in commit 45627eef65 on Jan 11, 2022
bitcoin-core locked this on Feb 15, 2022

Contributors

Linked (view graph)

#7 Crash on exit #247 bitcoin-qt stays at "Done loading" for a long time #545 SIGABRT on macOS after Dock menu --> Quit #668 Got stuck in "loading wallets", then segfault on closing #689 Segmentation fault when closing while loading wallets #785 New crash in v26.0 #796 ci_native_asan: UndefinedBehaviorSanitizer: null-pointer-use qt/test/wallettests.cpp:424:25 in #880 SegFault in QSortFilterProxyModelPrivate::build_source_to_proxy_mapping