Skip to content

[REVIEW NEEDED] fixes for macOS - #255

Open
seiuryuu wants to merge 7 commits into
mainfrom
seiuryuu/macos-fix
Open

seiuryuu wants to merge 7 commits into
mainfrom
seiuryuu/macos-fix

Conversation

@seiuryuu

Copy link
Copy Markdown
Collaborator

Summary

Fixes two crashes and two layout issues on macOS.

1. Crash after the system button area is destroyed

Problem: setSystemButtonArea() kept a raw pointer to the area QWidget / QQuickItem. If the area was deleted while the agent was alive, the next button positioning pass (e.g. a window resize) used the dangling pointer.

Reproduce:

  1. Install the agent and register a system button area using setSystemButtonArea(area).
  2. Show the window in its normal state and let the first native layout complete.
  3. Delete the area while keeping both the window and agent alive, then resize the window and process events.

Fix: Track the area with QPointer, guard callback access, and clear the positioning callback when the area is destroyed. Also handle explicit clearing after the tracked pointer has become null.

2. QWidget content shifted down by the title bar height (#166)

Problem: Reported with Qt 6.8.2 in #166; the reporter did not observe it with Qt 6.6.3. Automatic QWidget safe-area margins can include the native title bar area on macOS, pushing the custom title bar and the content below it downward and leaving an empty strip at the top.

Reproduce: With upstream main and Qt 6.8.2 or Qt 6.12.0, install a WidgetWindowAgent on a QMainWindow, set a fixed-height menu widget using setMenuWidget(), register it using setTitleBar(), and show the window without manually changing its safe-area attributes. In the local comparison, the title bar started at y=32 instead of y=0; this branch placed it at y=0 before and after resizing.

Fix: Disable Qt::WA_ContentsMarginsRespectsSafeArea on the host widget, the title bar subtree, and the containers between them. Track hierarchy changes, restore affected child widgets' previous settings when they leave the managed hierarchy, and refresh layouts for already visible windows.

Set QWK_DISABLE_SAFE_AREA_WORKAROUND=1 before calling setup() to skip QWindowKit's QWidget safe-area adjustments.

3. Custom system button positions reset by AppKit (related to #131)

Problem: With a custom system button area, AppKit can lay out the traffic-light buttons again after QWindowKit has positioned them, for example on first show. The buttons stay at the default position until the next resize (comment).

Reproduce: Before showing the window, register a callback that returns a visibly non-default area, for example:

agent->setSystemButtonAreaCallback([](const QSize &) {
    return QRect(95, 0, 165, 35);
});

Show the window and let the first native layout complete without resizing or reinstalling the callback. On upstream main, the buttons remain at their default positions; resizing then moves them to the requested area. On this branch, the requested positions are applied during the first display cycle and remain correct after resizing. This was reproduced with both Widgets and Quick on Qt 6.8.2 and Qt 6.12.0.

Fix: Apply the native title bar style before positioning the buttons. While a custom area is set, observe the buttons' frame changes and coalesce position restoration on the main queue without a fixed delay. Guard queued updates against proxy destruction, suppress notifications generated by QWindowKit's own positioning writes, and skip redundant moves.

In native fullscreen, button positioning remains controlled by AppKit. Custom positions are reapplied after exiting fullscreen if a custom area or callback is still registered.

4. Crash when adding a QQuickWidget to a visible window

Problem: Adding a QQuickWidget to a visible window can change the required surface type and cause Qt to recreate the top-level native window. Qt can release the old NSView before QWidget delivers WinIdChange. NSWindowProxy held that view without owning it, and called removeObserver:forKeyPath: on it after it was deallocated.

Reproduce: On upstream main (5626b3a), the program below crashed with SIGSEGV in 5/5 runs with Qt 6.12.0. It also reproduced with Qt 6.8.2. No explicit winId() call or QML file is required. With NSZombieEnabled=YES, the Qt 6.12.0 run reports:

*** -[NSKVONotifying_QNSView removeObserver:forKeyPath:]: message sent to deallocated instance 0x...
main.cpp / CMakeLists.txt
#include <QtCore/QCoreApplication>
#include <QtCore/QTimer>
#include <QtWidgets/QApplication>
#include <QtWidgets/QMainWindow>
#include <QtQuickWidgets/QQuickWidget>

#include <QWKWidgets/widgetwindowagent.h>

class MainWindow : public QMainWindow {
public:
    explicit MainWindow(QWidget *parent = nullptr) : QMainWindow(parent) {
        auto *agent = new QWK::WidgetWindowAgent(this);
        agent->setup(this);

        auto *titleBar = new QWidget(this);
        titleBar->setFixedHeight(32);
        setMenuWidget(titleBar);

        agent->setTitleBar(titleBar);
    }
};

int main(int argc, char *argv[]) {
    QCoreApplication::setAttribute(Qt::AA_DontCreateNativeWidgetSiblings);

    QApplication app(argc, argv);
    MainWindow w;
    w.resize(800, 600);
    w.show();

    // Adding a QQuickWidget can cause Qt to recreate the top-level native window.
    QTimer::singleShot(0, &w, [&w] { w.setCentralWidget(new QQuickWidget); });

    return app.exec();
}
cmake_minimum_required(VERSION 3.19)
project(qwk_quickwidget_crash LANGUAGES CXX)

set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

find_package(Qt6 REQUIRED COMPONENTS Core Gui Widgets QuickWidgets)
find_package(QWindowKit REQUIRED COMPONENTS Widgets)

add_executable(qwk_quickwidget_crash main.cpp)
target_link_libraries(qwk_quickwidget_crash PRIVATE
    Qt6::Core
    Qt6::Gui
    Qt6::Widgets
    Qt6::QuickWidgets
    QWindowKit::Widgets
)

Fix: NSWindowProxy retains its NSView and releases it after removing its observer. This keeps the view alive during cleanup and prevents its address from being reused while the proxy still references it.

@wangwenx190

Copy link
Copy Markdown
Collaborator

Please try the safe-fixes branch, it also contains some fixes for macOS, maybe it can fix your issue

This branch has not been deployed

No deployments
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.

2 participants