qt: Cleanup MacDockIconHandler class

hebasto commented at 10:44 pm on October 28, 2018: member

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

fanquake added the label GUI on Oct 28, 2018

fanquake added the label macOS on Oct 28, 2018

DrahtBot commented at 11:41 pm on October 28, 2018: member

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

Conflicts

No conflicts as of last run.

promag commented at 10:01 am on October 29, 2018: member

Concept ACK.

practicalswift commented at 1:36 pm on October 29, 2018: contributor

Concept ACK

Net change: -189 lines. Nice cleanup!

fanquake commented at 11:00 am on October 30, 2018: member

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 commented at 11:20 am on October 30, 2018: member

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.

fanquake deleted a comment on Oct 30, 2018

hebasto renamed this:
~~qt: Remove old MacDockIconHandler class~~
[WIP] qt: Remove old MacDockIconHandler class
on Oct 30, 2018

Remove obj_c for macOS Dock icon setting

Qt `setWindowIcon()` does this work.

53bb6be3f8

hebasto force-pushed on Oct 31, 2018

hebasto renamed this:
~~[WIP] qt: Remove old MacDockIconHandler class~~
[WIP] qt: Cleanup MacDockIconHandler class
on Oct 31, 2018

hebasto force-pushed on Oct 31, 2018

hebasto commented at 8:30 pm on October 31, 2018: member

@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?

hebasto renamed this:
~~[WIP] qt: Cleanup MacDockIconHandler class~~
qt: Cleanup MacDockIconHandler class
on Oct 31, 2018

in src/qt/macdockiconhandler.mm:47 in 43627b961c outdated

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

fanquake commented at 7:30 am on November 2, 2018:

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 force-pushed on Nov 2, 2018

hebasto renamed this:
~~qt: Cleanup MacDockIconHandler class~~
[WIP] qt: Cleanup MacDockIconHandler class
on Nov 2, 2018

hebasto force-pushed on Nov 2, 2018

hebasto renamed this:
~~[WIP] qt: Cleanup MacDockIconHandler class~~
qt: Cleanup MacDockIconHandler class
on Nov 2, 2018

hebasto commented at 9:51 am on November 2, 2018: member

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

in src/qt/macdockiconhandler.mm:31 in efb681dc91 outdated

27@@ -34,74 +28,13 @@ void setupDockClickHandler() {
28         id delegate = objc_msgSend(appInst, sel_registerName("delegate"));
29         Class delClass = (Class)objc_msgSend(delegate,  sel_registerName("class"));
30         SEL shouldHandle = sel_registerName("applicationShouldHandleReopen:hasVisibleWindows:");
31-        if (class_getInstanceMethod(delClass, shouldHandle))
32-            class_replaceMethod(delClass, shouldHandle, (IMP)dockClickHandler, "B@:");
33-        else
34-            class_addMethod(delClass, shouldHandle, (IMP)dockClickHandler,"B@:");
35+        class_replaceMethod(delClass, shouldHandle, (IMP)dockClickHandler, "B@:");

Sjors commented at 5:00 pm on November 2, 2018:

Why did/do we need this class_replaceMethod stuff?

hebasto commented at 6:51 pm on November 2, 2018:

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.

Sjors approved

Sjors commented at 5:01 pm on November 2, 2018: member

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

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.

2464925e7b

Remove obj_c for macOS Dock icon menu

Qt `setAsDockMenu()` does this work.

6b1d2972bf

hebasto force-pushed on Nov 4, 2018

hebasto commented at 0:52 am on November 4, 2018: member

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

jonasschnelli commented at 2:01 pm on November 4, 2018: contributor

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 commented at 2:30 pm on November 4, 2018: member

@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?

meshcollider deleted a comment on Nov 4, 2018

jonasschnelli commented at 2:20 am on November 7, 2018: contributor

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 commented at 9:23 am on November 10, 2018: member

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.

laanwj merged this on Nov 12, 2018

laanwj closed this on Nov 12, 2018

hebasto deleted the branch on Nov 12, 2018

promag referenced this in commit 4d4bc37df9 on Dec 30, 2018

promag referenced this in commit 90347141bd on Dec 30, 2018

promag referenced this in commit 0c2fb87dc1 on Dec 30, 2018

laanwj referenced this in commit 5ff7b372cd on Jan 3, 2019

DrahtBot locked this on Sep 8, 2021

qt: Cleanup MacDockIconHandler class #14597

Conflicts