qt: Replace objc_msgSend() function calls with the native Objective-C syntax #16720

hebasto · 2019-08-25T10:34:58Z

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:

DrahtBot · 2019-08-25T12:07:04Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16738 (gui: Remove needless GCC diagnostic pragmas by hebasto)

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.

Sjors · 2019-08-25T14:05:43Z

I can confirm 77dabca still builds on macOS Mojave 10.14.6. It also fixes the build for me on macOS Catalina 10.15 beta 3.

Conversely you can reproduce the error in Mojave by setting -DOBJC_OLD_DISPATCH_PROTOTYPES=0.

I suggest tagging this 0.19, but hold off merging in favor of a solution that uses the new syntax. Apple deprecated this back in 2015.

laanwj · 2019-08-29T14:00:32Z

I suggest tagging this 0.19, but hold off merging in favor of a solution that uses the new syntax. Apple deprecated this back in 2015.

Done, agree here.

hebasto · 2019-08-29T19:57:36Z

I suggest tagging this 0.19, but hold off merging in favor of a solution that uses the new syntax.

Done with the native Objective-C syntax.

Sjors · 2019-08-29T20:11:45Z

Nice! Will test soon. Maybe set -DOBJC_OLD_DISPATCH_PROTOTYPES=0 for good measure?

hebasto · 2019-08-29T20:52:52Z

@Sjors

Maybe set -DOBJC_OLD_DISPATCH_PROTOTYPES=0 for good measure?

Done.

fanquake

Concept ACK

Definitely prefer actually fixing our code, rather than passing the flag to enable the old behaviour.

fanquake · 2019-08-30T01:44:53Z

src/qt/macdockiconhandler.mm

Given that we don't support Qt 5.4 anymore, can you update this comment?
Obviously this workaround is still required with Qt 5.5+. Is there a Qt version where it's no-longer necessary?

That would indeed be useful to know. I didn't try removing the workaround altogether.

Is there a Qt version where it's no-longer necessary?

Looking through the Qt code base:

since Qt 5.3:
https://github.com/qt/qtbase/blob/5.3/src/plugins/platforms/cocoa/qcocoaintegration.mm#L299

since Qt 5.6:
https://github.com/qt/qtbase/blob/5.6/src/plugins/platforms/cocoa/qcocoaapplicationdelegate.mm#L299

since Qt 5.8:
https://github.com/qt/qtbase/blob/5.8/src/plugins/platforms/cocoa/qcocoawindow.mm#L1013

I suppose the workaround is not needed on Qt 5.6+. But I have no capabilities to test this hypothesis now.

I didn't try removing the workaround altogether.

I did. It works as expected on Catalina (Beta 3) + Qt 5.13.

Our minimum QT version is 5.5.1, so maybe we should make a Github issue to consider dropping this workaround at the next bump https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md

8b6f5aa qt: Replace QFontMetrics::width() with TextWidth() (Hennadii Stepanov) Pull request description: Compiling master (d8fc997) on macOS Catalina (with a patch from #16720) reveals one more instance of `QFontMetrics::width()` which is supposed to be replaced with `TextWidth()` in the merged #16701. Sorry for incomplete solution provided in #16701. It’s especially sad that the line I missed lies in only 7 lines from the code touched in #16701. ACKs for top commit: fanquake: ACK 8b6f5aa Tree-SHA512: 65cd8bea550150e5ee47c1e906d8c2393547cf4feba3701a933a4f24fad5ecdb552ac2de4e1200ed14efaa0df0480150dd58fccbddc3b902f6c2141603874902

Sjors

Tested ACK f959d2f on Mojave and Catalina, using homebrew QT 5.13.0

Sjors · 2019-08-30T16:03:04Z

src/qt/macdockiconhandler.mm

That would indeed be useful to know. I didn't try removing the workaround altogether.

l2a5b1

Concept ACK!

l2a5b1 · 2019-08-30T17:50:46Z

src/qt/guiutil.cpp

Perhaps move the function declaration to macdockiconhandler.h and include this header instead?

What are benefits of doing that? A header including will increase the current translation unit, however.

When I noticed the function declaration it was not immediately obvious why the corresponding definition was not in the same cpp file. It was only obvious why it is as it is after I found the corresponding function definition in the mackdockiconhandler.mm file.

I thought that by declaring the function in the mackdockiconhandler.h header file and keeping the declaration and definition close to each other, it would save the next person from wondering the same.

But perhaps it just me. I am afraid I don't have a better argument.

src/qt/macdockiconhandler.mm

hebasto · 2019-08-31T09:38:17Z

@fanquake @Sjors @l2a5b1
Thank you all for your reviews. Your comments have been addressed.

jonasschnelli

utACK 0bb33b5 - Confirmed that the called macOS framework function is available on our build targets.

Travis error is irrelevant.

fanquake

ACK 0bb33b5 - Still works as expected.

l2a5b1 · 2019-09-01T08:00:51Z

ACK 0bb33b5 - Diff looks good. Sending messages via native Objective-C code feels more robust and is more readable than casting all the objc_msgSend function calls to the appropriate function signature (which would also have fixed the issue).

…ve Objective-C syntax 0bb33b5 qt: Replace objc_msgSend with native syntax (Hennadii Stepanov) Pull request description: 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](https://developer.apple.com/documentation/objectivec/1456712-objc_msgsend?changes=latest_minor&language=objc) - [`OBJC_OLD_DISPATCH_PROTOTYPES` macro](https://developer.apple.com/documentation/objectivec/objc_old_dispatch_prototypes?language=objc) ACKs for top commit: l2a5b1: ACK 0bb33b5 - Diff looks good. Sending messages via native Objective-C code feels more robust and is more readable than casting all the `objc_msgSend` function calls to the appropriate function signature (which would also have fixed the issue). jonasschnelli: utACK 0bb33b5 - Confirmed that the called macOS framework function is available on our build targets. fanquake: ACK 0bb33b5 - Still works as expected. Tree-SHA512: c09cb684d06bd1da053a17c182b7bb1642e45bb347d26c76e1c5d835c320567caee366d85e34bb7f2be38e63ed041e0d06a56c2a9d89f7e5bece9b19cc5c6772

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]>

…he native Objective-C syntax 0bb33b5 qt: Replace objc_msgSend with native syntax (Hennadii Stepanov) Pull request description: 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](https://developer.apple.com/documentation/objectivec/1456712-objc_msgsend?changes=latest_minor&language=objc) - [`OBJC_OLD_DISPATCH_PROTOTYPES` macro](https://developer.apple.com/documentation/objectivec/objc_old_dispatch_prototypes?language=objc) ACKs for top commit: l2a5b1: ACK 0bb33b5 - Diff looks good. Sending messages via native Objective-C code feels more robust and is more readable than casting all the `objc_msgSend` function calls to the appropriate function signature (which would also have fixed the issue). jonasschnelli: utACK 0bb33b5 - Confirmed that the called macOS framework function is available on our build targets. fanquake: ACK 0bb33b5 - Still works as expected. Tree-SHA512: c09cb684d06bd1da053a17c182b7bb1642e45bb347d26c76e1c5d835c320567caee366d85e34bb7f2be38e63ed041e0d06a56c2a9d89f7e5bece9b19cc5c6772

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

… syntax bitcoin/bitcoin#16720 TODO: add bringToFront method in guiutil.cpp and it's usage in komodooceangui.cpp .

* 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]>

8b6f5aa qt: Replace QFontMetrics::width() with TextWidth() (Hennadii Stepanov) Pull request description: Compiling master (d8fc997) on macOS Catalina (with a patch from bitcoin#16720) reveals one more instance of `QFontMetrics::width()` which is supposed to be replaced with `TextWidth()` in the merged bitcoin#16701. Sorry for incomplete solution provided in bitcoin#16701. It’s especially sad that the line I missed lies in only 7 lines from the code touched in bitcoin#16701. ACKs for top commit: fanquake: ACK 8b6f5aa Tree-SHA512: 65cd8bea550150e5ee47c1e906d8c2393547cf4feba3701a933a4f24fad5ecdb552ac2de4e1200ed14efaa0df0480150dd58fccbddc3b902f6c2141603874902

fanquake added Build system macOS labels Aug 25, 2019

hebasto mentioned this pull request Aug 25, 2019

macOS Catalina #16387

Closed

laanwj added this to the 0.19.0 milestone Aug 29, 2019

hebasto mentioned this pull request Aug 29, 2019

qt: Replace QFontMetrics::width() with TextWidth() #16758

Merged

hebasto force-pushed the 20190825-fix-catalina-build branch from 77dabca to 30fbfd8 Compare August 29, 2019 19:54

hebasto changed the title ~~build: Fix macOS build on Xcode 11~~ qt: Replace objc_msgSend() function calls with the native Objective-C syntax Aug 29, 2019

hebasto force-pushed the 20190825-fix-catalina-build branch 2 times, most recently from 5f6d923 to f959d2f Compare August 29, 2019 20:21

fanquake reviewed Aug 30, 2019

View reviewed changes

Sjors approved these changes Aug 30, 2019

View reviewed changes

src/qt/macdockiconhandler.mm Outdated

Copy link

Member

Sjors Aug 30, 2019

Choose a reason for hiding this comment

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

That would indeed be useful to know. I didn't try removing the workaround altogether.

l2a5b1 reviewed Aug 30, 2019

View reviewed changes

qt: Replace objc_msgSend with native syntax

0bb33b5

hebasto force-pushed the 20190825-fix-catalina-build branch from f959d2f to 0bb33b5 Compare August 31, 2019 09:27

jonasschnelli approved these changes Aug 31, 2019

View reviewed changes

fanquake approved these changes Sep 1, 2019

View reviewed changes

fanquake merged commit 0bb33b5 into bitcoin:master Sep 1, 2019

hebasto deleted the 20190825-fix-catalina-build branch September 1, 2019 12:52

ChillerDragon mentioned this pull request Sep 25, 2019

macOS build fails: objc_msgSend teeworlds/teeworlds#2245

Closed

Sjors mentioned this pull request Feb 2, 2020

macOS Catalina: probably needs bitcoin rebase bitcoinops/taproot-workshop#163

Closed

olegmitrakhovich mentioned this pull request Apr 10, 2020

MAC OS Catalina 10.15.4, PIVX 4.0 doesn't compile PIVX-Project/PIVX#1512

Closed

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

trappist mentioned this pull request Nov 27, 2020

Fix mac build issues gravio-net/graviocoin#1

Open

DeckerSU added a commit to DeckerSU/KomodoOcean that referenced this pull request Feb 25, 2021

qt: Replace objc_msgSend() function calls with the native Objective-C…

7c1f3c0

… syntax bitcoin/bitcoin#16720 TODO: add bringToFront method in guiutil.cpp and it's usage in komodooceangui.cpp .

ddoan mentioned this pull request Mar 15, 2021

qt: Replace objc_msgSend() with native Objective-C syntax bitcointokenbtct/Bitcoin-Token-Core#1

Merged

JaredTate mentioned this pull request Jul 14, 2021

v7.17.3 / develop branch won't compile GUI natively on macOS Big Sur v11.4 & Xcode 12.5.1 DigiByte-Core/digibyte#55

Closed

lyricidal mentioned this pull request Sep 13, 2021

[Upstream] [GUI] Back port latest MacOS dock icon handler. PRCYCoin/PRCYCoin#113

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

qt: Replace objc_msgSend() function calls with the native Objective-C syntax #16720

qt: Replace objc_msgSend() function calls with the native Objective-C syntax #16720

Uh oh!

Conversation

hebasto commented Aug 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Sjors commented Aug 25, 2019

Uh oh!

laanwj commented Aug 29, 2019

Uh oh!

hebasto commented Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Aug 29, 2019

Uh oh!

hebasto commented Aug 29, 2019

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake Aug 30, 2019

Choose a reason for hiding this comment

Uh oh!

Sjors Aug 30, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 31, 2019

Choose a reason for hiding this comment

Uh oh!

Sjors Aug 31, 2019

Choose a reason for hiding this comment

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Aug 30, 2019

Choose a reason for hiding this comment

Uh oh!

l2a5b1 left a comment

Choose a reason for hiding this comment

Uh oh!

l2a5b1 Aug 30, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 31, 2019

Choose a reason for hiding this comment

Uh oh!

l2a5b1 Aug 31, 2019 • 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!

Uh oh!

hebasto commented Aug 31, 2019

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

l2a5b1 commented Sep 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hebasto commented Aug 25, 2019 •

edited

Loading

DrahtBot commented Aug 25, 2019 •

edited

Loading

hebasto commented Aug 29, 2019 •

edited

Loading

l2a5b1 Aug 31, 2019 •

edited

Loading