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.

Sign up for GitHub

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

Jump to bottom

Add "Copy address" item to the context menu in the Peers table #264

Closed
wants to merge 1 commit into from
Closed

Add "Copy address" item to the context menu in the Peers table #264

wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Mar 30, 2021 •
edited
Loading

It seems a useful feature. Related to #262.

@hebasto hebasto mentioned this pull request Mar 30, 2021
Open
@ghost
Copy link

ghost commented Mar 30, 2021

Thanks, I applied only the last commit 03a2b74 to the master branch, built it on Debian 10 Buster, and copying of the node address incl. port works. One can afterwards paste it from the clipboard.
I do not understand the code, just applied and tested it.
Screenshot:

bildschirmfoto

jarolrod
jarolrod reviewed Apr 5, 2021
View reviewed changes
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

since we are adding the ability to copy the address, is there any reason to still allow this to show up/be clicked on:

Screen Shot 2021-04-05 at 1 21 42 PM

@hebasto
Copy link
Member Author

hebasto commented Apr 5, 2021

since we are adding the ability to copy the address, is there any reason to still allow this to show up/be clicked on:

Why not?

@hebasto
Copy link
Member Author

hebasto commented Apr 14, 2021

Rebased 03a2b74 -> bb36f6e (pr264.01 -> pr264.02) due to the conflict with #260.

jarolrod
jarolrod reviewed Apr 16, 2021
View reviewed changes
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.

tACK bb36f6e

Tested on macOS 11.2.3 Qt 5.15.2. The menu action works as it should.
Screen Shot 2021-04-15 at 10 53 14 PM

#263, which this is based on, is on my plate to review for tomorrow. 🥃

@promag
Copy link
Contributor

promag commented Apr 17, 2021

If you copy a row with the keyboard shortcut and paste it you will get rubbish.

@hebasto
Copy link
Member Author

hebasto commented Apr 17, 2021

@promag

If you copy a row with the keyboard shortcut and paste it you will get rubbish.

I suppose you are testing on macOS, no?

@hebasto
Copy link
Member Author

hebasto commented Apr 17, 2021

@promag

If you copy a row with the keyboard shortcut and paste it you will get rubbish.

It appears, the problem is much bigger, at least on Linux -- see #283.

Although keyboard shortcuts are out of this PR scope, do you think it'd better to postpone this PR until #283 is fixed?

@promag
Copy link
Contributor

promag commented Apr 23, 2021

Needs rebase.

@maflcko
Copy link
Contributor

maflcko commented Apr 23, 2021

Needs rebase.

why?

@promag
Copy link
Contributor

promag commented Apr 23, 2021

Needs rebase.

why?

Right, it doesn't actually need, just github showing more commits.

@maflcko
Copy link
Contributor

maflcko commented Apr 23, 2021

Ok, I removed commits from showing up

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@hebasto
Copy link
Member Author

hebasto commented Apr 28, 2021

Rebased due to the conflict with #18.

@promag

If you copy a row with the keyboard shortcut and paste it you will get rubbish.

This is fixed in #18.

@hebasto hebasto marked this pull request as draft April 28, 2021 20:17
@hebasto
Copy link
Member Author

hebasto commented Apr 28, 2021

This branch is currently broken, leaving it for now up for grabs.

@hebasto hebasto closed this Apr 28, 2021
@jarolrod jarolrod mentioned this pull request May 5, 2021
Merged
@jarolrod
Copy link
Member

@hebasto no longer up for grabs, picked up in #318

@hebasto hebasto added UX All about "how to get things done" and removed Feature Up for grabs labels May 26, 2021
hebasto added a commit that referenced this pull request Sep 12, 2021
@hebasto
Merge #318: Add Copy address Peers Tab Context Menu Action
ee1db7b 
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
@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.
Reviewers

@jarolrod jarolrod jarolrod left review comments

Assignees
No one assigned
Labels
UX All about "how to get things done"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hebasto @promag @maflcko @DrahtBot @jarolrod