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 Copy address Peers Tab Context Menu Action #318

Merged
merged 1 commit into from
Sep 12, 2021

Conversation

jarolrod
Copy link
Member

@jarolrod jarolrod commented May 5, 2021

Picking up #264

This adds a Copy Address context menu action to the Peers Tab.

Based on the first commit of PR #317 so that we can use Qt::DisplayRole in the copyEntryData function.

Master PR
Screen Shot 2021-05-05 at 4 51 11 AM Screen Shot 2021-05-05 at 4 49 15 AM

@hebasto hebasto added the UX All about "how to get things done" label May 9, 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.

Concept ACK. But I'm not fond of the base commit "gui: add Direction column to peers tab" - I understand it simplifies this change.

@jarolrod
Copy link
Member Author

updated from 6ee6d2f -> d6099b7 (pr318.01, pr318.02, diff)

@jarolrod
Copy link
Member Author

updated from d6099b7 -> 85e0f95 (pr318.02 -> pr318.03)

Changes:

@jarolrod
Copy link
Member Author

updated from 85e0f95 -> 65d1d35 (pr318.03 -> pr318.04, diff)

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@promag
Copy link
Contributor

promag commented Sep 5, 2021

@jarolrod needs rebase, this is a cool change.

@jonatack
Copy link
Member

jonatack commented Sep 5, 2021

Concept ACK

Co-authored-by: Jarol Rodriguez <jarolrod@tutanota.com>
@jarolrod
Copy link
Member Author

jarolrod commented Sep 5, 2021

updated from 65d1d35 -> 3ec061d (pr318.04 -> pr318.05)

Changes:

Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

tACK 3ec061d
Tested on Ubuntu 20.04 (Using Qt version 5.12.8)

This PR adds the functionality to copy the address of a peer from the peertable itself. Tested this PR on Ubuntu 20.04. And I was able to observe the change working correctly.

Master PR
Master Screenshot from 2021-09-10 20-09-40

I agree with the changes suggested in this PR. After #384 being merged, I think it is the next logical step to also having the functionality of copying the address of peer from peertable using the context menu action.

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

utACK 3ec061d

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 3ec061d, tested on Linux Mint 20.2 (Qt 5.12.8):

Screenshot from 2021-09-12 18-49-30

@@ -680,6 +680,11 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_

// create peer table context menu
peersTableContextMenu = new QMenu(this);
//: Context menu action to copy the address of a peer
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
//: Context menu action to copy the address of a peer
//: Context menu action to copy the address of a peer.

@hebasto hebasto merged commit ee1db7b into bitcoin-core:master Sep 12, 2021
@jonatack
Copy link
Member

Nice and useful change!

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 13, 2021
… Action

3ec061d qt: Add "Copy address" item to the context menu in the Peers table (Hennadii Stepanov)

Pull request description:

  Picking up #264

  This adds a `Copy Address` context menu action to the `Peers Tab`.

  Based on the first commit of PR #317 so that we can use `Qt::DisplayRole` in the `copyEntryData` function.

  | Master        | PR               |
  | ----------- | ----------- |
  |  ![Screen Shot 2021-05-05 at 4 51 11 AM](https://user-images.githubusercontent.com/23396902/117117822-fb067400-ad5d-11eb-9466-228456108e52.png) | ![Screen Shot 2021-05-05 at 4 49 15 AM](https://user-images.githubusercontent.com/23396902/117117835-fe99fb00-ad5d-11eb-8de0-f6a9acdbf40e.png) |

ACKs for top commit:
  shaavan:
    tACK 3ec061d
  luke-jr:
    utACK 3ec061d
  hebasto:
    ACK 3ec061d, tested on Linux Mint 20.2 (Qt 5.12.8):

Tree-SHA512: be0d7324592aae3928fa3cc522294f17226419fe8cbe3587df12a36bd4fa9c81bead377b13051e950b9a3fcd290b273861e70d6c76b75cdf76eaf58224b834cd
hebasto added a commit that referenced this pull request Sep 14, 2021
5cc783f qt: ensure translator comments end in full stop (Jarol Rodriguez)

Pull request description:

  This is a follow-up to #318 which addresses this [nit](#318 (comment)) by addressing it globally.

  This ensures that all GUI translator comments end in a full stop. If a comment does not end in a full stop, a translator may think  that the rest of the comment is being cut off.

  While here, add a colon to the word "see" for any comments touched which point to look at a link.

ACKs for top commit:
  hebasto:
    ACK 5cc783f, I have reviewed the code and it looks OK, I agree it can be merged.
  shaavan:
    Code Review ACK 5cc783f

Tree-SHA512: 67a1d56175c974e0af9b460fa44163f7ce139a7b81cfaf8ed2c0e7fb6d5120957c3135d96010aeb6229689468e36673fe9571b5a8c3e1c07e047aba1bd563444
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 15, 2021
5cc783f qt: ensure translator comments end in full stop (Jarol Rodriguez)

Pull request description:

  This is a follow-up to #318 which addresses this [nit](bitcoin-core/gui#318 (comment)) by addressing it globally.

  This ensures that all GUI translator comments end in a full stop. If a comment does not end in a full stop, a translator may think  that the rest of the comment is being cut off.

  While here, add a colon to the word "see" for any comments touched which point to look at a link.

ACKs for top commit:
  hebasto:
    ACK 5cc783f, I have reviewed the code and it looks OK, I agree it can be merged.
  shaavan:
    Code Review ACK 5cc783f

Tree-SHA512: 67a1d56175c974e0af9b460fa44163f7ce139a7b81cfaf8ed2c0e7fb6d5120957c3135d96010aeb6229689468e36673fe9571b5a8c3e1c07e047aba1bd563444
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Sep 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
UX All about "how to get things done"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants