Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide peers details #505

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RandyMcMillan
Copy link
Contributor

@RandyMcMillan RandyMcMillan commented Dec 12, 2021

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 <w0xlt@users.noreply.github.com>

@RandyMcMillan
Copy link
Contributor Author

RandyMcMillan commented Dec 12, 2021

Screen Shot 2021-12-12 at 6 41 53 PM

@RandyMcMillan
Copy link
Contributor Author

Tooltip message for better UX.

Screen Shot 2021-12-12 at 6 45 24 PM

@jarolrod jarolrod added Feature UX All about "how to get things done" labels Dec 13, 2021
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -555,6 +555,7 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
ui->lineEdit->setMaxLength(16 * 1024 * 1024);
ui->messagesWidget->installEventFilter(this);

connect(ui->hidePeersDetailButton, &QAbstractButton::clicked, [this] { hidePeersDetail(); });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just

connect(ui->hidePeersDetailButton, &QAbstractButton::clicked, this, &RPCConsole::clearSelectedNode);

and ditch hidePeersDetails.

@RandyMcMillan
Copy link
Contributor Author

RandyMcMillan commented Dec 13, 2021

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.

close file icon close file icon

@luke-jr
Copy link
Member

luke-jr commented Dec 13, 2021

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?

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concept ACK

@luke-jr

Since it's possible to select multiple peers, but only one shows in the panel

Since #13 this is not possible. If multiple peers are selected, the panel will not show

src/qt/forms/debugwindow.ui Outdated Show resolved Hide resolved
src/qt/forms/debugwindow.ui Outdated Show resolved Hide resolved
@RandyMcMillan
Copy link
Contributor Author

RandyMcMillan commented Dec 14, 2021

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.

@RandyMcMillan RandyMcMillan marked this pull request as draft December 14, 2021 22:07
@RandyMcMillan RandyMcMillan marked this pull request as ready for review December 22, 2021 01:28
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 30, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK pablomartin4btc
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.

Conflicts

No conflicts as of last run.

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 205c965 on Ubuntu 21.10 (Qt 5.15.2).

@RandyMcMillan
Copy link
Contributor Author

@w0xlt any suggestions on reducing the diff in the .ui file would be appreciated - this is left unsquashed to allow for suggestions/variations

@w0xlt
Copy link
Contributor

w0xlt commented Jan 28, 2022

@RandyMcMillan

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">

@RandyMcMillan RandyMcMillan changed the title RPCConsole: add hidePeersDetail() button and functionality RPCConsole: add hidePeersDetailButton and connect to RPCConsole::clearSelectedNode Jan 29, 2022
@RandyMcMillan
Copy link
Contributor Author

@w0xlt - applied the patch - thanks for the collaboration! attribution included in commit message.
I also implemented @promag's suggestion - much more prudent approach! - thanks!

@RandyMcMillan
Copy link
Contributor Author

RandyMcMillan commented Jan 29, 2022

Commit: 2ae79d0

screen shot screen shot

@rsafier
Copy link

rsafier commented Jan 29, 2022

tACK 2ae79d0 on ARM M1/macOS/Monterey 12.1 (Qt 5.15.2)

Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK 2ae79d0

@RandyMcMillan
Copy link
Contributor Author

To avoid re-adding diff junk to the debugwindow.ui file - I prefer to add translator comments in a follow up PR.

@RandyMcMillan RandyMcMillan marked this pull request as draft February 17, 2022 15:10
@hebasto
Copy link
Member

hebasto commented Oct 26, 2022

@RandyMcMillan Still working on this PR?

@RandyMcMillan RandyMcMillan changed the title Add close Peers detail panel feature Hide Peers Detail Button Sep 20, 2023
@DrahtBot DrahtBot requested review from w0xlt and removed request for w0xlt September 20, 2023 14:50
@RandyMcMillan
Copy link
Contributor Author

@pablomartin4btc - I am just grateful for the interest - I had forgotten about this PR :)

retesting on MacOS X86_64 Arm64 right now.

@DrahtBot DrahtBot requested review from w0xlt and removed request for w0xlt September 20, 2023 15:44
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 5, 2024

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

@hebasto
Copy link
Member

hebasto commented Feb 11, 2024

This #505 (comment) still needs to be addressed.

@DrahtBot DrahtBot requested review from w0xlt and removed request for w0xlt February 11, 2024 22:12
@RandyMcMillan
Copy link
Contributor Author

RandyMcMillan commented Mar 28, 2024

436ac4f duplicate ui element removed per @pablomartin4btc comment.

cc: @hebasto @w0xlt

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 436ac4f

UI look & feel.
  • General view.

    image

  • Tooltip.

    image

  • Focus.

    image

nit: not a blocker, please consider previous feedback regarding commit title length and meaningfulness, maybe gui: + this PR's title.

@RandyMcMillan
Copy link
Contributor Author

Thanks I saw that - I can update the commit message...

@DrahtBot
Copy link
Contributor

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

Debug: https://github.com/bitcoin-core/gui/runs/23215732626

@RandyMcMillan RandyMcMillan changed the title Hide Peers Detail Button gui: Hide peers details Apr 9, 2024
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 bitcoin-core#485:

Co-authored-by: @w0xlt <w0xlt@users.noreply.github.com>
@RandyMcMillan
Copy link
Contributor Author

41a1a86: update per previous suggestions

tACK 436ac4f

UI look & feel.
nit: not a blocker, please consider previous feedback regarding commit title length and meaningfulness, maybe gui: + this PR's title.

@RandyMcMillan RandyMcMillan changed the title gui: Hide peers details Hide peers details Apr 9, 2024
Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re ACK 41a1a86

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature UX All about "how to get things done"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No obvious way to close peer detail panel
10 participants