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

qt: Disable requests context menu actions when appropriate #214

Merged

Conversation

jarolrod
Copy link
Member

The recent requests table will allow you to copy data points even if they do not exist. This PR implements checks to disable the copy label, copy message, and copy amount context menu actions if the respective fields are empty. This brings the recent requests table context menu behavior closer to the behavior seen in the transaction view.

On a payment request entry which does not have a value for label, message, or amount:

Master PR
Screen Shot 2021-02-19 at 1 22 28 AM Screen Shot 2021-02-19 at 1 21 49 AM

copy URI never needs to be disabled as an entry in the recent requests table must have a URI even if it doesn't have a label, message, or amount. #213 will add a copy address context menu action. This also does not need a check as an entry must be associated with an address.

Below are some more examples of how this PR will behave:
Has Label, Message, and Amount
Screen Shot 2021-02-19 at 12 05 38 AM

Has Label and Amount, but no Message
Screen Shot 2021-02-19 at 12 05 58 AM

@jonasschnelli
Copy link
Contributor

utACK 69915b8 - I guess comparing against zero for an "empty" amount is acceptable here.

@jarolrod
Copy link
Member Author

utACK 69915b8 - I guess comparing against zero for an "empty" amount is acceptable here.

Comparing against zero is what's used to determine (no amount requested) in recentrequeststablemodel.cpp Line 84

@Talkless
Copy link

Concept ACK.

@jarolrod jarolrod force-pushed the disable-contextactions-novalue branch from 69915b8 to 336a643 Compare February 21, 2021 21:44
@jarolrod
Copy link
Member Author

updated 69915b8 -> 336a643

Addressed @Talkless suggestions, thanks for the review!

Changes:

  • variables that can be const are made const
  • refactor context menu action disable logic from using setEnabled with negations to using setDisabled without negations

Copy link
Contributor

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

tested ACK 336a643

didn't review code

@hebasto
Copy link
Member

hebasto commented Feb 22, 2021

Concept ACK.

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 336a643, tested on Linux Mint 20.1 (Qt 5.12.8).

@jarolrod jarolrod force-pushed the disable-contextactions-novalue branch from 336a643 to 3f83f05 Compare February 23, 2021 02:16
@jarolrod
Copy link
Member Author

updated from 336a643 -> 3f83f05, thanks @hebasto @MarcoFalke for the review

Changes:

  • Adhere to clang-format
  • req is made to be a const reference as that makes more sense

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 3f83f05, only suggested changes and rebased since my previous review, verified with

$ git range-diff master 336a6435965dc813bdc2bf11398bce564e52c984 3f83f05f52be8f3bac2267d447b5037409cde17d

The recent requests table will allow you to copy data points even if they do not exist.
This PR implements checks to disable the 'copy label', 'copy message', and 'copy amount' context menu action if the respective fields are empty.
@jarolrod jarolrod force-pushed the disable-contextactions-novalue branch from 3f83f05 to bb3da8f Compare February 23, 2021 17:20
@jarolrod
Copy link
Member Author

Updated from 3f83f05 -> bb3da8f

Changes: Resolve conflict with #213

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 bb3da8f

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

@maflcko maflcko merged commit 09bc7bf into bitcoin-core:master Feb 25, 2021
@jarolrod jarolrod deleted the disable-contextactions-novalue branch March 8, 2021 15:39
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 28, 2022
@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