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

Peer details: connection type follow-ups #180

Merged

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Jan 10, 2021

Three follow-ups to #163:

  • return relay type for inbound peers
  • improve markup handling in the tooltip to facilitate translations
  • update ConnectionType doxygen documentation

Screenshot from 2021-01-11 08-37-44

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@jonatack jonatack force-pushed the peer-details-connection-type-followups branch from 09f4414 to 3bb3ad2 Compare January 10, 2021 16:59
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@@ -459,11 +460,22 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty

ui->splitter->restoreState(settings.value("PeersTabSplitterSizes").toByteArray());

QChar nonbreaking_hyphen(8209);
const QChar nonbreaking_hyphen(8209);
const std::vector<QString> CONNECTION_TYPE_DOC{
Copy link
Member Author

@jonatack jonatack Jan 10, 2021

Choose a reason for hiding this comment

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

Named it CONNECTION_TYPE_DOC to show up in git grep with the constant of the same name in rpc/net.cpp and updated the net.h documentation

@jonatack jonatack force-pushed the peer-details-connection-type-followups branch 2 times, most recently from 4bb8c0c to 1a297a7 Compare January 10, 2021 17:34
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Approach ACK 1a297a7.

src/qt/guiutil.cpp Outdated Show resolved Hide resolved
@jonatack jonatack force-pushed the peer-details-connection-type-followups branch from 1a297a7 to e711200 Compare January 10, 2021 18:07
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK e711200, tested on Linux Mint 20.1 (Qt 5.12.8):

Screenshot from 2021-01-10 21-32-34

src/net.h Show resolved Hide resolved
src/qt/guiutil.cpp Show resolved Hide resolved
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@jonatack jonatack force-pushed the peer-details-connection-type-followups branch from e711200 to 79a2576 Compare January 10, 2021 20:40
@jonatack
Copy link
Member Author

jonatack commented Jan 10, 2021

Thanks @hebasto! Updated.

git diff e711200 79a2576

diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
index 4fdc74c48..44addeeda 100644
--- a/src/qt/rpcconsole.cpp
+++ b/src/qt/rpcconsole.cpp
@@ -445,7 +445,7 @@ void RPCExecutor::request(const QString &command, const WalletModel* wallet_mode
     }
 }
 
-RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle* _platformStyle, QWidget* parent) :
+RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformStyle, QWidget *parent) :
     QWidget(parent),
     m_node(node),
     ui(new Ui::RPCConsole),
@@ -460,9 +460,9 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle* _platformSty
 
     ui->splitter->restoreState(settings.value("PeersTabSplitterSizes").toByteArray());
 
-    const QChar nonbreaking_hyphen(8209);
+    constexpr QChar nonbreaking_hyphen(8209);
     const std::vector<QString> CONNECTION_TYPE_DOC{
-        tr("Inbound: initiated by peer"),
+        tr("Inbound Full/Block Relay: initiated by peer"),
         tr("Outbound Full Relay: default"),
         tr("Outbound Block Relay: does not relay transactions or addresses"),
         tr("Outbound Manual: added using RPC %1 or %2/%3 configuration options")

Screenshot from 2021-01-11 08-37-44

Copy link
Member

@hebasto hebasto 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 79a2576, only suggested changes since my previous review.

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.

Concept ACK.

src/qt/forms/debugwindow.ui Show resolved Hide resolved
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.

ACK 79a2576, tested on macOS 11.1 with Qt 5.15.2

Screen Shot 2021-01-15 at 3 08 51 PM

@RandyMcMillan
Copy link
Contributor

I don't believe this much information should be displayed in a mouse over event.
Attaching a click event to a UILabel (or a question mark image) would be a better approach (imo).
The click event could trigger a modal view that gives a more detailed explanation of
the information contained in the entire panel.

@jonatack
Copy link
Member Author

This pull doesn't add the tooltip. It makes these follow-ups to #163:

  • return relay type for inbound peers
  • improve markup handling in the tooltip to facilitate translations
  • update ConnectionType doxygen documentation

(Feel free to open a pull to propose your approach.)

@jonatack
Copy link
Member Author

This has 2 ACKs from @hebasto and @jarolrod and a Concept ACK from @promag. Anyone wish to ACK?

Copy link
Member

@laanwj laanwj left a comment

Choose a reason for hiding this comment

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

Code review ACK 79a2576

@laanwj laanwj merged commit d1ddead into bitcoin-core:master Jan 27, 2021
@jonatack jonatack deleted the peer-details-connection-type-followups branch January 27, 2021 19:54
@jonatack
Copy link
Member Author

Thanks!

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 28, 2021
79a2576 doc: update ConnectionType Doxygen documentation (Jon Atack)
f3153dc gui: improve markup handling of connection type tooltip (Jon Atack)
4f09615 gui: return inbound {full, block} relay type in peer details (Jon Atack)

Pull request description:

  Three follow-ups to #163:
  - return relay type for inbound peers
  - improve markup handling in the tooltip to facilitate translations
  - update ConnectionType doxygen documentation

  ![Screenshot from 2021-01-11 08-37-44](https://user-images.githubusercontent.com/2415484/104156081-50e69300-53e0-11eb-9b0f-880cb5626d68.png)

ACKs for top commit:
  hebasto:
    re-ACK 79a2576, only suggested changes since my [previous](bitcoin-core/gui#180 (review)) review.
  jarolrod:
    ACK 79a2576, tested on macOS 11.1 with Qt 5.15.2
  laanwj:
    Code review ACK 79a2576

Tree-SHA512: 4a8d8f8bfbaefd68e8d1bf3b20d29e4a8e8cfe97b2f8d59d3a4c338a50b61de0a67d97bd8646c04bd5df5a9679c4954b9b46e7cba24bb89f4d0e44e94cf9d66c
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants