gui: Add GUIUtil::bringToFront #14123

promag · 2018-08-31T22:39:29Z

The sequence show -> raise -> activateWindow is factored out to the new function GUIUtil::bringToFront where a macOS fix is added in order to fix #13829.

Also included:

simplification of BitcoinGUI::showNormalIfMinimized
simplified some connections to BitcoinGUI::showNormalIfMinimized
added missing connections to BitcoinGUI::showNormalIfMinimized.

promag · 2018-08-31T22:41:17Z

@jonasschnelli @Sjors @ken2812221 review/test much appreciated.

DrahtBot · 2018-08-31T22:45:36Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14594 (qt:Fix minimized window bug on Linux by hebasto)
#11625 (Add BitcoinApplication & RPCConsole tests by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/qt/bitcoingui.cpp

laanwj · 2018-09-01T09:41:18Z

I think it would make sense, for ease of review, to split off the fix to #13829 from pure refactoring.

promag · 2018-09-01T10:02:42Z

Which refactor?

Note that the issue is observed in more places than the one reported in #13829.

Sjors

tACK eed869b on macOS, this indeed fixes #13829.

The change from static_cast<void (BitcoinGUI::*)()> to [this] is most eye pleasing :-)

I think @laanwj is referring to e.g. the new bringToFront() function which contains both existing and new code. I didn't find it too confusing though.

Sjors · 2018-09-02T07:38:14Z

src/qt/guiutil.cpp

Apple documentation suggests that this normally shouldn't be needed, which suggests there's a deeper problem somewhere.

I'd also like to know a bit more about this, and the new <objc/objc-runtime.h> #include.

@promag Can you comment here on why we need to manually do this? For reference Apples docs state:

You rarely need to invoke this method. Under most circumstances, AppKit takes care of proper activation. However, you might find this method useful if you implement your own methods for inter-app communication.

so I'd have thought this would be something handled by Qt. There are a couple mentions of this specific flag in the cocoa integration plugin, as well as use in the QCocoaWindow::raise() method.

Added comment.

According to the sel_getUid documentation:

The implementation of this method is identical to the implementation of sel_registerName.

On master(7504157):

sel_registerName used 4 times

sel_getUid not used at all

Could sel_getUid replace with sel_registerName?

fanquake · 2018-09-02T08:14:56Z

Testing current master on macOS 10.13.6 2070a54.
If Command + H or the menu item is used to hide the window:

Clicking the task bar icon shows the window.
Clicking show/hide does not work.
Clicking show (bottom of the menu ) does work.
If Send or Receive is clicked, the window changes to the correct tab, but the window is not shown.
Sign, Verify & Debug all unhide and show the correct dialog.

Testing eed869b.
If Command + H or the menu item is used to hide the window:

Clicking the task bar icon shows the window.
Clicking show/hide does work.
Click show (bottom of the menu ) does work.
If Send or Receive is clicked, the window changes to the correct tab, and unhides.
Sign, Verify & Debug all unhide and show the correct dialog. (this has also fixed the annoying "attention" bounce behaviour that was present before.)
When the window has been minimised (CMD + M, which doesn't actually work at the moment..), only the sign, verify and debug items work, and they only display the sub window, the main window remains minimised. This seems to be a regression, as the options work with current master.

promag · 2018-09-02T09:41:41Z

Thanks for the reviews! I'll fix it.

BTW, in macOS "show/hide" action could be removed since it's already there natively.

ken2812221 · 2018-09-04T03:48:58Z

src/qt/bitcoingui.cpp

I think this is better

connect(signMessageAction, &QAction::triggered, [this]{ showNormalIfMinimized(); gotoSignMessageTab(); }); connect(verifyMessageAction, &QAction::triggered, [this]{ showNormalIfMinimized(); gotoSignMessageTab(); });

Keeping multiple connections for now for consistency with other actions.

I assume the other actions are only done that way for historical reasons -- prior to Qt5/C++11 this was not possible, and we only switched to using lambdas instead of the older syntax in 0.17 I believe.

ken2812221 · 2018-09-04T03:56:40Z

src/qt/bitcoingui.cpp

If the window was minimized, it might need to call showNormal.

This should be fixed.

promag · 2018-09-04T14:02:07Z

Rebased (so that testing takes #14133) and added a fixup that s/show/showNormal after @ken2812221 #14123 (comment).

fanquake · 2018-09-05T01:23:16Z

src/qt/guiutil.cpp

Can you move this comment back done to line 710, given it's in reference to findStartupItemInList.

Sure, done.

hebasto · 2018-09-15T05:28:23Z

src/qt/guiutil.cpp

According to Qt docs

Note that the window must be visible, otherwise activateWindow() has no effect.

Therefore, the safer sequence may be:

w->showNormal(); // or w->show(); w->activateWindow(); w->raise();

src/qt/guiutil.cpp

hebasto · 2018-09-15T05:50:26Z

IMO show()/showNormal() should not be aggregated with activateWindow()->raise() sequence in a single function. Rationale: show() is reverse to hide(), but showNormal() restores the widget after it has been maximized or minimized.

jonasschnelli · 2018-10-17T18:54:56Z

@promag: can you comment or address @hebasto's comments; rebase or close?

ken2812221

ACK d3becb175270d715daaffb75a037433a126a95f4

hebasto · 2018-11-03T21:46:38Z

Tested d3becb175270d715daaffb75a037433a126a95f4:

on Windows 10: bug Qt: Fix restoration of minimized to tray window #14222 fixed
on macOS 10.13.6: bug Unhide QT window doesn't work when hidden with ⌘ + H #13829 fixed
There is a new bug on macOS: obscure main window with another app window, click Dock icon; main windows appears and hides. ~~Investigating...~~ Fixed: qt: Cleanup MacDockIconHandler class #14597 (comment) Would you mind rebasing one more time?
on Linux Mint 19
@promag is it worth to combine a Linux-specific bugfix Minimized window bug on Linux #14591 into this PR?

Also a tiny nit left: #14123 (comment)

This moves the Dock icon click reaction code to the common place and allows some cleanup in obj_c code. According to the Apple's docs `class_replaceMethod` behaves as `class_addMethod`, if the method identified by name does not yet exist; or as `method_setImplementation`, if it does exist.

Qt `setAsDockMenu()` does this work.

promag · 2018-11-05T11:25:22Z

Rebased to account for the new fix and applied suggestion #14123 (comment).

@promag is it worth to combine a Linux-specific bugfix #14591 into this PR?

I think it is worth a new PR.

Sjors · 2018-11-10T09:11:45Z

re-tACK 0a656f8 on macOS 10.14.1 QT 5.11.2 (I didn't check the commits inherited from #14597)

Also +1 on doing Linux in another PR, it's already quite difficult to get any macOS changes through review.

hebasto · 2018-11-10T20:57:53Z

re-tACK

Systems:

Linux Mint 19 + Qt 5.9.5
macOS 10.13.6 + Qt 5.11.2

Commits: 5796671, 6fc21ac, 0a656f8

0a656f8 qt: All tray menu actions call showNormalIfMinimized (João Barbosa) 6fc21ac qt: Use GUIUtil::bringToFront where possible (João Barbosa) 5796671 qt: Add GUIUtil::bringToFront (João Barbosa) 6b1d297 Remove obj_c for macOS Dock icon menu (Hennadii Stepanov) 2464925 Use Qt signal for macOS Dock icon click event (Hennadii Stepanov) 53bb6be Remove obj_c for macOS Dock icon setting (Hennadii Stepanov) Pull request description: The sequence `show -> raise -> activateWindow` is factored out to the new function `GUIUtil::bringToFront` where a macOS fix is added in order to fix #13829. Also included: - simplification of `BitcoinGUI::showNormalIfMinimized` - simplified some connections to `BitcoinGUI::showNormalIfMinimized` - added missing connections to `BitcoinGUI::showNormalIfMinimized`. Tree-SHA512: a8e301aebc359aa353821e2af352ae356f44555724921b01da907e128653ef9dc55d8764a1bff72a579e5ff96df8a681f6804bfe83acba441da92fedff974a55

Github-Pull: bitcoin#14123 Rebased-From: 5796671

Github-Pull: bitcoin#14123 Rebased-From: 6fc21ac

Github-Pull: bitcoin#14123 Rebased-From: 0a656f8

27beb83 qt: All tray menu actions call showNormalIfMinimized (João Barbosa) c470bbd qt: Use GUIUtil::bringToFront where possible (João Barbosa) ac73c7d qt: Add GUIUtil::bringToFront (João Barbosa) 0c2fb87 Remove obj_c for macOS Dock icon menu (Hennadii Stepanov) 9034714 Use Qt signal for macOS Dock icon click event (Hennadii Stepanov) 4d4bc37 Remove obj_c for macOS Dock icon setting (Hennadii Stepanov) d2ed162 Clean systray icon menu for -disablewallet mode (Hennadii Stepanov) 298dc15 gui: Favor macOS show / hide action in dock menu (João Barbosa) Pull request description: Backport #14123 #14133 #14383 and #14597 to 0.17 branch to fix #13606 (comment). Tree-SHA512: 543c80e7e2130870e801e0c9a69b06b9eea27c288478fc5dddeb662f7f3ec5b56b30916e5a9a629fced3fffcb8be77e2cd155e75cfd0a4392299add9730840f4

a60b7cc qt: Replace objc_msgSend with native syntax (furszy) 5d6f9b5 qt: Use GUIUtil::bringToFront where possible (furszy) 9e85183 qt: Add GUIUtil::bringToFront (furszy) 188199a Remove obj_c for macOS Dock icon menu (furszy) 75a41e2 Use Qt signal for macOS Dock icon click event (furszy) ff319ad Remove obj_c for macOS Dock icon setting. (furszy) Pull request description: Updating `MacDockIconHandler` to latest upstream. Fixing #1512 . Back ported PRs: #### gui: Add GUIUtil::bringToFront [14123](bitcoin#14123): ``` The sequence show -> raise -> activateWindow is factored out to the new function GUIUtil::bringToFront where a macOS fix is added in order to fix bitcoin#13829. Also included: simplification of BitcoinGUI::showNormalIfMinimized ``` #### qt: Replace objc_msgSend() function calls with the native Objective-C syntax [16720](bitcoin#16720): ``` Changes in Xcode 11 Objective-C Runtime cause an error (bitcoin#16387) during building on MacOS 10.15 Catalina. This PR fixes this issue by replacing objc_msgSend() function calls with the native Objective-C syntax. Refs: changes in objc_msgSend function OBJC_OLD_DISPATCH_PROTOTYPES macro ``` ACKs for top commit: random-zebra: utACK a60b7cc Fuzzbawls: ACK a60b7cc Tree-SHA512: 95543a12d58187f17d27c6757351becd43a92c3edb8fcbc0a2b03a54d31cbad604ae149f2e866d82c3ce5f9a636571b8e51137fc5f639031a20080d8290889d9

* Remove obj_c for macOS Dock icon setting Qt `setWindowIcon()` does this work. * Use Qt signal for macOS Dock icon click event This moves the Dock icon click reaction code to the common place and allows some cleanup in obj_c code. According to the Apple's docs `class_replaceMethod` behaves as `class_addMethod`, if the method identified by name does not yet exist; or as `method_setImplementation`, if it does exist. * Remove obj_c for macOS Dock icon menu Qt `setAsDockMenu()` does this work. * qt: Add GUIUtil::bringToFront * qt: Use GUIUtil::bringToFront where possible * qt: All tray menu actions call showNormalIfMinimized * qt: Replace objc_msgSend with native syntax Co-authored-by: Hennadii Stepanov <[email protected]> Co-authored-by: João Barbosa <[email protected]>

a60b7cceb4d142239207ac2be2244aa999bdc1fd qt: Replace objc_msgSend with native syntax (furszy) 5d6f9b5c4c2288e1dd150c4e2722db2db2f5db5a qt: Use GUIUtil::bringToFront where possible (furszy) 9e851839737618f8215ec69768702ca6328ab9d9 qt: Add GUIUtil::bringToFront (furszy) 188199a77653bd548d293c5222e1a05773947918 Remove obj_c for macOS Dock icon menu (furszy) 75a41e2d3d7223da8381c9ce19e9454ee96a60b9 Use Qt signal for macOS Dock icon click event (furszy) ff319ad0e3d3319fb9a98087ac73686c8b6f4e17 Remove obj_c for macOS Dock icon setting. (furszy) Pull request description: Updating `MacDockIconHandler` to latest upstream. Fixing #1512 . Back ported PRs: #### gui: Add GUIUtil::bringToFront [14123](bitcoin/bitcoin#14123): ``` The sequence show -> raise -> activateWindow is factored out to the new function GUIUtil::bringToFront where a macOS fix is added in order to fix #13829. Also included: simplification of BitcoinGUI::showNormalIfMinimized ``` #### qt: Replace objc_msgSend() function calls with the native Objective-C syntax [16720](bitcoin/bitcoin#16720): ``` Changes in Xcode 11 Objective-C Runtime cause an error (#16387) during building on MacOS 10.15 Catalina. This PR fixes this issue by replacing objc_msgSend() function calls with the native Objective-C syntax. Refs: changes in objc_msgSend function OBJC_OLD_DISPATCH_PROTOTYPES macro ``` ACKs for top commit: random-zebra: utACK a60b7cceb4d142239207ac2be2244aa999bdc1fd Fuzzbawls: ACK a60b7cceb4d142239207ac2be2244aa999bdc1fd Tree-SHA512: 95543a12d58187f17d27c6757351becd43a92c3edb8fcbc0a2b03a54d31cbad604ae149f2e866d82c3ce5f9a636571b8e51137fc5f639031a20080d8290889d9 # Conflicts: # configure.ac # src/qt/macdockiconhandler.mm

* Remove obj_c for macOS Dock icon setting Qt `setWindowIcon()` does this work. * Use Qt signal for macOS Dock icon click event This moves the Dock icon click reaction code to the common place and allows some cleanup in obj_c code. According to the Apple's docs `class_replaceMethod` behaves as `class_addMethod`, if the method identified by name does not yet exist; or as `method_setImplementation`, if it does exist. * Remove obj_c for macOS Dock icon menu Qt `setAsDockMenu()` does this work. * qt: Add GUIUtil::bringToFront * qt: Use GUIUtil::bringToFront where possible * qt: All tray menu actions call showNormalIfMinimized * qt: Replace objc_msgSend with native syntax Co-authored-by: Hennadii Stepanov <[email protected]> Co-authored-by: João Barbosa <[email protected]>

fanquake added the GUI label Aug 31, 2018

This was referenced Sep 1, 2018

utils: Make fs::path::string() always return utf-8 string on Windows #13877

Merged

Test for Windows encoding issue #13787

Merged

ken2812221 reviewed Sep 1, 2018

View reviewed changes

src/qt/bitcoingui.cpp Outdated Show resolved Hide resolved

promag force-pushed the 2018-08-bringtofront branch from f2cd327 to eed869b Compare September 1, 2018 07:59

Sjors approved these changes Sep 2, 2018

View reviewed changes

fanquake mentioned this pull request Sep 2, 2018

gui: Favor macOS show / hide action in dock menu #14133

Merged

ken2812221 reviewed Sep 4, 2018

View reviewed changes

promag force-pushed the 2018-08-bringtofront branch from eed869b to 242e61f Compare September 4, 2018 14:01

maflcko added Refactoring and removed Refactoring labels Sep 4, 2018

fanquake reviewed Sep 5, 2018

View reviewed changes

promag force-pushed the 2018-08-bringtofront branch from 242e61f to d5284ed Compare September 10, 2018 12:44

DrahtBot mentioned this pull request Sep 15, 2018

utils: Add fstream wrapper to allow to pass unicode filename on Windows #13878

Merged

fanquake mentioned this pull request Sep 15, 2018

Qt: Fix restoration of minimized to tray window #14222

Closed

hebasto reviewed Sep 15, 2018

View reviewed changes

src/qt/guiutil.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Sep 21, 2018

Add BitcoinApplication & RPCConsole tests #11625

Merged

DrahtBot added the Needs rebase label Sep 25, 2018

promag force-pushed the 2018-08-bringtofront branch 2 times, most recently from 1aa064a to 56c61a3 Compare October 26, 2018 13:18

ken2812221 approved these changes Nov 3, 2018

View reviewed changes

hebasto and others added 5 commits November 4, 2018 02:37

Remove obj_c for macOS Dock icon menu

6b1d297

Qt `setAsDockMenu()` does this work.

qt: Add GUIUtil::bringToFront

5796671

qt: Use GUIUtil::bringToFront where possible

6fc21ac

qt: All tray menu actions call showNormalIfMinimized

0a656f8

promag force-pushed the 2018-08-bringtofront branch from d3becb1 to 0a656f8 Compare November 5, 2018 11:22

laanwj merged commit 0a656f8 into bitcoin:master Nov 12, 2018

hebasto mentioned this pull request Dec 25, 2018

Cannot restore bitcoin-qt window if minimized or closed to systray #13606

Closed

promag added a commit to promag/bitcoin that referenced this pull request Dec 30, 2018

qt: Add GUIUtil::bringToFront

ac73c7d

Github-Pull: bitcoin#14123 Rebased-From: 5796671

promag added a commit to promag/bitcoin that referenced this pull request Dec 30, 2018

qt: Use GUIUtil::bringToFront where possible

c470bbd

Github-Pull: bitcoin#14123 Rebased-From: 6fc21ac

promag added a commit to promag/bitcoin that referenced this pull request Dec 30, 2018

qt: All tray menu actions call showNormalIfMinimized

27beb83

Github-Pull: bitcoin#14123 Rebased-From: 0a656f8

promag mentioned this pull request Dec 30, 2018

0.17: GUI Backports #14123 #14133 #14383 #14597 #15065

Merged

furszy mentioned this pull request Apr 10, 2020

[GUI] Back port latest MacOS dock icon handler. PIVX-Project/PIVX#1515

Merged

xdustinface mentioned this pull request Apr 27, 2020

Backport bitcoin#14123 and bitcoin#16720 dashpay/dash#3463

Merged

CaveSpectre11 mentioned this pull request Jul 5, 2020

Address segmentation faults in the Log message handlers on macOS. Veil-Project/veil#766

Merged

hebasto mentioned this pull request Aug 25, 2021

refactor: Make paths to update Encryption and HD wallet statuses simpler bitcoin-core/gui#403

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

gui: Add GUIUtil::bringToFront #14123

gui: Add GUIUtil::bringToFront #14123

Uh oh!

Conversation

promag commented Aug 31, 2018

Uh oh!

promag commented Aug 31, 2018

Uh oh!

DrahtBot commented Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

laanwj commented Sep 1, 2018

Uh oh!

promag commented Sep 1, 2018

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented Sep 2, 2018

Uh oh!

promag commented Sep 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Sep 15, 2018

Uh oh!

jonasschnelli commented Oct 17, 2018

Uh oh!

ken2812221 left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Nov 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 5, 2018

Uh oh!

Sjors commented Nov 10, 2018

Uh oh!

hebasto commented Nov 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

DrahtBot commented Aug 31, 2018 •

edited

Loading

promag Sep 5, 2018 •

edited

Loading

promag commented Sep 4, 2018 •

edited

Loading

hebasto commented Nov 3, 2018 •

edited

Loading