-
Notifications
You must be signed in to change notification settings - Fork 261
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
debugwindow: update session ID tooltip #788
Conversation
remove "if any"
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
nit: @MarnixCroes, maybe you can add the screenshot for transport v1 and session ID is empty/ not shown, if possible, even this PR doesn't affect that logic.
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3bf00e1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
ACK 3bf00e1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3bf00e1
What if the connection type is still The code reference: gui/src/node/connection_types.h Lines 83 to 88 in 6737331
|
then there is no session ID property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 3bf00e1
Tested on Ubuntu 22.04 using signet
(./src/qt/bitcoin-qt -signet
) otherwise I couldn't find v2
peers on testnet
.
v2
transport with Session ID field displayed:
v1
transport without it:
@hebasto this PR only updates peerSessionIdLabel
tooltip; this widget and the "Session ID" one, right below on the ui
form, won't be shown on "DETECTING
" TransportProtocolType enum
:
Lines 1208 to 1215 in d04324a
if (stats->nodeStats.m_transport_type == TransportProtocolType::V2) { | |
ui->peerSessionIdLabel->setVisible(true); | |
ui->peerSessionId->setVisible(true); | |
ui->peerSessionId->setText(QString::fromStdString(stats->nodeStats.m_session_id)); | |
} else { | |
ui->peerSessionIdLabel->setVisible(false); | |
ui->peerSessionId->setVisible(false); | |
} |
nit: you could expand a bit on the details of the reason of the removal of "if any" from the label in the commit body (i.e. as it's only shown in v2 transport which will always have a session id). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3bf00e1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Help!
When you have a v2 connection, there is always a session ID.
the if any is a leftover from #754, where the session ID property initially would always be displayed (transport v1 and v2).
So the session ID could be empty when you have a v1 connection.
As now the Session ID property only is displayed for v2 connection, there will always be a session ID.
master
PR
Session ID not shown when transport v1