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

Revamp context menus #263

Merged
merged 4 commits into from
Apr 20, 2021
Merged

Revamp context menus #263

merged 4 commits into from
Apr 20, 2021

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Mar 30, 2021

This PR:

  1. removes useless Alt + <KEY> shortcuts from context menu items
  2. replaces 3 lines of code with the only call of QMenu::addAction for each context menu item (it became possible since build: Bump minimum Qt version to 5.9.5 bitcoin/bitcoin#21286 was merged)
  3. makes other minor cleanups

No behavior change.

Such shortcuts are useless as pressing the Alt key closes a context menu
widget immediately.
This overloaded function was introduced in Qt 5.6 and makes code more
concise.
@hebasto
Copy link
Member Author

hebasto commented Apr 14, 2021

Rebased b1ab2d1 -> 16c157d (pr263.01 -> pr263.02) due to the conflict with #260.

Copy link
Contributor

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

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

ACK 16c157d

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.

Code review ACK 16c157d. Nice code cleanup that takes advantage of more recent Qt API.


// Build context menu
contextMenu = new QMenu(this);
contextMenu->addAction(copyAddressAction);
contextMenu->addAction(copyLabelAction);
contextMenu->addAction(editAction);
if(tab == SendingTab)
if (tab == SendingTab) {
QAction* deleteAction = new QAction(ui->deleteAddress->text(), this);
Copy link
Contributor

Choose a reason for hiding this comment

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

1398a65

With this change, the delete action only shows up on the sending tab where before it would be disabled in the receiving tab. I prefer this approach since the context menu is consistent for all addresses in each tab. I would prefer the other approach if the context menu actions were different on each row.


// Connect signals for context menu actions
connect(copyAddressAction, &QAction::triggered, this, &AddressBookPage::on_copyAddress_clicked);
connect(copyLabelAction, &QAction::triggered, this, &AddressBookPage::onCopyLabelAction);
connect(editAction, &QAction::triggered, this, &AddressBookPage::onEditAction);

connect(ui->tableView, &QWidget::customContextMenuRequested, this, &AddressBookPage::contextualMenu);

Copy link
Contributor

Choose a reason for hiding this comment

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

963e120

👀

@maflcko
Copy link
Contributor

maflcko commented Apr 19, 2021

@hebasto Ready for merge?

@hebasto
Copy link
Member Author

hebasto commented Apr 19, 2021

@jarolrod @Sjors Do you mind looking into this PR?

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.

ACK 16c157d

Tested that we maintain the same shortcuts between master and this PR and that the functionality is the same.

The use of this overloaded addAction is correct and fits with our minimum supported Qt version: https://doc.qt.io/archives/qt-5.9/qmenu.html#addAction-4

This is a nice simplification, the use of this overloaded function makes adding new context menu actions easier.

@hebasto hebasto merged commit f385ad7 into bitcoin-core:master Apr 20, 2021
@hebasto hebasto deleted the 210330-context branch April 20, 2021 20:21
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 21, 2021
hebasto added a commit that referenced this pull request Jun 14, 2021
e4c916a Bugfix: GUI: Use a different shortcut for "1 d&ay" banning, due to conflict with "&Disconnect" (Luke Dashjr)
94e7cdd GUI: Add keyboard shortcuts for other context menus (Luke Dashjr)
02b5263 GUI: Restore keyboard shortcuts for context menu entries (Luke Dashjr)

Pull request description:

  Various keyboard shortcuts were lost in #263; this restores them, and also adds new ones for other context menus.

  Note that with a context menu open, simply the shortcut by itself (no Alt) is used.

ACKs for top commit:
  jarolrod:
    Code Review ACK e4c916a
  hebasto:
    ACK e4c916a, tested on Linux Mint 20.1 (Qt 5.12.8).

Tree-SHA512: 949461acf7aac592bc48a1c5abad41b167365830e0cedb3aa11b6a87bd347e16126830ea87936f9c9efc4b7df5b09d3833fae784964d6d119ed45703cfba2ffd
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants