Skip to content

gui: Disable unavailable context menu items in transactions tab #17956

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
Merged
merged 1 commit into from
May 29, 2020
Merged

gui: Disable unavailable context menu items in transactions tab #17956

merged 1 commit into from
May 29, 2020

Conversation

kristapsk
Copy link
Contributor

Fixes #9192.

@fanquake fanquake added the GUI label Jan 18, 2020
@hebasto
Copy link
Member

hebasto commented Jan 18, 2020

@kristapsk screenshots "before" and "after" are welcome for GUI-related PRs ;)

@kristapsk
Copy link
Contributor Author

@hebasto Functionality seemed so simple and obvious (just menu items being disabled) that there isn't need for screenshots. But could add. All four pairs of possible before and after screenshots?

hebasto
hebasto reviewed Jan 18, 2020
View reviewed changes
src/qt/transactionview.cpp
@@ -395,6 +396,8 @@ void TransactionView::contextualMenu(const QPoint &point)
hash.SetHex(selection.at(0).data(TransactionTableModel::TxHashRole).toString().toStdString());
abandonAction->setEnabled(model->wallet().transactionCanBeAbandoned(hash));
bumpFeeAction->setEnabled(model->wallet().transactionCanBeBumped(hash));
copyAddressAction->setEnabled(GUIUtil::hasEntryData(transactionView, 0, TransactionTableModel::AddressRole));
Copy link
Member

Choose a reason for hiding this comment

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

How to test this line of the code?

Copy link
Contributor Author

@kristapsk kristapsk Jan 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I tested with some JoinMarket transactions, where I had partially imported JM wallet addresses as watchonly (some of tx inputs belonged to the wallet, but no output addresses).

@jonasschnelli
Copy link
Contributor

Nice!
Concept ACK.

promag
promag reviewed Jan 26, 2020
View reviewed changes
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.

@kristapsk
Copy link
Contributor Author

Looks like I fucked this up. Any hints how to undo this git merge mess?

@kristapsk
Copy link
Contributor Author

Ok, figured out myself, addressed comments by @promag.

nopara73
nopara73 approved these changes Feb 8, 2020
View reviewed changes
Copy link

@nopara73 nopara73 left a comment

Choose a reason for hiding this comment

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

Untested concept ACK, my original suggestion was the removal of non-functional context menu entries, but disabling is better, as it's used already:

image

@DrahtBot DrahtBot mentioned this pull request Mar 3, 2020
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 3, 2020 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@promag
Copy link
Contributor

promag commented Mar 3, 2020

Concept ACK.

@laanwj laanwj added the Bug label Mar 27, 2020
This was referenced Apr 13, 2020
Closed
Closed
jonatack
jonatack reviewed Apr 16, 2020
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Thanks, good to see user-facing improvements like this.

Tested ACK f780d5a modulo docstring fix.

@jonatack
Copy link
Member

Re-ACK 2b18fd2

git diff f780d5a 2b18fd2 shows the only change since the previous review is the bool docstring fixup

-/** Check whether a field of the currently selected entry of a view is not empty. Does nothing if
-    nothing is selected.
+/** Returns true if the specified field of the currently selected view entry is not empty.

@kristapsk
Copy link
Contributor Author

Is there anything anyone expects me to do more here or this is just forgotten in a long list of pending pull requests?

@fanquake fanquake requested review from jonasschnelli and promag May 15, 2020 23:48
@jonatack
Copy link
Member

@kristapsk the latter, I think. I agree this would be good, for now it needs a couple more reviewers' eyes on it.

@hebasto
Copy link
Member

hebasto commented May 16, 2020

Concept ACK.

May I suggest to:

  • split out refactor-only changes into a separate commit
  • use standard prefixes in commit messages, e.g., refactor: ... and qt: ...
  • apply clang-format-diff.py for each commit

?

@jonasschnelli
Copy link
Contributor

codereview utACK 2b18fd2

@jonasschnelli jonasschnelli merged commit 0d5c182 into bitcoin:master May 29, 2020
@kristapsk kristapsk deleted the tx-menu-disable branch May 29, 2020 08:59
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 31, 2020
@jonasschnelli
Merge bitcoin#17956: gui: Disable unavailable context menu items in t…
cf64aec 
…ransactions tab

2b18fd2 Disable unavailable context menu items in transactions tab (Kristaps Kaupe)

Pull request description:

  Fixes bitcoin#9192.

ACKs for top commit:
  jonatack:
    Re-ACK 2b18fd2
  jonasschnelli:
    codereview utACK 2b18fd2

Tree-SHA512: 4ea19c7b5976f1f0b1baecccb3077cf82f078af7257f92162686bcce2188efe49511a5f557853bc5fe0b10616708957d61c006944babbe60b8105e78751e865f
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Aug 24, 2021
@kristapsk @PiRK
gui: Disable unavailable context menu items in transactions tab
624c94d 
Summary:
Disable "Copy label" and "Copy address" context menu actions when no label or address is available for a transaction.

This is a backport of [[bitcoin/bitcoin#17956 | core#17956]]

Test Plan:
`ninja && src/qt/bitcoin-qt`

Right click on a transaction with no label.

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9925
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hebasto hebasto hebasto left review comments

@jonasschnelli jonasschnelli Awaiting requested review from jonasschnelli

@promag promag Awaiting requested review from promag

+2 more reviewers

@jonatack jonatack jonatack left review comments

@nopara73 nopara73 nopara73 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Bug GUI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Context menu options that are not available should be disabled (in transactions tab)
9 participants
@kristapsk @hebasto @jonasschnelli @DrahtBot @promag @jonatack @nopara73 @laanwj @fanquake