Pull request overview

This PR adjusts shutdown/teardown ordering for several Qt worker-thread QObjects so they are stopped and destroyed on their owning threads, addressing Qt thread-affinity warnings during application exit.

Changes:

• Update RadioModel teardown to stop worker objects via blocking queued calls, then schedule deleteLater() before quitting worker threads.
• Update MainWindow teardown to stop audio/controller resources on their worker threads and use deleteLater() instead of deleting from the GUI thread.
• Apply similar shutdown ordering to spot-client worker objects (DX cluster / RBN / WSJT-X / etc.) before quitting their worker thread.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Thanks for tackling this, @rfoust — the Qt thread-affinity warnings during shutdown are genuinely annoying and this is the right general approach (stop-on-worker → deleteLater() → quit thread). A few things to address before merging:

1. deleteLater() ordering vs quit() — potential for object leak

In several places the pattern is:

obj->deleteLater();
thread->quit();
thread->wait(3000);

deleteLater() posts a QDeferredDeleteEvent to the object's thread event loop. If quit() drains the loop before that event is processed, the deferred delete never fires and the object leaks. The safer Qt idiom is to connect the thread's finished signal to the object's deleteLater() slot, or call quit() first and let the event loop process the deferred delete during wind-down. In practice Qt does process pending deferred deletes during QThread::quit() wind-down in most cases, but the order shown here isn't guaranteed by the Qt docs.

Consider either:

• Connecting thread->finished → obj->deleteLater() (idiomatic Qt), or
• Keeping the explicit delete on the main thread after wait() returns (the thread is dead at that point, so cross-thread destruction is safe for objects whose timers/sockets have already been stopped via the preceding BlockingQueuedConnection call — which is exactly what the old code did after moveToThread).

2. Copilot's nullptr-after-deleteLater concern is valid

Copilot flagged this correctly in multiple places. For example in RadioModel.cpp, the reconnect-timer lambda at line ~368 does:

QMetaObject::invokeMethod(m_connection, [this] {
    m_connection->connectToRadio(m_lastInfo);
});

The PR nulls m_connection immediately after deleteLater(). If a queued invocation from the reconnect timer (or any other async path) fires between deleteLater() and actual destruction, it will dereference nullptr. The old code didn't null the pointer until after delete, which was fine since delete was synchronous.

The BlockingQueuedConnection call to disconnectFromRadio should prevent new reconnect attempts, and signals are disconnected at the top of the destructor — so in practice the race window is very small. But it's still a correctness issue. Safest fix: don't null the member pointers at all — the object is about to be destroyed anyway, so the member values are irrelevant after the destructor body completes.

The same applies to the MainWindow destructor nulling of m_serialPort, m_flexControl, m_midiControl, m_hidEncoder, and the closeEvent nulling of m_dxCluster etc.

3. Copilot's fallback-path concern for m_audio is valid

The old code always did delete m_audio unconditionally. The new code only calls deleteLater() inside the isRunning() branch. The else branch correctly adds delete m_audio, which addresses this — good. Just confirming Copilot's concern was handled.

4. The ext-controller block now stops MIDI/HID on the worker thread — nice addition

The old code only did BlockingQueuedConnection for m_serialPort->close(). Adding the same treatment for m_midiControl and m_hidEncoder is a good improvement that goes slightly beyond the stated scope but makes sense here.

5. Minor: duplicate quit()/wait() guard

In both RadioModel.cpp blocks there's a pattern like:

if (m_connThread && m_connThread->isRunning()) {
    // ...
    m_connThread->quit();
    m_connThread->wait(3000);
}
// then immediately:
if (m_connThread && m_connThread->isRunning()) {
    m_connThread->quit();
    m_connThread->wait(3000);
}

The second isRunning() check is only reachable when the first branch wasn't taken (thread not running), so the second block is dead code. It doesn't hurt, but it reads as if it's guarding against something — consider removing it or adding a comment explaining the intent.

Summary

The core idea is correct and welcome. The main actionable items are:

1. Don't null member pointers after deleteLater() — they serve no purpose (object is being destroyed) and create a nullptr race with queued lambdas.
2. Verify deleteLater() + quit() ordering — either use the finished → deleteLater() pattern, or keep synchronous delete after wait() since the blocking stop calls already ensure timers/sockets are inactive.

Happy to re-review after updates. Thanks for the contribution!