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

Add address relay/processed/rate-limited fields to peer details #526

Merged
merged 4 commits into from
Jan 28, 2022

Conversation

jonatack
Copy link
Member

This pull adds the following address fields in rpc getpeerinfo and cli -netinfo to the gui peers details:

  • Address Relay (Yes/No)
  • Addresses Processed (integer)
  • Addresses Rate-Limited (integer)

and uses the additional horizontal space to display "Last Transaction" (instead of "Last Tx").

Screenshot from 2022-01-21 00-05-49

@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #505 (RPCConsole: add hidePeersDetail() button and functionality by RandyMcMillan)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot mentioned this pull request Jan 21, 2022
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 5efd391 on Ubuntu 21.10 (Qt 5.15.2).

@hebasto
Copy link
Member

hebasto commented Jan 24, 2022

Concept ACK, the added fields correspond to the getpeerinfo RPC output.

@jonatack jonatack force-pushed the add-addr-fields-to-peer-details branch from a00bc71 to 9fbd1bb Compare January 24, 2022 09:57
@jonatack
Copy link
Member Author

Thanks @w0xlt and @hebasto, updated.

git diff a00bc71 9fbd1bb

diff --git a/src/qt/forms/debugwindow.ui b/src/qt/forms/debugwindow.ui
index 174954087..219680102 100644
--- a/src/qt/forms/debugwindow.ui
+++ b/src/qt/forms/debugwindow.ui
@@ -1355,7 +1355,7 @@
                <item row="13" column="0">
                 <widget class="QLabel" name="peerLastTxLabel">
                  <property name="toolTip">
-                  <string>Elapsed time since a novel transaction accepted into our mempool was received from this peer.</string>
+                  <string extracomment="Tooltip text for the Last Transaction field in the peer details area.">Elapsed time since a novel transaction accepted into our mempool was received from this peer.</string>
                  </property>
                  <property name="text">
                   <string>Last Transaction</string>
@@ -1594,7 +1594,7 @@
                <item row="23" column="0">
                 <widget class="QLabel" name="peerAddrRelayEnabledLabel">
                  <property name="toolTip">
-                  <string>Whether we relay addresses to this peer.</string>
+                  <string extracomment="Tooltip text for the Address Relay field in the peer details area.">Whether we relay addresses to this peer.</string>
                  </property>
                  <property name="text">
                   <string>Address Relay</string>
@@ -1620,7 +1620,7 @@
                <item row="24" column="0">
                 <widget class="QLabel" name="peerAddrProcessedLabel">
                  <property name="toolTip">
-                  <string>Total number of addresses processed, excluding those dropped due to rate limiting.</string>
+                  <string extracomment="Tooltip text for the Addresses Processed field in the peer details area.">Total number of addresses processed, excluding those dropped due to rate-limiting.</string>
                  </property>
                  <property name="text">
                   <string>Addresses Processed</string>
@@ -1646,7 +1646,7 @@
                <item row="25" column="0">
                 <widget class="QLabel" name="peerAddrRateLimitedLabel">
                  <property name="toolTip">
-                  <string>Total number of addresses dropped due to rate limiting.</string>
+                  <string extracomment="Tooltip text for the Addresses Rate-Limited field in the peer details area.">Total number of addresses dropped due to rate-limiting.</string>
                  </property>
                  <property name="text">
                   <string>Addresses Rate-Limited</string>

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 9fbd1bb, tested on Ubuntu 21.10 (Qt 5.15.2).

@hebasto hebasto added this to the 23.0 milestone Jan 26, 2022
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 9fbd1bb

@hebasto hebasto merged commit 5b4b8f7 into bitcoin-core:master Jan 28, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 29, 2022
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.

Cross-posted feedback from French translation team.

<string extracomment="Tooltip text for the Addresses Processed field in the peer details area.">Total number of addresses processed, excluding those dropped due to rate-limiting.</string>
</property>
<property name="text">
<string>Addresses Processed</string>
Copy link
Member

@hebasto hebasto Feb 17, 2022

Choose a reason for hiding this comment

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

A feedback from a Transifex translator:

Rather: "Processed Addresses"

Copy link
Member Author

Choose a reason for hiding this comment

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

If "processed" was an adjective, then I would agree with the feedback, i.e. "red car" and not "car red".

However, in this case it is a verb and this is fine as-is, i.e. "cars sold" vs "sold cars".

Choose a reason for hiding this comment

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

