Add a close (X) button to the Peers Detail panel. Reuse the same icon used in the Console Tab. The close button deselects the peer highlighted in the PeerTableView and hides the detail panel.
fixes #485
Co-authored-by: [@w0xlt](/bitcoin-core-gui/contributor/w0xlt/) <w0xlt@users.noreply.github.com>
Tooltip message for better UX.
554 | @@ -555,6 +555,7 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty 555 | ui->lineEdit->setMaxLength(16 * 1024 * 1024); 556 | ui->messagesWidget->installEventFilter(this); 557 | 558 | + connect(ui->hidePeersDetailButton, &QAbstractButton::clicked, [this] { hidePeersDetail(); });
Just
connect(ui->hidePeersDetailButton, &QAbstractButton::clicked, this, &RPCConsole::clearSelectedNode);
and ditch hidePeersDetails.
We reuse the same icon used in the Console Tab for Gui consistency.
How so? One clears the console while the other unselects the peer.
Is there a better icon to use for a "close" action?
We don't have a "clear" icon in the repo - but something like this would be more appropriate for "clear console". So reusing the (X) icon seems prudent.
<details> <summary>close file icon</summary> <img src="https://user-images.githubusercontent.com/152159/145851539-16d15aad-7ae2-409a-abb5-ecd806d2c5e8.jpeg" alt="close file icon"> </p> </details>
Since it's possible to select multiple peers, but only one shows in the panel, perhaps leaving the "close" button is best, but don't have it deselect?
1022 | - </property> 1023 | - <property name="textInteractionFlags"> 1024 | - <set>Qt::LinksAccessibleByMouse|Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse</set> 1025 | - </property> 1026 | - </widget> 1027 | + <item>
could you try to minimize the diff on this file, I don't think all this moving around and changes would be required to add the button. QtCreator creates a lot of junk, try to pinpoint the minimal diff required to add the button.
To avoid re-adding diff junk to the debugwindow.ui file - I prefer to add translator comments in a follow up PR.
1100 | + <width>32</width> 1101 | + <height>32</height> 1102 | + </size> 1103 | + </property> 1104 | + <property name="toolTip"> 1105 | + <string>Hide Peers Detail</string>
you can add a translator comment to this string by using the extracomment property. I have written some notes on constructing translator comments here: translator-comments.md
@jarolrod - if you want to post a patch - I will apply it and give you attribution.
@jarolrod - I will do a follow up for translator comments.
I split this PR into 2 commits so others can experiment with adding the button to src/qt/forms/debugwindow.ui Feel free to fetch this branch and experiment.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | pablomartin4btc, hebasto |
| Concept ACK | jarolrod |
| Stale ACK | rsafier, w0xlt, hernanmarino |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
tACK 205c965 on Ubuntu 21.10 (Qt 5.15.2).
@w0xlt any suggestions on reducing the diff in the .ui file would be appreciated - this is left unsquashed to allow for suggestions/variations
You can delete the last commit (git reset --hard 718589f) and apply the patch below.
diff --git a/src/qt/forms/debugwindow.ui b/src/qt/forms/debugwindow.ui
index 15e0d3fad..ad671b02e 100644
--- a/src/qt/forms/debugwindow.ui
+++ b/src/qt/forms/debugwindow.ui
@@ -984,6 +984,117 @@
</size>
</property>
<layout class="QVBoxLayout" name="verticalLayout_8">
+ <item>
+ <layout class="QHBoxLayout" name="horizontalLayout_8">
+ <property name="topMargin">
+ <number>0</number>
+ </property>
+ <property name="bottomMargin">
+ <number>0</number>
+ </property>
+ <item>
+ <widget class="QLabel" name="peerHeading">
+ <property name="sizePolicy">
+ <sizepolicy hsizetype="Preferred" vsizetype="Minimum">
+ <horstretch>0</horstretch>
+ <verstretch>0</verstretch>
+ </sizepolicy>
+ </property>
+ <property name="minimumSize">
+ <size>
+ <width>0</width>
+ <height>32</height>
+ </size>
+ </property>
+ <property name="font">
+ <font>
+ <pointsize>10</pointsize>
+ </font>
+ </property>
+ <property name="cursor">
+ <cursorShape>IBeamCursor</cursorShape>
+ </property>
+ <property name="text">
+ <string>Select a peer to view detailed information.</string>
+ </property>
+ <property name="alignment">
+ <set>Qt::AlignHCenter|Qt::AlignTop</set>
+ </property>
+ <property name="wordWrap">
+ <bool>true</bool>
+ </property>
+ <property name="textInteractionFlags">
+ <set>Qt::LinksAccessibleByMouse|Qt::TextSelectableByKeyboard|Qt::TextSelectableByMouse</set>
+ </property>
+ </widget>
+ </item>
+ <item>
+ <widget class="QWidget" name="hidePeersDetail" native="true">
+ <property name="enabled">
+ <bool>true</bool>
+ </property>
+ <property name="minimumSize">
+ <size>
+ <width>32</width>
+ <height>32</height>
+ </size>
+ </property>
+ <property name="maximumSize">
+ <size>
+ <width>32</width>
+ <height>32</height>
+ </size>
+ </property>
+ <widget class="QToolButton" name="hidePeersDetailButton">
+ <property name="geometry">
+ <rect>
+ <x>0</x>
+ <y>0</y>
+ <width>32</width>
+ <height>32</height>
+ </rect>
+ </property>
+ <property name="minimumSize">
+ <size>
+ <width>32</width>
+ <height>32</height>
+ </size>
+ </property>
+ <property name="baseSize">
+ <size>
+ <width>32</width>
+ <height>32</height>
+ </size>
+ </property>
+ <property name="toolTip">
+ <string>Hide Peers Detail</string>
+ </property>
+ <property name="layoutDirection">
+ <enum>Qt::LeftToRight</enum>
+ </property>
+ <property name="text">
+ <string />
+ </property>
+ <property name="icon">
+ <iconset resource="../bitcoin.qrc">
+ <normaloff>:/icons/remove</normaloff>
+ :/icons/remove
+ </iconset>
+ </property>
+ <property name="iconSize">
+ <size>
+ <width>22</width>
+ <height>22</height>
+ </size>
+ </property>
+ <property name="shortcut">
+ <string>Ctrl+X</string>
+ </property>
+ </widget>
+ </widget>
+ </item>
+ </layout>
+ </item>
<item>
<widget class="QLabel" name="peerHeading">
<property name="sizePolicy">
Commit: 2ae79d0954c46862535114c0d3f7bcfe2837918f
<details> <summary>screen shot</summary> <img src="https://user-images.githubusercontent.com/152159/151678025-71d43bb0-2786-4420-9c23-5a7cb10f4991.png" alt="screen shot"> </p> </details>
tACK 2ae79d0 on ARM M1/macOS/Monterey 12.1 (Qt 5.15.2)
reACK 2ae79d0
To avoid re-adding diff junk to the debugwindow.ui file - I prefer to add translator comments in a follow up PR.
@RandyMcMillan Still working on this PR?
rebase: d59363aaa0f39da8f27fead0b8e5b0c1a5a32226
utACK d59363aaa0f39da8f27fead0b8e5b0c1a5a32226 I 'd like to see this implemented.
991 | + </property> 992 | + <property name="bottomMargin"> 993 | + <number>0</number> 994 | + </property> 995 | + <item> 996 | + <widget class="QLabel" name="peerHeading">
Looks like this structure is duplicated, is this correct?
thanks - taking a look right now.
thank you!
tACK d59363aaa0f39da8f27fead0b8e5b0c1a5a32226
nit: I'd update both the title of the PR and the commit subject line for something more meaningful or concise, it would provide clarity of the changes being made. In the specific case of the commit subject line it's recommended to keep its length 50 chars max according to the guidelines. Perhaps something like gui: Add close Peers detail panel feature could work.
If you mention "fix" follow by the issue # it will link them both together.
@pablomartin4btc - great suggestion - thanks!
@pablomartin4btc - great suggestion - thanks!
You are welcome, the "gui:" prefix, IMO, would be more for the commit subject line, since we are already on the gui repo, we would't need to clarify it in the PR title.
@pablomartin4btc - I am just grateful for the interest - I had forgotten about this PR :)
retesting on MacOS X86_64 Arm64 right now.
<!--2e250dc3d92b2c9115b66051148d6e47-->
🤔 There hasn't been much activity lately and the CI seems to be failing.
If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.
This #505 (review) still needs to be addressed.
436ac4f duplicate ui element removed per @pablomartin4btc comment.
tACK 436ac4f42b88e8131bf1e02554a20b3c549e7d6f
<details> <summary>UI look & feel.</summary> <ul> <li><details><summary>General view.</summary>
</details></li>
<li><details><summary>Tooltip.</summary>
</details></li>
<li><details><summary>Focus.</summary>
</details></li> </ul> </details>
<u>nit</u>: not a blocker, please consider previous feedback regarding commit title length and meaningfulness, maybe gui: + this PR's title.
Thanks I saw that - I can update the commit message...
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
<sub>Debug: https://github.com/bitcoin-core/gui/runs/23215732626</sub>
Add a close (X) button to the Peers Detail panel.
Reuse the same icon used in the Console Tab.
The close button deselects the peer highlighted
in the PeerTableView and hides the detail panel.
This PR addresses issue #485:
Co-authored-by: @w0xlt <w0xlt@users.noreply.github.com>
re ACK 41a1a8615dd48fdd9811b9824c49ceb934c6375e
ACK 41a1a8615dd48fdd9811b9824c49ceb934c6375e, tested on Ubuntu 23.10.
thanks!