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

1. 
   hebasto commented at 10:34 AM on August 25, 2019: member

   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
2. fanquake added the label Build system on Aug 25, 2019
3. fanquake added the label macOS on Aug 25, 2019
4. hebasto cross-referenced this on Aug 25, 2019 from issue macOS Catalina by Sjors
5. 
   DrahtBot commented at 12:07 PM on August 25, 2019: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   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.

6. 
   Sjors commented at 2:05 PM on August 25, 2019: member

   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.

7. laanwj added this to the milestone 0.19.0 on Aug 29, 2019
8. 
   laanwj commented at 2:00 PM on August 29, 2019: member

   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.

9. hebasto cross-referenced this on Aug 29, 2019 from issue qt: Replace QFontMetrics::width() with TextWidth() by hebasto
10. hebasto force-pushed on Aug 29, 2019
11. 
    hebasto commented at 7:57 PM on August 29, 2019: member

    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.

12. hebasto renamed this:
    build: Fix macOS build on Xcode 11
    qt: Replace objc_msgSend() function calls with the native Objective-C syntax
    on Aug 29, 2019
13. 
    Sjors commented at 8:11 PM on August 29, 2019: member

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

14. hebasto force-pushed on Aug 29, 2019
15. hebasto force-pushed on Aug 29, 2019
16. 
    hebasto commented at 8:52 PM on August 29, 2019: member

    @Sjors

    Maybe set -DOBJC_OLD_DISPATCH_PROTOTYPES=0 for good measure?

    Done.