Hi @jonatack The lack of string context in Transifex is a real problem for this project, as proper context (dev notes and/or screenshots) are essential to achieve quality translations.
How is this string used, is this a title, a tooltip, a notification? Do we actually mean “Addresses are processed”?

Copy link
Member Author

@jonatack jonatack Feb 20, 2022

Choose a reason for hiding this comment

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

@AO-LocLab it is the field name in the peer details area, as displayed in the screenshot in the PR description. There is also a tooltip saying this, and its explanatory description, 3 lines higher at line 1623. There is one of these for each added field. I don't know if they are visible in transifex.

<string extracomment="Tooltip text for the Addresses Processed field in the peer details area.">
    Total number of addresses processed, excluding those dropped due to rate-limiting.</string>

Copy link
Member

Choose a reason for hiding this comment

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

I don't know if they are visible in transifex.

Screenshot from 2022-02-20 19-09-24

Copy link
Member

Choose a reason for hiding this comment

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

Although, this one has no developer notes:

Should an extracomment be added, or something else?

Actually, extracomments in *.ui files are Qt translator comments which are presented on Transifex as "developer notes".

Copy link
Member

@hebasto hebasto Feb 20, 2022

Choose a reason for hiding this comment

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

Should an extracomment be added...?

That is the plan :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok! Will propose adding extracomments to the titles, then (if I understand correctly) and maybe to the tooltips without them.

Copy link
Member

Choose a reason for hiding this comment

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

Ok! Will propose adding extracomments to the titles, then (if I understand correctly) and maybe to the tooltips without them.

Thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

Started adding and improving the dev notes (and tooltips) in #554.

@jonatack jonatack deleted the add-addr-fields-to-peer-details branch February 17, 2022 09:11
hebasto added a commit that referenced this pull request Mar 20, 2022
…s tab address fields

4d2b503 gui: improve "Addresses Rate-Limited" translator comments and tooltip in peers tab (Jon Atack)
81ef1f7 gui: improve "Addresses Processed" translator comments and tooltip in peers tab (Jon Atack)
77f24aa gui: improve "Address Relay" translator comments and tooltip in peers tab (Jon Atack)

Pull request description:

  Per translator feedback in this thread: #526 (comment)

  *"The lack of string context in Transifex is a real problem for this project, as proper context (dev notes and/or screenshots) are essential to achieve quality translations."*

  This pull adds developer notes for transifex translators via `extracomment` tags, and it improves the existing ones and their tooltips with more context, clarity and completeness for the following peer tab fields as a follow-up to #526:

  - address relay
  - addresses processed
  - addressed rate-limited

  It looks like only six lines of diff, but they are loooong lines.

  If this is the right direction, the same can be done for other fields in follow-ups.

ACKs for top commit:
  jarolrod:
    re-ACK [4d2b503](4d2b503)
  hebasto:
    ACK 4d2b503, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: a185f46a66375a5fd6854640745b7d1d00740cf7be58db03256f44d71acc351e1770de137cb3bc9c1f0ea3cabd7cfa1cb1ccb87ec0df222680924ca3dab6c8bf
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 22, 2022
…ooltips for peers tab address fields

4d2b503 gui: improve "Addresses Rate-Limited" translator comments and tooltip in peers tab (Jon Atack)
81ef1f7 gui: improve "Addresses Processed" translator comments and tooltip in peers tab (Jon Atack)
77f24aa gui: improve "Address Relay" translator comments and tooltip in peers tab (Jon Atack)

Pull request description:

  Per translator feedback in this thread: bitcoin-core/gui#526 (comment)

  *"The lack of string context in Transifex is a real problem for this project, as proper context (dev notes and/or screenshots) are essential to achieve quality translations."*

  This pull adds developer notes for transifex translators via `extracomment` tags, and it improves the existing ones and their tooltips with more context, clarity and completeness for the following peer tab fields as a follow-up to bitcoin-core/gui#526:

  - address relay
  - addresses processed
  - addressed rate-limited

  It looks like only six lines of diff, but they are loooong lines.

  If this is the right direction, the same can be done for other fields in follow-ups.

ACKs for top commit:
  jarolrod:
    re-ACK [4d2b503](bitcoin-core/gui@4d2b503)
  hebasto:
    ACK 4d2b503, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: a185f46a66375a5fd6854640745b7d1d00740cf7be58db03256f44d71acc351e1770de137cb3bc9c1f0ea3cabd7cfa1cb1ccb87ec0df222680924ca3dab6c8bf
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Feb 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants