Handle peer addition/removal in a right way #164

hebasto · 2020-12-24T19:03:49Z

This PR makes PeerTableModel handle a peer addition/removal in a right way. See:

Fixes #160.

Fixes #191.

hebasto · 2020-12-30T19:54:51Z

Rebased 734dd5f -> d325799 (pr164.01 -> pr164.02) due to the conflict with #162.

hebasto · 2021-01-07T15:11:29Z

Will be rebased after any progress in #161 :)

…tion b3e9bca qt, refactor: Drop no longer used PeerTableModel::getNodeStats function (Hennadii Stepanov) 49c6040 qt: Use PeerTableModel::StatsRole (Hennadii Stepanov) 35007ed qt: Add PeerTableModel::StatsRole (Hennadii Stepanov) Pull request description: This PR allows to access to the `CNodeCombinedStats` instance directly from any view object. The `PeerTableModel::getNodeStats` member function removed as a kind of layer violation. No behavior changes. Also other pulls (bugfixes) are based on this one: #18 and #164. ACKs for top commit: jonatack: Tested re-ACK b3e9bca per `git range-diff ae8f797 4c05fe0 b3e9bca` promag: Code review ACK b3e9bca. jonasschnelli: utACK b3e9bca Tree-SHA512: 6ba50d5dd2c0373655d491ce8b130c47d598da2db5ff4b00633f447404c7e70f8562ead53ddf166e851384d9632ff9146a770c99845c2cdd3ff7250677e4c130

hebasto · 2021-01-11T09:55:40Z

Rebased d325799 -> f73d340 (pr164.02 -> pr164.03) due to the conflict with #161.

promag · 2021-01-11T10:00:57Z

Why is this based on #18?

hebasto · 2021-01-11T10:06:19Z

Why is this based on #18?

It fixes selection behavior and handling in general.

jarolrod

tACK f73d340

Confirming that the PR fixes the issue laid out in #160 on macOS 11.1 with Qt 5.15.2.

On master: If I select a peer, and a peer above it is disconnected, then the peer I had selected becomes unselected. Below are screenshots exhibiting this behavior.

Master: I have selected Peer ID 32

Master: Peer ID 32 is no longer selected because Peer ID 29 is above it and disconnected

on this PR: This behavior no longer occurs. If I select a peer, it remains selected even if a peer above it is disconnected.

PR: Peer ID 14 is selected

PR: Peer ID 0 has disconnected. Peer ID 14 remains selected.

hebasto · 2021-01-27T21:05:24Z

Rebased f73d340 -> 32ce58c (pr164.03 -> pr164.04) due to the conflict with #180.

hebasto · 2021-02-22T08:03:44Z

Rebased 32ce58c -> a3d21d3 (pr164.04 -> pr164.05) due to the conflict with #179.

jarolrod

re-ACK a3d21d3, tested on macOS 11.1 Qt 5.15.2 after rebase

hebasto · 2021-03-06T18:33:04Z

Rebased a3d21d3 -> fa95a22 (pr164.05 -> pr164.06) due to the conflict with bitcoin/bitcoin#21015.

hebasto · 2021-03-12T12:36:06Z

Updated b7fe2b1 -> 7bb0dc9 (pr164.07 -> pr164.08, diff):

addressed @Talkless' comment

Talkless · 2021-03-12T15:34:56Z

src/qt/peertablemodel.cpp

+ m_node.getNodesStats(nodes_stats);
+ decltype(m_peers_data) new_peers_data;
+ new_peers_data.reserve(nodes_stats.size());
+ for (const auto& node_stats : nodes_stats) {


Ugh, my suggestion was not good enought, you can actually move values out of that local nodes_stats:

for (auto&& node_stats : nodes_stats) { const CNodeCombinedStats stats{std::get<0>(std::move(node_stats)), std::get<2>(std::move(node_stats)), std::get<1>(std::move(node_stats))};

(note loop variable is no longer cost reference!)

Or even with an algorithm (also moves, at least it does on local test), if one would care about "No raw loops" mantra :)

std::transform(nodes_stats.begin(), nodes_stats.end(), std::back_inserter(new_peers_data), [](auto&& i) { return CNodeCombinedStats{std::get<0>(std::move(i)), std::get<2>(std::move(i)), std::get<1>(std::move(i))}; });

Though moving std::transform seems fishy, not sure if it's "legal" to move source... But gcc 10.2.1 does that.

Ugh, my suggestion was not good enought...

It seems like a kind of premature optimization, no?

Maybe, probably. Since there will be mostly on up to ~128 nodes by default, "nobody" would care if there are some extra copies I guess, and std::transform variant do look particularly "noisy" (i.e. "optimization" not worth the effort/readability cost).

hebasto · 2021-04-28T18:06:40Z

Rebased 7bb0dc9 -> ec3075d (pr164.08 -> pr164.09) due to the merging of #18.

Talkless

tACK ec3075d, tested on Debian Sid with Qt 5.15.2.

Tested using two bitcoind's connected to PR's bitcoin-qt in regtest mode. Did some reconnects, and so far table seems to work fine.

See Qt docs for QAbstractTableModel and QAbstractItemModel classes.

This commit does not change behavior.

hebasto · 2021-05-27T19:48:44Z

Rebased ec3075d -> e102c90 (pr164.09 -> pr164.10) due to the conflict with #311.

promag

Tested ACK e102c90 on macos 11.2.3 with depends build.

Also ran with -server and while some peer was selected removing/adding others from -cli didn't mess with the selection. Also didn't observe weird behaviors while resizing columns.

jonatack · 2021-06-05T14:42:18Z

Concept ACK. If this fixes what I think it does, it would be good to see it in v22 as a bug fix.

jarolrod

ACK e102c90

Tested functionality on Arch Linux Qt 5.15.2, macOS 11.3 Qt 5.15.2, and cross-compile from Linux to Windows 10.

Notes on Commits:

efb7e5a
- code review ACK cross-checked with Qt docs for QAbstractTableModel and QAbstractItemModel
1b66f6e
- code review ACK, the PeerTablePriv class would no longer be needed.
e102c90
- code review ACK, patch looks correct in its implementation in following recommendations from Qt docs
- some nits (1, 2, 3)

Screenshots:

The below screenshots show that this PR will close the mutated version of #160 (see, #160 (comment)). While I am not including videos showing that this PR also fixes #191, I can confirm that it does on all three tested platforms.

Arch Linux

master

Select Peer 6	Disconnect Peer 0, Peer 7 is now selected ❌

PR

Select Peer 9	Disconnect Peer 1, Peer 9 remains selected ✅

macOS 11.3

master

Select Peer 6	Disconnect Peer 0, Peer 7 is now selected ❌

PR

Select Peer 8	Disconnect Peer 0, Peer 8 remains selected ✅

Windows 10

Note: Difference between UI on master and PR is because master includes ab86ac7

master

Select Peer 4	Disconnect Peer 0, Peer 6 is now selected ❌

PR

Select Peer 5	Disconnect Peer 1, Peer 5 remains selected ✅

src/qt/peertablemodel.cpp

This change fixes a bug when a multiple rows selection gets inconsistent after a peer addition/removal.

hebasto · 2021-06-07T14:41:48Z

Updated e102c90 -> ecbd911 (pr164.10 -> pr164.11, diff):

addressed recent @jarolrod's comments

promag

reACK ecbd911 just improvements to the comment since last review.

jarolrod · 2021-06-07T17:34:22Z

re-ACK ecbd911

Only changes since last review are implementation of comment nits

ecbd911 qt: Handle peer addition/removal in a right way (Hennadii Stepanov) 1b66f6e qt: Drop PeerTablePriv class (Hennadii Stepanov) efb7e5a qt, refactor: Use default arguments for overridden functions (Hennadii Stepanov) Pull request description: This PR makes `PeerTableModel` handle a peer addition/removal in a right way. See: - https://doc.qt.io/qt-5/model-view-programming.html#inserting-and-removing-rows - https://doc.qt.io/qt-5/model-view-programming.html#resizable-models Fixes #160. Fixes #191. ACKs for top commit: jarolrod: re-ACK ecbd911 promag: reACK ecbd911 just improvements to the comment since last review. Tree-SHA512: 074935d67f78561724218e8b33822e2de16749f873c29054926b720ffcd642f08249a222b563983cf65a9b716290aa14e2372c47fc04e5f401f759db25ca710f

986bf78 qt: Emit dataChanged signal to dynamically re-sort Peers table (Hennadii Stepanov) Pull request description: [By default](https://doc.qt.io/qt-5/qsortfilterproxymodel.html#details), the `PeerTableSortProxy` > dynamically re-sorts ... data whenever the original model changes. That is not the case on master (8cdf917) as in ecbd911 (#164) no signals are emitted to notify about model changes. This PR uses a dedicated [`dataChanged`](https://doc.qt.io/qt-5/qabstractitemmodel.html#dataChanged) signal. Fixes #367. An alternative to #374. ACKs for top commit: jarolrod: ACK 986bf78 Tree-SHA512: dcb92c2f9a2c632880429e9528007db426d2ad938c64dfa1f1538c03e4b62620df52ad7daf33b582976c67b472ff76bc0dae707049f4bbbd4941232cee9ce3d4

hebasto mentioned this pull request Dec 24, 2020

Add peertablesortproxy module #18

Merged

DrahtBot added the Needs rebase label Dec 28, 2020

hebasto force-pushed the 201224-signal branch from 734dd5f to d325799 Compare December 30, 2020 19:35

hebasto mentioned this pull request Dec 30, 2020

Add PeerTableModel::StatsRole to prevent data layer violation #161

Merged

DrahtBot removed the Needs rebase label Dec 30, 2020

DrahtBot added the Needs rebase label Jan 7, 2021

hebasto force-pushed the 201224-signal branch from d325799 to f73d340 Compare January 11, 2021 09:54

DrahtBot removed the Needs rebase label Jan 11, 2021

jarolrod reviewed Jan 14, 2021

View reviewed changes

hebasto mentioned this pull request Jan 21, 2021

User Agent header flickers when trying to resize any column #191

Closed

DrahtBot added the Needs rebase label Jan 27, 2021

hebasto force-pushed the 201224-signal branch from f73d340 to 32ce58c Compare January 27, 2021 21:03

DrahtBot removed the Needs rebase label Jan 27, 2021

DrahtBot added the Needs rebase label Feb 22, 2021

hebasto force-pushed the 201224-signal branch from 32ce58c to a3d21d3 Compare February 22, 2021 07:59

DrahtBot removed the Needs rebase label Feb 22, 2021

jarolrod reviewed Feb 22, 2021

View reviewed changes

DrahtBot added the Needs rebase label Mar 4, 2021

hebasto force-pushed the 201224-signal branch from a3d21d3 to fa95a22 Compare March 6, 2021 18:29

hebasto added the Bug Something isn't working label Mar 6, 2021

DrahtBot removed the Needs rebase label Mar 6, 2021

Talkless reviewed Mar 12, 2021

View reviewed changes

hebasto force-pushed the 201224-signal branch from 7bb0dc9 to ec3075d Compare April 28, 2021 18:04

Talkless approved these changes May 3, 2021

View reviewed changes

DrahtBot added the Needs rebase label May 26, 2021

hebasto added 2 commits May 27, 2021 22:33

qt, refactor: Use default arguments for overridden functions

efb7e5a

See Qt docs for QAbstractTableModel and QAbstractItemModel classes.

qt: Drop PeerTablePriv class

1b66f6e

This commit does not change behavior.

hebasto force-pushed the 201224-signal branch from ec3075d to e102c90 Compare May 27, 2021 19:47

DrahtBot removed the Needs rebase label May 27, 2021

promag reviewed Jun 3, 2021

View reviewed changes

hebasto mentioned this pull request Jun 5, 2021

Save/restore column sizes of the tables in the Peers tab #256

Merged

jarolrod mentioned this pull request Jun 7, 2021

Selected peer in the "Peers" tab gets unselected if any previous peer disconnects #160

Closed

jarolrod reviewed Jun 7, 2021

View reviewed changes

src/qt/peertablemodel.cpp Outdated Show resolved Hide resolved

src/qt/peertablemodel.cpp Outdated Show resolved Hide resolved

src/qt/peertablemodel.cpp Outdated Show resolved Hide resolved

qt: Handle peer addition/removal in a right way

ecbd911

This change fixes a bug when a multiple rows selection gets inconsistent after a peer addition/removal.

hebasto force-pushed the 201224-signal branch from e102c90 to ecbd911 Compare June 7, 2021 14:38

promag reviewed Jun 7, 2021

View reviewed changes

hebasto merged commit e638acf into bitcoin-core:master Jun 7, 2021

hebasto deleted the 201224-signal branch June 7, 2021 19:15

This was referenced Jun 25, 2021

Peer table (node window) does not remain sorted on update #367

Closed

Fix #367 #374

Closed

hebasto mentioned this pull request Jun 28, 2021

Emit dataChanged signal to dynamically re-sort Peers table #375

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge e638acf into merged_master (Bitcoin PR bitcoin-core/gui#164)

b6205a6

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle peer addition/removal in a right way #164

Handle peer addition/removal in a right way #164

hebasto commented Dec 24, 2020 •

edited

Loading

hebasto commented Dec 30, 2020

hebasto commented Jan 7, 2021

hebasto commented Jan 11, 2021

promag commented Jan 11, 2021

hebasto commented Jan 11, 2021

jarolrod left a comment

hebasto commented Jan 27, 2021

hebasto commented Feb 22, 2021

jarolrod left a comment

hebasto commented Mar 6, 2021

hebasto commented Mar 12, 2021

Talkless Mar 12, 2021

hebasto Mar 26, 2021

Talkless Mar 27, 2021

hebasto commented Apr 28, 2021

Talkless left a comment

hebasto commented May 27, 2021

promag left a comment •

edited

Loading

jonatack commented Jun 5, 2021

jarolrod left a comment •

edited

Loading

hebasto commented Jun 7, 2021

promag left a comment •

edited

Loading

jarolrod commented Jun 7, 2021

Handle peer addition/removal in a right way #164

Handle peer addition/removal in a right way #164

Conversation

hebasto commented Dec 24, 2020 • edited Loading

hebasto commented Dec 30, 2020

hebasto commented Jan 7, 2021

hebasto commented Jan 11, 2021

promag commented Jan 11, 2021

hebasto commented Jan 11, 2021

jarolrod left a comment

Choose a reason for hiding this comment

hebasto commented Jan 27, 2021

hebasto commented Feb 22, 2021

jarolrod left a comment

Choose a reason for hiding this comment

hebasto commented Mar 6, 2021

hebasto commented Mar 12, 2021

Talkless Mar 12, 2021

Choose a reason for hiding this comment

hebasto Mar 26, 2021

Choose a reason for hiding this comment

Talkless Mar 27, 2021

Choose a reason for hiding this comment

hebasto commented Apr 28, 2021

Talkless left a comment

Choose a reason for hiding this comment

hebasto commented May 27, 2021

promag left a comment • edited Loading

Choose a reason for hiding this comment

jonatack commented Jun 5, 2021

jarolrod left a comment • edited Loading

Choose a reason for hiding this comment

Notes on Commits:

Screenshots:

Arch Linux

master

PR

macOS 11.3

master

PR

Windows 10

master

PR

hebasto commented Jun 7, 2021

promag left a comment • edited Loading

Choose a reason for hiding this comment

jarolrod commented Jun 7, 2021

hebasto commented Dec 24, 2020 •

edited

Loading

promag left a comment •

edited

Loading

jarolrod left a comment •

edited

Loading

promag left a comment •

edited

Loading