17. in src/qt/macdockiconhandler.mm:48 in f959d2f642 outdated

      42 | @@ -44,3 +43,12 @@ void setupDockClickHandler() {
      43 |  {
      44 |      delete s_instance;
      45 |  }
      46 | +
      47 | +/**
      48 | + * Force application activation on macOS. With Qt 5.4 this is required when
    

    ---

    ---

    

    fanquake commented at 1:44 AM on August 30, 2019:

    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?

    ---

    

    Sjors commented at 4:03 PM on August 30, 2019:

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

    ---

    

    hebasto commented at 7:59 AM on August 31, 2019:

    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.

    ---

    

    Sjors commented at 8:11 AM on August 31, 2019:

    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

18. 
    fanquake commented at 1:46 AM on August 30, 2019: member

    Concept ACK

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

19. fanquake referenced this in commit f5db3f2128 on Aug 30, 2019
20. Sjors approved
21. 
    Sjors commented at 4:03 PM on August 30, 2019: member

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

22. in src/qt/guiutil.cpp:64 in f959d2f642 outdated

      57 | @@ -58,9 +58,10 @@
      58 |  #pragma GCC diagnostic push
      59 |  #pragma GCC diagnostic ignored "-Wdeprecated-declarations"
      60 |  
      61 | -#include <objc/objc-runtime.h>
      62 |  #include <CoreServices/CoreServices.h>
      63 |  #include <QProcess>
      64 | +
      65 | +void ForceActivation();
    

    ---

    ---

    

    l2a5b1 commented at 5:50 PM on August 30, 2019:

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

    ---

    

    hebasto commented at 8:06 AM on August 31, 2019:

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

    ---

    

    l2a5b1 commented at 4:52 PM on August 31, 2019:

    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.

23. in src/qt/macdockiconhandler.mm:25 in f959d2f642 outdated

      19 | @@ -21,9 +20,9 @@ bool dockClickHandler(id self, SEL _cmd, ...) {
      20 |  }
      21 |  
      22 |  void setupDockClickHandler() {
      23 | -    id app = objc_msgSend((id)objc_getClass("NSApplication"), sel_registerName("sharedApplication"));
      24 | -    id delegate = objc_msgSend(app, sel_registerName("delegate"));
      25 | -    Class delClass = (Class)objc_msgSend(delegate, sel_registerName("class"));
      26 | +    id app = [NSApplication sharedApplication];
      27 | +    id delegate = [app delegate];
      28 | +    Class delClass = (Class)[delegate class];
    

    ---

    ---

    

    l2a5b1 commented at 5:53 PM on August 30, 2019:

    Simplify id app = ...; id delegate = ...; Class delClass = ...; to Class delClass = [NSApplication.sharedApplication.delegate class];?

    ---

    

    hebasto commented at 8:20 AM on August 31, 2019:

    As dot notation is present in both C++ and Objective-C syntaxes, I'd prefer to keep the native Objective-C code as different from C++'s one as possible in mixed "Objective-C++" source files.

    The suggestion to combine some statements into the single one will be implemented.

24. 
    l2a5b1 commented at 6:18 PM on August 30, 2019: contributor

    Concept ACK!

25. qt: Replace objc_msgSend with native syntax 0bb33b5348
26. hebasto force-pushed on Aug 31, 2019
27. 
    hebasto commented at 9:38 AM on August 31, 2019: member

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

28. jonasschnelli approved
29. 
    jonasschnelli commented at 7:39 PM on August 31, 2019: contributor

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

    Travis error is irrelevant.

30. fanquake approved
31. 
    fanquake commented at 6:25 AM on September 1, 2019: member

    ACK 0bb33b5348dbddd65b88a7f00449c965562355d3 - Still works as expected.

32. 
    l2a5b1 commented at 8:00 AM on September 1, 2019: contributor

    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).

33. fanquake referenced this in commit 7d6f63cc2c on Sep 1, 2019
34. fanquake merged this on Sep 1, 2019
35. fanquake closed this on Sep 1, 2019

    ---

36. hebasto deleted the branch on Sep 1, 2019
37. ChillerDragon cross-referenced this on Sep 25, 2019 from issue macOS build fails: objc_msgSend by ChillerDragon
38. Sjors cross-referenced this on Feb 2, 2020 from issue macOS Catalina: probably needs bitcoin rebase by Sjors
39. olegmitrakhovich cross-referenced this on Apr 10, 2020 from issue MAC OS Catalina 10.15.4, PIVX 4.0 doesn't compile by olegmitrakhovich
40. furszy cross-referenced this on Apr 10, 2020 from issue [GUI] Back port latest MacOS dock icon handler. by furszy
41. furszy referenced this in commit 24bc866346 on Apr 13, 2020
42. xdustinface cross-referenced this on Apr 27, 2020 from issue Backport bitcoin#14123 and bitcoin#16720 by xdustinface
43. UdjinM6 referenced this in commit 124824da41 on Apr 30, 2020
44. 10xcryptodev referenced this in commit 148c2ddcc1 on May 14, 2020
45. CaveSpectre11 cross-referenced this on Jul 5, 2020 from issue Address segmentation faults in the Log message handlers on macOS. by sinetek
46. michelvankessel referenced this in commit 532482c35b on Oct 25, 2020
47. michelvankessel referenced this in commit 110367de6a on Oct 25, 2020
48. trappist cross-referenced this on Nov 27, 2020 from issue Fix mac build issues by trappist
49. wqking referenced this in commit d0b34513a5 on Nov 29, 2020
50. DeckerSU referenced this in commit 7c1f3c0c35 on Feb 25, 2021
51. ddoan cross-referenced this on Mar 15, 2021 from issue qt: Replace objc_msgSend() with native Objective-C syntax by ddoan
52. ckti referenced this in commit abcf559ce3 on Mar 28, 2021
53. gades referenced this in commit 278055c051 on Jun 26, 2021
54. JaredTate cross-referenced this on Jul 14, 2021 from issue v7.17.3 / develop branch won't compile GUI natively on macOS Big Sur v11.4 & Xcode 12.5.1 by JaredTate
55. PastaPastaPasta referenced this in commit 170c1c9d64 on Sep 11, 2021
56. PastaPastaPasta referenced this in commit d2492b6f31 on Sep 11, 2021
57. PastaPastaPasta referenced this in commit b42e8ef500 on Sep 12, 2021
58. PastaPastaPasta referenced this in commit f99d692e91 on Sep 12, 2021
59. PastaPastaPasta referenced this in commit 1d6920fa69 on Sep 12, 2021
60. lyricidal cross-referenced this on Sep 13, 2021 from issue [Upstream] [GUI] Back port latest MacOS dock icon handler. by lyricidal
61. PastaPastaPasta referenced this in commit ac03841c46 on Sep 14, 2021
62. bitcoin locked this on Dec 16, 2021

Contributors

hebastoDrahtBotSjorslaanwjfanquakel2a5b1jonasschnelli

Milestone
0.19.0