Skip to content

qt: Prevent the main window popup menu#219

Merged
maflcko merged 1 commit intobitcoin-core:masterfrom
hebasto:210223-toolbar
Feb 25, 2021
Merged

qt: Prevent the main window popup menu#219
maflcko merged 1 commit intobitcoin-core:masterfrom
hebasto:210223-toolbar

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Feb 23, 2021

bitcoin/bitcoin#11168 is not fixed by bitcoin/bitcoin#11169 completely, as users are allowed to right click on the menu bar:

Screenshot from 2021-02-23 14-18-24

This PR moves the context menu prohibition from QToolBar instance to its parent BitcoinGUI instance, which is derived from QMainWindow.

@hebasto
qt: Prevent the main window popup menu
ca5bd1c 
By default, a popup menu contains checkable entries for the toolbars
and dock widgets present in the main window. This allows users to
accidentally hide the toolbar.
@hebasto hebasto mentioned this pull request Feb 23, 2021
Merged
jarolrod
jarolrod reviewed Feb 23, 2021
View reviewed changes
Copy link
Contributor

@jarolrod jarolrod left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK ca5bd1c, tested on Ubuntu 20.04 Qt 5.12. Confirming that I can replicate the behavior described on master and this PR fixes it.

The question is: "Is there a use case for disabling tabs?"
I would lean towards no. To me, it doesn't make sense to allow a user to toggle the visibility of the tabs.

leonardojobim
leonardojobim approved these changes Feb 23, 2021
View reviewed changes
Copy link

@leonardojobim leonardojobim left a comment

Choose a reason for hiding this comment

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

tACK ca5bd1c

Tested on Ubuntu 20.04 Qt 5.12.8.
I also confirm that context menu is displayed on master branch but it does not happen on the branch of this PR.

@luke-jr
Copy link
Member

luke-jr commented Feb 24, 2021

Concept ACK, but I don't understand why the toolbar inherits this prohibition and other widgets (where we want a context menu) don't...

@hebasto
Copy link
Member Author

hebasto commented Feb 24, 2021

I don't understand why the toolbar inherits this prohibition and other widgets (where we want a context menu) don't...

Probably, due to the fact that QMainWindow has its own layout which a QMenuBar belongs to.

@maflcko maflcko merged commit 434065a into bitcoin-core:master Feb 25, 2021
@hebasto hebasto deleted the 210223-toolbar branch February 25, 2021 08:03
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 25, 2021
Merge bitcoin-core/gui#219: qt: Prevent the main window popup menu
d3898b0 
ca5bd1c qt: Prevent the main window popup menu (Hennadii Stepanov)

Pull request description:

  bitcoin#11168 is not fixed by bitcoin#11169 completely, as users are allowed to right click on the menu bar:

  ![Screenshot from 2021-02-23 14-18-24](https://user-images.githubusercontent.com/32963518/108842753-699eb700-75e2-11eb-92ec-3aff9aa80bd4.png)

  This PR moves the context menu prohibition from `QToolBar` instance to its parent `BitcoinGUI` instance, which is derived from `QMainWindow`.

ACKs for top commit:
  jarolrod:
    ACK ca5bd1c, tested on Ubuntu 20.04 Qt 5.12. Confirming that I can replicate the behavior described on `master` and this `PR` fixes it.
  leonardojobim:
    tACK bitcoin-core/gui@ca5bd1c

Tree-SHA512: a654ecf7ee35bb271df039be77077c1e1f9514e332587ba8622cea18da6a5b3ae8a7eb421e404ec5993c31a2f4d028e0e456fcc01facdbf61a2bc3b1e8423982
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 28, 2022
@apoelstra
Merge 434065a into merged_master (Bitcoin PR bitcoin-core/gui#219)
d2d2109
@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

2 more reviewers

@jarolrod jarolrod jarolrod left review comments

@leonardojobim leonardojobim leonardojobim approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hebasto @luke-jr @leonardojobim @jarolrod @maflcko