[Qt] extend rpc console peers tab #6209

Diapolo commented at 7:20 AM on June 1, 2015: none

add node id, ping wait, whitelisted and common height
rephrase some labels to make them easier to understand for users

peers

laanwj added the label GUI on Jun 1, 2015

laanwj commented at 8:08 AM on June 1, 2015: member

Post-0.11 utACK

jonasschnelli commented at 8:14 AM on June 1, 2015: contributor

utACK Will test soon.

in src/qt/forms/rpcconsole.ui:None in 2b2dcc52a6 outdated

 750 | +             <string>Whitelisted</string>
 751 | +            </property>
 752 | +           </widget>
 753 | +          </item>
 754 | +          <item row="0" column="2">
 755 | +           <widget class="QCheckBox" name="peerWhitelisted">

laanwj commented at 11:16 AM on June 1, 2015:

Let's use yes/no here instead of a checkbox (more consistent with rest of the table)

laanwj commented at 11:18 AM on June 1, 2015: member

I don't think it was introduced in this pull, but the Synced Headers, Synched Blocks and Ban Score rows blink furiously between actual values and Fetching.. all the time. I think we should display the last known value if the fetching fails.

jonasschnelli commented at 11:28 AM on June 1, 2015: contributor

I would prefer the checkbox – but only – if you could also set the state. Together with banning options (https://github.com/bitcoin/bitcoin/pull/6158) it would be a adequate look and feel. On the other hand, checkboxes sometimes don't really imply a change before the users presses "save" or something similar.

Diapolo commented at 11:32 AM on June 1, 2015: none

@laanwj I changed to Yes/No and also we don't display Fetching... anymore. I also thought about making whitelisting an option but would prefer doing that via a context menu :).

Diapolo commented at 11:41 AM on June 1, 2015: none

@jonasschnelli If I find the time I'm going to further test your ban/unban pull and would like to add a context menu for it :).

jonasschnelli commented at 11:46 AM on June 1, 2015: contributor

Style wise, the peers view should get an update:

The table doesn't auto-expand its cols/width.
Because now we have more lines in the text key/value table, the chance is very high that you run into the situations as the screen below shows (truncated text because of line height). @Diapolo: i can try to create a commit on top of your PR to improve the general look and feel of the peers view if you don't care about this.

bildschirmfoto-2015-06-01-um-13 37 11

bildschirmfoto 2015-06-01 um 13 39 47

bildschirmfoto-2015-06-01-um-13 28 58

jonasschnelli commented at 11:48 AM on June 1, 2015: contributor

@Diapolo: for banning/unbanning i think it would be more visible and more clear if the user could ban/unban over checkboxes or buttons at the peer details section (right part of the view) instead of a context menu. Context menus are not always comprehensible at first sight.

Diapolo commented at 12:05 PM on June 1, 2015: none

@jonasschnelli If my pull gets ACKs feel free to base a further UI polish on top, that's fine with me :). As for banning/unbanning, I think we should allow both options (click and via context menu).

Diapolo commented at 12:36 PM on June 1, 2015: none

Why is this failing the No Wallet build with: "No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

The build has been terminated"

Diapolo commented at 1:35 PM on June 1, 2015: none

@jonasschnelli Mind taking a look at commit 3, could be interresting as this just "kicks" a node via context menu :).

luke-jr commented at 4:38 AM on June 2, 2015: member

Concept ACK. I had to read the code to figure out what "Ping Wait" was, so maybe a tooltip explaining that it is the duration of a currently outstanding ping?

Diapolo commented at 5:43 AM on June 2, 2015: none

@luke-jr I'm fine with adding a tooltip for Ping Wait and yeah I also had to read the code to understand what it does ^^.

laanwj commented at 6:54 AM on June 2, 2015: member

I agree that a checkbox makes sense if the user would be able to change the value. However I don't see the point to be able to change whitelisting status on a connected node. I wouldn't want to encourage people to manually whitelist peers until they disconnect/reconnect. It's micro-management.

(I'm not against whitelist editing in the UI in itself, but not in this way, it should be more similar to the command line options)

in src/qt/rpcconsole.cpp:None in 4723d6e38b outdated

 688 | +}
 689 | +
 690 | +void RPCConsole::disconnectSelectedNode()
 691 | +{
 692 | +    QString strNode = GUIUtil::getEntryData(ui->peerWidget, 0, PeerTableModel::Address);
 693 | +    while (CNode *bannedNode = FindNode(strNode.toStdString())) {

laanwj commented at 6:55 AM on June 2, 2015:

Why is this a loop?

Diapolo commented at 7:29 AM on June 2, 2015:

For no reason I guess ^^... will update.

laanwj commented at 6:58 AM on June 2, 2015: member

Scope drift alert: let's focus here on improving the peers table, features such as kicking/banning from the peers list should be a separate feature pull.

Diapolo commented at 7:30 AM on June 2, 2015: none

@laanwj I just found that kicking feature sooo cool and easy I wanted to show it :). But right, I'll open a new pull for it. Did you test/review it?

Edit: Feature now in a seperate pull: #6217

jonasschnelli commented at 7:40 PM on June 2, 2015: contributor

Looks good! Tested ACK.

Nit: One thing which might be worth a short discussion: Because now there are 17 rows of key/value information this would fix the min window height to be slightly above 600px. This would break the peers window on 800x600 screens (which is okay for me, but you never know what kind of screens are used in node only wallet disabled environments).

I think a easy addition would be to deselect a selected peer if the user switches away from peers to console or net graph, etc. (or if he closes the console/debug window). This would re-allow the user to change the height of the window below 600px.

Diapolo commented at 6:33 AM on June 3, 2015: none

<pre> I think a easy addition would be to deselect a selected peer if the user switches away from peers to console or net graph, etc. (or if he closes the console/debug window). This would re-allow the user to change the height of the window below 600px. </pre>

@jonasschnelli Im not sure, how would that change redude the screens size?

jonasschnelli commented at 6:50 AM on June 3, 2015: contributor

@Diapolo: it wouldn't resize the screen automatically, but, it would allow to reduce the height/size below 600px. If you don't deselect the row, the 17row key/value will constraint the height above 600px.

Diapolo commented at 8:40 AM on June 6, 2015: none

@jonasschnelli See commit 3, this adds your suggestion. Switching away from peers tab clears the selection.

[Qt] extend rpc console peers tab

- add node id, ping wait, whitelisted and common height
- rephrase some labels to make them easier to understand for users

1b0db7b984

[Qt] replace Boost foreach with Qt version peertablemodel.cpp 7211adad85

[Qt] deselect peer when switching away from peers tab in RPC console e059726811

Diapolo commented at 9:49 AM on June 12, 2015: none

@laanwj @jonasschnelli Maybe some final ACKs :)?

laanwj commented at 2:49 PM on June 12, 2015: member

ACK. Works for me.

laanwj merged this on Jun 12, 2015

laanwj closed this on Jun 12, 2015

laanwj referenced this in commit ab0ec67903 on Jun 12, 2015

Diapolo deleted the branch on Jun 15, 2015

laanwj referenced this in commit acec9e45c6 on Jan 16, 2019

deadalnix referenced this in commit 952b59ff50 on Nov 28, 2020

Munkybooty referenced this in commit 70096c6cbc on Aug 21, 2021

MarcoFalke locked this on Sep 8, 2021