-
Notifications
You must be signed in to change notification settings - Fork 120
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
Update Qt to 6.5.3 #666
Conversation
@@ -189,6 +185,12 @@ void MainWindow::contextMenuEvent(QContextMenuEvent *event) | |||
|
|||
void MainWindow::showEvent(QShowEvent *event) | |||
{ | |||
#ifdef COCOA_LOADED |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto above
src/qvcocoafunctions.mm
Outdated
{ | ||
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Thanks! |
Explanation of changes:
#if
.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):
Invoke-WebRequest
(e.g.-OutFile
instead of-O
) because that was somehow broken too (I guess one of the new runner releases updated PowerShell?).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.