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

pull droark wants to merge 2 commits into bitcoin:master from droark:rm_icon_buffer changing 1 files +6 −14
  1. droark commented at 9:13 am on June 28, 2018: contributor

    The code curently writes the Mac dock icon to a buffer and then sets the final image data from the buffer. This was presumably done to satisfy Qt 4. Now that Qt 5 is mandatory, remove the buffer and write the data directly to the image.

    Note that this patch will force the bitcoin-qt binary to have a minimum of Qt 5.2 on Macs due to usage of QtMacExtras (https://wiki.qt.io/New_Features_in_Qt_5.2). While this needs to be discussed before accepting the patch, Qt 5.2 is almost five years old. It’s arguable that a Mac with Qt 5 but staying below 5.2 is an improbable corner case.

  2. fanquake added the label GUI on Jun 28, 2018
  3. fanquake added the label macOS on Jun 28, 2018
  4. promag commented at 9:46 am on June 28, 2018: member

    Discussion about minimum Qt5 version in #13478.

    utACK ec16f4a.

  5. promag commented at 10:14 am on June 28, 2018: member

    Build error:

    0checking for QT5... no
1configure: WARNING: Qt dependencies not found; bitcoin-qt frontend will not be built
  6. fanquake commented at 6:08 am on July 1, 2018: member

    This would be a nice to have cleanup/simplification. I’ve tested on macOS 10.13.5, building with Qt 5.11.1:

    ec16f4a:

    However, there are a couple issues:

    This PR isn’t the place to discuss it, but I’m currently Concept NACK on adding per platform Qt minimums. I’ll continue that discussion in #13478.

    As pointed out above, the macOS Travis build is currently failing. I’ve verified locally using a native depends build:

    0checking for Qt5Core Qt5Gui Qt5Network Qt5Widgets Qt5MacExtras... no
1configure: WARNING: Qt dependencies not found; bitcoin-qt frontend will not be built
2checking whether to build Bitcoin Core GUI... no (Qt5)

    The issue is that we aren’t currently including macExtras with qt in depends, so if we want to do this our qt package will need to be updated.

    Also, if this change is going to force the macOS Qt minimum to be 5.2, can you drop this Qt VESION check: https://github.com/bitcoin/bitcoin/blob/a6ed99a1e6facd38913711106bce6fb65bd14862/src/qt/macdockiconhandler.mm#L53

  7. droark commented at 6:13 am on July 1, 2018: contributor
    @fanquake - Thanks. Was looking into the Qt package when you commented. I’ll pick up the conversation at 13478 and update this PR as necessary.
  8. jonasschnelli commented at 7:48 pm on July 12, 2018: contributor
    Concept ACK
  9. fanquake commented at 12:00 pm on July 18, 2018: member
    @droark Have you had a chance to follow up here?
  10. droark commented at 10:53 pm on July 18, 2018: contributor
    @fanquake - Hi. I started working on some depends stuff and then got distracted. I’ll pick it up ASAP. I’ll see if I can optimize any more Mac-specific code but that might take a little longer. In the meantime, I’ve rebased.
  11. droark force-pushed on Jul 18, 2018
  12. laanwj commented at 11:42 am on August 31, 2018: member

    Note that this patch will force the bitcoin-qt binary to have a minimum of Qt 5.2 on Macs due to usage of QtMacExtras

    That’s acceptable.

  13. fanquake commented at 5:18 am on October 9, 2018: member

    @droark Are you planning on continuing this? Otherwise II’ll label as “Up for Grabs”. \

    The 5.2 requirement is no longer an issue, as master currently requires >5.4 anyways. The plan for 0.18 will likely be a minimum required version above 5.2 regardless.

  14. MarcoFalke added the label Up for grabs on Oct 9, 2018
  15. droark commented at 8:03 am on October 9, 2018: contributor
    @fanquake - Yes, I’m still on it. I was just waiting to see what was decided regarding Qt (i.e., 5.4 minimum). I’ll rebase it right now and look into the failures from there.
  16. Remove unnecessary image buffer for Mac dock icon 
    The code curently writes the Mac dock icon to a buffer and then sets the final image data from the buffer. This was presumably done to satisfy Qt 4. Now that Qt 5 is mandatory, remove the buffer and write the data directly to the image.

Note that this patch will force the bitcoin-qt binary to have a minimum of Qt 5.2 on Macs due to usage of QtMacExtras (https://wiki.qt.io/New_Features_in_Qt_5.2). While this needs to be discussed before accepting the patch, Qt 5.2 is almost five years old. It's arguable that a Mac with Qt 5 but staying below 5.2 is an improbable corner case.
    b9e8f01842
  17. droark force-pushed on Oct 9, 2018
  18. Remove QtMacExtras requirement 
    Cross-compiling for platform-specific Qt extras may not be possible. Work around by copying the code from QtMac::toNSImage() and modifying as necessary. A redundant Qt version check was also removed.
    462b35b827
  19. droark commented at 9:13 am on October 10, 2018: contributor
    Added a new commit that incorporated feedback. I also removed QtMacExtras. As best I can tell, trying to cross-compile Qt “extras” packages isn’t a good idea. I did an end-run by just copying the code for QtMac::toNSImage() and modifying as necessary.
  20. droark commented at 7:02 pm on October 10, 2018: contributor
    Hmmm. Still getting an error when cross-compiling. In particular, toCGImage() still requires QtMacExtras, and the underlying code seems to tap into a private Qt API that we have no business using. (Besides, it fails as-is.) This PR may need some rethinking. Anybody have any ideas? The cleanest thing to do would be to cross-compile QtMacExtras. I’m just not sure it’s doable, or sane if it is doable. If not, this PR will probably have to be withdrawn, or radically reworked. The current method, while inefficient, manages to avoid extraneous Mac-specific libraries.
  21. MarcoFalke removed the label Up for grabs on Oct 28, 2018
  22. DrahtBot commented at 0:56 am on October 29, 2018: member
    • #14597 (qt: Remove old MacDockIconHandler class 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.

  23. droark commented at 1:18 am on October 29, 2018: contributor
    @DrahtBot - Thanks. I’ll look at that PR more closely. At a glance, it should be a cleaner merge, so I’ll probably work around that one (assuming my PR can be salvaged, per my posts above).
  24. fanquake commented at 12:29 pm on October 29, 2018: member
    @droark Given that #14597 removes the entire macdockiconhandler.mm file, I’m not sure there’ll be much to salvage (code wise) here. I’m going to close this for now, if it turns out this is still needed, i.e 14597 falls through we’ll reopen. If you’ve got further macOS depends related, please open a new PR.
  25. fanquake closed this on Oct 29, 2018
  26. DrahtBot locked this on Sep 8, 2021


droark promag fanquake jonasschnelli laanwj DrahtBot

Labels
GUI macOS

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me