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. 
   DrahtBot commented at 12:07 PM on August 25, 2019: member

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

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

6. laanwj added this to the milestone 0.19.0 on Aug 29, 2019
7. 
   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.

8. hebasto force-pushed on Aug 29, 2019
9. 
   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.

10. 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
11. 
    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?

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

    @Sjors

    Maybe set -DOBJC_OLD_DISPATCH_PROTOTYPES=0 for good measure?

    Done.

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

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

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

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

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

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

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

    Concept ACK!

23. qt: Replace objc_msgSend with native syntax 0bb33b5348
24. hebasto force-pushed on Aug 31, 2019
25. 
    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.

26. jonasschnelli approved
27. 
    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.

28. fanquake approved
29. 
    fanquake commented at 6:25 AM on September 1, 2019: member

    ACK 0bb33b5348dbddd65b88a7f00449c965562355d3 - Still works as expected.

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

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

    ---

34. hebasto deleted the branch on Sep 1, 2019
35. furszy referenced this in commit 24bc866346 on Apr 13, 2020
36. UdjinM6 referenced this in commit 124824da41 on Apr 30, 2020
37. 10xcryptodev referenced this in commit 148c2ddcc1 on May 14, 2020
38. michelvankessel referenced this in commit 532482c35b on Oct 25, 2020
39. michelvankessel referenced this in commit 110367de6a on Oct 25, 2020
40. wqking referenced this in commit d0b34513a5 on Nov 29, 2020
41. DeckerSU referenced this in commit 7c1f3c0c35 on Feb 25, 2021
42. ddoan referenced this in commit cff1f764a3 on Mar 15, 2021
43. ddoan referenced this in commit be5fd8672f on Mar 15, 2021
44. ckti referenced this in commit abcf559ce3 on Mar 28, 2021
45. gades referenced this in commit 278055c051 on Jun 26, 2021
46. PastaPastaPasta referenced this in commit 170c1c9d64 on Sep 11, 2021
47. PastaPastaPasta referenced this in commit d2492b6f31 on Sep 11, 2021
48. PastaPastaPasta referenced this in commit b42e8ef500 on Sep 12, 2021
49. PastaPastaPasta referenced this in commit f99d692e91 on Sep 12, 2021
50. PastaPastaPasta referenced this in commit 1d6920fa69 on Sep 12, 2021
51. PastaPastaPasta referenced this in commit ac03841c46 on Sep 14, 2021
52. MarcoFalke locked this on Dec 16, 2021

Contributors

hebastoDrahtBotSjorslaanwjfanquakel2a5b1jonasschnelli

Milestone
0.19.0