qt6: Handle different signatures of QANEF::nativeEventFilter #840

1. 
   hebasto commented at 12:00 pm on October 4, 2024: member

   Split from https://github.com/bitcoin/bitcoin/pull/30997.

   This PR ensures compatibility across all supported Qt versions.

   For more details, please refer to https://github.com/qt/qtbase/commit/3b38c73c7ffa71c00c172cf0e05742835a304300.

   No behaviour change.

2. qt6: Handle different signatures of `QANEF::nativeEventFilter`

   This change ensures compatibility across all supported Qt versions.

   80761afced
3. hebasto added the label Refactoring on Oct 4, 2024
4. hebasto added the label Qt 6 on Oct 4, 2024
5. 
   DrahtBot commented at 12:00 pm on October 4, 2024: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	promag, maflcko

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #bitcoin/bitcoin/30997 (build: Switch to Qt 6 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. 
   fanquake commented at 12:16 pm on October 4, 2024: member

   If Qt 6.7.0 is going to be required for Windows after the switchover; why change this code now just to immediately drop it after migrating to Qt 6?
7. 
   hebasto commented at 12:50 pm on October 4, 2024: member

   If Qt 6.7.0 is going to be required for Windows after the switchover; why change this code now just to immediately drop it after migrating to Qt 6?

   To make https://github.com/bitcoin/bitcoin/pull/30997 focused on the build system changes only.

   We already have:

   0$ git grep 'QT_VERSION_CHECK(6, 0, 0)'
   1src/qt/addressbookpage.cpp:#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
   2src/qt/addressbookpage.cpp:#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
   3src/qt/bitcoin.cpp:#if (QT_VERSION < QT_VERSION_CHECK(6, 0, 0))
   4src/qt/bitcoin.cpp:#if (QT_VERSION < QT_VERSION_CHECK(6, 0, 0))
   5src/qt/bitcoin.cpp:#if (QT_VERSION < QT_VERSION_CHECK(6, 0, 0))
   

8. 
   fanquake commented at 1:00 pm on October 4, 2024: member

   Not sure we should be having to open and review multiple additional PRs (one now, one later to remove the code again), and be making pointless code changes, just to keep a different PR “focussed”. Especially when the changes you’re trying to extract are completely relevant.
9. 
   promag commented at 11:05 pm on October 4, 2024: contributor

   I think small changes to make code compatible with both versions are fine until support for qt5 is dropped.

   Code review ACK 80761afced12c774a1768fb27a3975d456981ae0.

   Alternative is to use type alias

   0#if (QT_VERSION >= QT_VERSION_CHECK(6, 0, 0))
   1using result_t = qintptr*;
   2#else
   3using result_t = long*;
   4#endif
   

10. 
    maflcko commented at 8:03 am on October 7, 2024: contributor

    lgtm ACK 80761afced12c774a1768fb27a3975d456981ae0
11. 
    fanquake commented at 8:05 am on October 7, 2024: member

    I think small changes to make code compatible with both versions are fine

    I don’t think that is the case here? Qt 6 isn’t supported for windows until the later PR, at which point Qt 5 isn’t supported?

12. 
    promag commented at 8:26 am on October 7, 2024: contributor

    @fanquake oh I see your point, we won’t need to support the old signature of qt5 so the diff could be just the arg type change.

    at which point Qt 5 isn’t supported?

    Unless this is not the case? @hebasto?

    I’m keeping the ACK, but what @fanquake suggests is fine too, with less maintenance efforts.

13. 
    fanquake commented at 8:30 am on October 7, 2024: member

    @promag see this commit in the switchover PR: https://github.com/bitcoin/bitcoin/commit/812f3e7727211a60989e613bd5edf0a4ec3f146f. Qt 6.7.0 or later will be required for Windows (although as far as I’m aware, Qt 5 won’t be supported generally in any case).
14. 
    maflcko commented at 8:34 am on October 7, 2024: contributor

    Looks good to me either way as well. Merge now and remove later, or change later. Not sure how hard the overall changes are, but “spreading-out” review could be useful. Though, no strong opinion, either way is fine.
15. 
    hebasto commented at 9:29 am on October 7, 2024: member

    After an offline discussion with @fanquake, I’m going to proceed with merging this PR.
16. hebasto merged this on Oct 7, 2024
17. hebasto closed this on Oct 7, 2024

    ---

Contributors
hebasto DrahtBot fanquake promag maflcko

Labels
Refactoring Qt 6