Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Qt to 6.5.3 #666

Merged
merged 9 commits into from
May 6, 2024
Merged

Update Qt to 6.5.3 #666

merged 9 commits into from
May 6, 2024

Conversation

jdpurcell
Copy link
Contributor

@jdpurcell jdpurcell commented Feb 18, 2024

Explanation of changes:

  • Use Qt 6.5.3. I've been using this in my builds for months and it's been working fine. Could go to 6.6.x I suppose but I haven't seen anything really important that it adds yet, and have only tested it very briefly.
  • Use kimageformats-binaries 6.4.3 builds for now. Should be okay due to Qt's backwards binary compatibility.
  • Includes a workaround for window geometry save/restore on macOS, because the full-size content view stuff doesn't play nicely with some more recent changes in Qt's geometry save/restore logic. Without this workaround, if a window is moved to the very top of the monitor (touching the menu bar), upon exiting and reopening, the window ends up offset from the top a bit.
  • There was an old workaround for QTBUG-69975 which was fixed as of Qt 6.2, so I surrounded it with an #if.
  • For the 64-bit Windows build, updates to OpenSSL 3 since Qt 6.5 won't load the OpenSSL 1.1 libraries as far as I can tell (checked with QSslSocket::activeBackend()). Even without OpenSSL, networking still works because Qt 6 has SChannel fallback, but still including OpenSSL just in case any of its features are needed (not sure).

Other changes (build errors/warnings):

  • The URL for the OpenSSL 1.1 zip was updated because the old one stopped working. Changed syntax for Invoke-WebRequest (e.g. -OutFile instead of -O) because that was somehow broken too (I guess one of the new runner releases updated PowerShell?).
  • Included commit from Fix deprecation warnings #675, thanks @MaykGyver. That PR was accidentally based on my master branch though hence all the other commits on that PR. The v4 upload-artifact upgrade revealed an additional issue, since it now checks for duplicate artifact names - the Linux builds were apparently named the same and I believe one was overwriting the other - so I suffixed one of them.
  • GitHub recently changed the OS that macos-latest refers to from 12 to 14, breaking the legacy macOS build. This is because the newer macOS runners (both 13 and 14) use Xcode 15.0.1 by default which has some issues as documented here. Note that if # 1 is worked around as I initially tried here, the others involve potential crashes at runtime even if the build is successful (as discovered in Float on top #677). So I left them both on macos-12 for this PR as the easy/safe fix. Or to build on macos-14, you could consider the other workaround I ended up using.
  • Adjusted the macOS build script to fix some warnings/errors in the deploy step for the nightly versions, a few which were already happening before ("file exists") due to running macdeployqt twice on the same app, and one that seems Qt 6.5-related ("cannot resolve rpath" for "QtPrintSupport") if macdeployqt tries to create its own dmg.

NOTE: Qt 6.5 requires macOS 11 or later, dropping support for macOS 10.14 and 10.15 as compared to Qt 6.2 (see Qt 6.2 Supported Platforms, Qt 6.5 Supported Platforms). If accepted/merged, remember to update the download page at the time of release to show "macOS 11+" for the non-legacy .dmg.

@@ -189,6 +185,12 @@ void MainWindow::contextMenuEvent(QContextMenuEvent *event)

void MainWindow::showEvent(QShowEvent *event)
{
#ifdef COCOA_LOADED
Copy link
Owner

Choose a reason for hiding this comment

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

Could we leave a comment here to explain what this does?

@@ -200,6 +202,10 @@ void MainWindow::showEvent(QShowEvent *event)

void MainWindow::closeEvent(QCloseEvent *event)
{
#ifdef COCOA_LOADED
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto above

{
auto *view = reinterpret_cast<NSView*>(window->winId());

// If this Qt and macOS version combination is already using layer-backed view, then enable full size content view
if (view.wantsLayer)
const bool isAlreadyEnabled = view.window.styleMask & NSWindowStyleMaskFullSizeContentView;
Copy link
Owner

Choose a reason for hiding this comment

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

Can we have more documentation here, on what this does and why it has to be done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what to put for comments here, I think the ones I just added in mainwindow covers it. Previously this function could only enable full size content view; the changes here are to allow it to be disabled as well. The isAlreadyEnabled stuff is just to protect against double calls (e.g. trying to disable it if it's already disabled).

Copy link
Owner

Choose a reason for hiding this comment

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

It would be good to be able to get a bit more info about what this function needs to do in its own context. It would be good to document:

  • the isAlreadyEnabled you mentioned
  • what is the purpose of shouldEnable?
  • What is the purpose of doing the setFrame situation?

Copy link
Contributor Author

@jdpurcell jdpurcell May 6, 2024

Choose a reason for hiding this comment

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

I cleaned things up a bit and added comments. I'm not even sure what the titlebarOverlap part was about anymore, I believe it was not strictly for the geometry save/restore workaround but somehow needed for the macOS session state feature I developed around the same time to work properly. Although even with that feature I'm not seeing problems by removing it, I probably eliminated the need for it with subsequent changes I made.

EDIT: I'm thinking it could have also been a workaround for an older macOS bug, where it would disallow full size content view from being turned off if doing so (which normally makes the window taller upward by the title bar height) would place the top too high inside the menu bar. But I can't reproduce that anymore.

@jurplel jurplel merged commit 018ef8a into jurplel:master May 6, 2024
6 checks passed
@jdpurcell jdpurcell mentioned this pull request May 6, 2024
3 tasks
@jurplel
Copy link
Owner

jurplel commented May 6, 2024

Thanks!

@jdpurcell jdpurcell deleted the qt_6_5 branch June 29, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICC Profile when enabled, instantly crashes qView
3 participants