[Qt] extend rpc console peers tab #6209

Merged
merged 3 commits into from Jun 12, 2015

Conversation

Projects
None yet
4 participants
@Diapolo

Diapolo commented Jun 1, 2015

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

peers

@laanwj laanwj added the GUI label Jun 1, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 1, 2015

Member

Post-0.11 utACK

Member

laanwj commented Jun 1, 2015

Post-0.11 utACK

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 1, 2015

Member

utACK
Will test soon.

Member

jonasschnelli commented Jun 1, 2015

utACK
Will test soon.

src/qt/forms/rpcconsole.ui
+ </widget>
+ </item>
+ <item row="0" column="2">
+ <widget class="QCheckBox" name="peerWhitelisted">

This comment has been minimized.

@laanwj

laanwj Jun 1, 2015

Member

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

@laanwj

laanwj Jun 1, 2015

Member

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 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.

Member

laanwj commented Jun 1, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 1, 2015

Member

I would prefer the checkbox – but only – if you could also set the state. Together with banning options (#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.

Member

jonasschnelli commented Jun 1, 2015

I would prefer the checkbox – but only – if you could also set the state. Together with banning options (#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

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 1, 2015

@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 Jun 1, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 1, 2015

@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 :).

Diapolo commented Jun 1, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 1, 2015

Member

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

Member

jonasschnelli commented Jun 1, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 1, 2015

Member

@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.

Member

jonasschnelli commented Jun 1, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 1, 2015

@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 Jun 1, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 1, 2015

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 Jun 1, 2015

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

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 1, 2015

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

Diapolo commented Jun 1, 2015

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

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 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?

Member

luke-jr commented Jun 2, 2015

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

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 2, 2015

@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 ^^.

Diapolo commented Jun 2, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 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)

Member

laanwj commented Jun 2, 2015

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)

src/qt/rpcconsole.cpp
+void RPCConsole::disconnectSelectedNode()
+{
+ QString strNode = GUIUtil::getEntryData(ui->peerWidget, 0, PeerTableModel::Address);
+ while (CNode *bannedNode = FindNode(strNode.toStdString())) {

This comment has been minimized.

@laanwj

laanwj Jun 2, 2015

Member

Why is this a loop?

@laanwj

laanwj Jun 2, 2015

Member

Why is this a loop?

This comment has been minimized.

@Diapolo

Diapolo Jun 2, 2015

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

@Diapolo

Diapolo Jun 2, 2015

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 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.

Member

laanwj commented Jun 2, 2015

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

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 2, 2015

@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

Diapolo commented Jun 2, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 2, 2015

Member

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.

Member

jonasschnelli commented Jun 2, 2015

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

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 3, 2015

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.

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

Diapolo commented Jun 3, 2015

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.

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 3, 2015

Member

@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.

Member

jonasschnelli commented Jun 3, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 6, 2015

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

Diapolo commented Jun 6, 2015

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

Philip Kaufmann added some commits Jun 1, 2015

Philip Kaufmann
[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
@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Jun 12, 2015

@laanwj @jonasschnelli Maybe some final ACKs :)?

Diapolo commented Jun 12, 2015

@laanwj @jonasschnelli Maybe some final ACKs :)?

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 12, 2015

Member

ACK. Works for me.

Member

laanwj commented Jun 12, 2015

ACK. Works for me.

@laanwj laanwj merged commit e059726 into bitcoin:master Jun 12, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 12, 2015

Merge pull request #6209
e059726 [Qt] deselect peer when switching away from peers tab in RPC console (Philip Kaufmann)
7211ada [Qt] replace Boost foreach with Qt version peertablemodel.cpp (Philip Kaufmann)
1b0db7b [Qt] extend rpc console peers tab (Philip Kaufmann)

@Diapolo Diapolo deleted the Diapolo:Qt_peers branch Jun 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment