qt: Cleanup MacDockIconHandler class #14597

hebasto · 2018-10-28T22:44:34Z

The Objective-C MacDockIconHandler class currently does such jobs on macOS:

Set the Dock icon
Support the Dock icon menu
Handle the Dock icon click event

Qt does the first job natively.
Qt has native support the Dock icon menu only since version 5.2
Qt does not handle applicationShouldHandleReopen event. And this brakes all Qt support for the Dock icon click event.

This PR:

removes Objective-C code for the Dock icon setting. Qt setWindowIcon() does this work
removes Objective-C code for the Dock icon menu. Qt setAsDockMenu() does this work
uses Qt signal for the Dock icon click event

DrahtBot · 2018-10-28T23:41:45Z

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

Conflicts

No conflicts as of last run.

promag · 2018-10-29T10:01:16Z

Concept ACK.

practicalswift · 2018-10-29T13:36:19Z

Concept ACK

Net change: -189 lines. Nice cleanup!

fanquake · 2018-10-30T11:00:48Z

Concept ACK

There's at least one bug here. Using e0d1883, if you close the main window using the red cross, you can no longer bring the window back by clicking on the dock icon.
This might be solved by one of the other GUI related PRs, possibly #14123.

promag · 2018-10-30T11:20:09Z

This might be solved by one of the other GUI related PRs, possibly #14123.

@fanquake no it doesn't solve it. But using a dock menu action, like "Send", it reopens.

Qt `setWindowIcon()` does this work.

hebasto · 2018-10-31T20:30:11Z

@fanquake @promag @practicalswift
Thank you for your reviews.
The original goal of this PR (to completely remove MacDockIconHandler class) has revealed inaccessible.
So PR has be reworked. Would you mind re-reviewing?

fanquake · 2018-11-02T07:30:00Z

src/qt/macdockiconhandler.mm

@@ -49,61 +44,22 @@ void setupDockClickHandler() {
    setupDockClickHandler();
    this->m_dummyWidget = new QWidget();
    this->m_dockMenu = new QMenu(this->m_dummyWidget);
-    this->setMainWindow(nullptr);
 #if QT_VERSION >= 0x050200


You can drop the QT_VERSION check here, as our next release will require at least 5.2. master currently requires 5.4 IIRC.

hebasto · 2018-11-02T09:51:47Z

@fanquake Your comment is addressed. Please re-review.

Sjors

tACK efb681d on macOS 10.14.1, thanks for reducing the amount of OS specific code we need to reason about!

Sjors · 2018-11-02T17:00:37Z

src/qt/macdockiconhandler.mm

-            class_replaceMethod(delClass, shouldHandle, (IMP)dockClickHandler, "B@:");
-        else
-            class_addMethod(delClass, shouldHandle, (IMP)dockClickHandler,"B@:");
+        class_replaceMethod(delClass, shouldHandle, (IMP)dockClickHandler, "B@:");


Why did/do we need this class_replaceMethod stuff?

This implements the method of the application delegate which handles the reopen application event (i.e., the user clicks application icon in the Dock).
Ref: How Cocoa Applications Handle Apple Events

Qt cannot handle the reopen application event for now.

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.

hebasto · 2018-11-04T00:52:42Z

Fixed bug with hidden/inactive/obscured main window: toggleHidden() does not work on macOS properly.
cc: @promag

jonasschnelli · 2018-11-04T14:01:59Z

This needs proper testing on macOS 10.9 up till 10.14. Also, I'm not sure that this works reliable with older Qt5 versions. We should probably also test against Qt5.4 till Qt5.9.

hebasto · 2018-11-04T14:30:19Z

@jonasschnelli

This needs proper testing on macOS 10.9 up till 10.14. Also, I'm not sure that this works reliable with older Qt5 versions. We should probably also test against Qt5.4 till Qt5.9.

https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes.md:

From 0.17.0 onwards macOS <10.10 is no longer supported. 0.17.0 is built using Qt 5.9.x, which doesn't support versions of macOS older than 10.10.

What is the reason to choose so wide range of the tested platforms?

jonasschnelli · 2018-11-07T02:20:01Z

This needs proper testing on macOS 10.9 up till 10.14. Also, I'm not sure that this works reliable with older Qt5 versions. We should probably also test against Qt5.4 till Qt5.9.
https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes.md:

The "official" binaries are using Qt5.9 which doesn't mean that we drop support self-compilation with an older Qt version.

We should at least do some tests on something between OSX 10.10 and 10.14 with Qt5.5 and Qt5.9 (and not just only 10.14&Qt5.9).

Sjors · 2018-11-10T09:23:53Z

I'm not a fan of testing older macOS versions, at least not every PR. There's very few core devs that work on both macOS and the GUI (very on the GUI period) and have old versions of macOS lying around. Meanwhile Apple hasn't increased hardware requirements for their recent OS updates and they make it really easy.

I think this leads to a bit of vicious circle where work on the GUI is very slow, leading to very few people using it, leading to very few devs working on it. I think it would be better for users if each major release only supports the current macOS version. If there's a killer feature that we really want people on older macOS versions to have, we could always backport that.

Qt `setWindowIcon()` does this work. Github-Pull: bitcoin#14597 Rebased-From: 53bb6be

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. Github-Pull: bitcoin#14597 Rebased-From: 2464925

Qt `setAsDockMenu()` does this work. Github-Pull: bitcoin#14597 Rebased-From: 6b1d297

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

fanquake added GUI macOS labels Oct 28, 2018

This was referenced Oct 29, 2018

gui: Add Windows taskbar progress #14137

Closed

gui: Add GUIUtil::bringToFront #14123

Merged

Qt: Remove unnecessary image buffer for Mac dock icon #13561

Closed

macOS: disable AppNap during sync #12783

Merged

bitcoin deleted a comment from ismail120572 Oct 30, 2018

hebasto changed the title ~~qt: Remove old MacDockIconHandler class~~ [WIP] qt: Remove old MacDockIconHandler class Oct 30, 2018

Remove obj_c for macOS Dock icon setting

53bb6be

Qt `setWindowIcon()` does this work.

hebasto force-pushed the 20181028-macos-dock-overhaul branch from e0d1883 to b913e68 Compare October 31, 2018 19:38

hebasto changed the title ~~[WIP] qt: Remove old MacDockIconHandler class~~ [WIP] qt: Cleanup MacDockIconHandler class Oct 31, 2018

hebasto force-pushed the 20181028-macos-dock-overhaul branch from b913e68 to 43627b9 Compare October 31, 2018 20:05

hebasto changed the title ~~[WIP] qt: Cleanup MacDockIconHandler class~~ qt: Cleanup MacDockIconHandler class Oct 31, 2018

fanquake reviewed Nov 2, 2018

View reviewed changes

hebasto force-pushed the 20181028-macos-dock-overhaul branch from 461806f to 14e7e16 Compare November 2, 2018 09:14

hebasto changed the title ~~qt: Cleanup MacDockIconHandler class~~ [WIP] qt: Cleanup MacDockIconHandler class Nov 2, 2018

hebasto force-pushed the 20181028-macos-dock-overhaul branch from 14e7e16 to efb681d Compare November 2, 2018 09:32

hebasto changed the title ~~[WIP] qt: Cleanup MacDockIconHandler class~~ qt: Cleanup MacDockIconHandler class Nov 2, 2018

Sjors approved these changes Nov 2, 2018

View reviewed changes

hebasto added 2 commits November 4, 2018 02:37

Remove obj_c for macOS Dock icon menu

6b1d297

Qt `setAsDockMenu()` does this work.

hebasto force-pushed the 20181028-macos-dock-overhaul branch from efb681d to 6b1d297 Compare November 4, 2018 00:47

bitcoin deleted a comment from ismail120572 Nov 4, 2018

laanwj merged commit 6b1d297 into bitcoin:master Nov 12, 2018

hebasto deleted the 20181028-macos-dock-overhaul branch November 12, 2018 21:44

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

Remove obj_c for macOS Dock icon setting

4d4bc37

Qt `setWindowIcon()` does this work. Github-Pull: bitcoin#14597 Rebased-From: 53bb6be

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

Remove obj_c for macOS Dock icon menu

0c2fb87

Qt `setAsDockMenu()` does this work. Github-Pull: bitcoin#14597 Rebased-From: 6b1d297

promag mentioned this pull request Dec 30, 2018

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

Merged

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

qt: Cleanup MacDockIconHandler class #14597

qt: Cleanup MacDockIconHandler class #14597

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

promag commented Oct 29, 2018

Uh oh!

practicalswift commented Oct 29, 2018

Uh oh!

fanquake commented Oct 30, 2018

Uh oh!

promag commented Oct 30, 2018

Uh oh!

hebasto commented Oct 31, 2018

Uh oh!

fanquake Nov 2, 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!

hebasto commented Nov 2, 2018

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Nov 2, 2018

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 2, 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!

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

Uh oh!

Uh oh!

jonasschnelli commented Nov 4, 2018

Uh oh!

hebasto commented Nov 4, 2018

Uh oh!

jonasschnelli commented Nov 7, 2018

Uh oh!

Sjors commented Nov 10, 2018

Uh oh!

Uh oh!

hebasto commented Oct 28, 2018 •

edited

Loading

DrahtBot commented Oct 28, 2018 •

edited

Loading

fanquake Nov 2, 2018 •

edited

Loading

hebasto Nov 2, 2018 •

edited

Loading

hebasto commented Nov 4, 2018 •

edited

Loading