(fix) assert WTrackTableView has a selectionModel()

[ronso0]

Issue:

Apparently it can happen that WTrackTableView::slotGuiTick50ms is called while may not yet have an item model, which in turn results in nonexistant selectionModel()

Fix:

(actually only a workaround for the WTrackTableView with no model when GuiTick [App], gui_tick_50ms_period_s is started)
Assert we have a selection model, ie. cherry-pick and extend a6f1ba4
exit WTrackTableView::slotGuiTick50ms early if WTrackTableView !isVisible()

(I already did this locally and I didn' notice any regressions, but it'll cause quite some conflicts when merging 2.4 into 2.5/main)

Initially experienced this with main, but also happens with 2.4

_{old description, based on wrong assumption: Issue: Mapping error dialog can block loading the main GUI. If this happens sufficiently fast (eg. with MIDI Through in developer mode), WTrackTableView may not yet have an item model, which in turn results in nonexistant selectionModel() in WTrackTableView::slotGuiTick50ms. More precisely: ErrorDialogProperties show a modal dialog by default, which can block MixxxMainWindow::initialize() before it reaches loadConfiguredSkin(). This function emits skinLoaded --> SidebarModel::activateDefaultSelection() --> WTrackTableView loads LibraryTableModel. Ie. if this is blocked there is one WTrackTableView without a model, and thereby without a selectionModel(), which causes the crash. Quick fix: Make the mapping error dialogs non-modal. IIUC the purpose of modal dialogs is solely [Blocks so the user has a chance to read it before application exit](https://github.com/mixxxdj/mixxx/blob/d9704db8b9be57730d64594437a9a0b1fc754d56/src/errordialoghandler.cpp#L227) which (IIUC) is not required during startup. (if a mapping can crash Mixxx would this dialog be brought up in time?)}

[daschuer]

I don't yet understand why the box was modal before (not tested). The comment alone seems to be incomplete.

A modal box disabled the main GUI. Why schoul that happen at all in case of a controller error? The user has no chance to perform the desired action with the GUI instead.

I think there is an issue that the non modal boxes can be hidden behind the main window. Is that an issue here?

[daschuer]

At least ln case of crash the modal box disappears immediately. I can imagine that a box stays open when mixxx is closed normally by the error. Does it still happen?

[ronso0]

A modal box disabled the main GUI. Why schoul that happen at all in case of a controller error?

🤷

The user has no chance to perform the desired action with the GUI instead.

I presume this is supposed to cover the case where the mapping script's shutdown function throws an error.
edit: no, the shutdown function is evluated when the script is loaded.

[ronso0]

I think there is an issue that the non modal boxes can be hidden behind the main window. Is that an issue here?

IIUC QMessageBox is always modal (stays on top). (even though it apparently doesn't have the flag (hint!) Qt::WindowStaysOnTopHint)

Fwiw in ErrorDialogHandler the m_modal flag is not related to window modality but makes the error dialog block execution, ie. shown with exec()) instead of show().
edit: but actually only block ErrorDialogHandler::errorDialog in case the m_shouldQuit flag has been set.
Why it blocks MixxxMainWindow::initialize() 🤷 but it does.

_{Note that if we want to make sure, there's also the `[Qt::WindowStaysOnTopHint](https://doc.qt.io/qt-6/qt.html#WindowType-enum)` window flag > Informs the window system that the window should stay on top of all other windows. Note that on some window managers on X11 you also have to pass Qt::X11BypassWindowManagerHint for this flag to work correctly.}

[ronso0]

puuuh, sorry for the noise, it's entirely unrelated to the dialog and its modal flag.
That blocks only the error dialog function and this is only relevant errors thrown by reportFatalErrorAndQuit and reportCriticalErrorAndQuit 🤦‍♂️

---

However, for some reason I can still reliably reproduce the crash related to slotGuiTick50ms with a mapping loaded to MIDI Though (mapping is cleaned up, doesn't throw any errors).
So this is still the fix:

• Assert we have a selection model, ie. cherry-pick and extend a6f1ba4
• exit WTrackTableView::slotGuiTick50ms early if WTrackTableView !isVisible()

Will push soon..

[ronso0]

With the fix and some debug output I can see it's only the very first call of slotGuiTick50ms that would cause the crash.

I'll take care of merging to 2.5

[daschuer]

I have not a setup to confirm the fix but the code looks reasonable even without fixing an issue. Thnak you. LGTM

[ronso0]

Yeah, I'm wondering why I didn't ever see such a crash and can reproduce it now each time. My env didn't change significantly, same Qt version, same Mixxx user data. Very weird..

[ronso0]

Update
one more thing that can trigger the selectionModel() assert:
if a mapping triggers any of the [Library],LoadSelectedTrack.. controls or [Library],GoToItem (or ??) too early, ie. in its init() or triggered by some callback.

This is the setup I can reliably reproduce it:

• (build with DEBUG_ASSERTIONS_FATAL)
• start with --developer flag
• load the built-in Numark Total Control mapping to MIDI Through Port-0
• MIDI Through receives various incoming signals¹, eg. one for [Channel2],LoadSelectedTrack
• WTracktableView has no track model (no model() at all) and no selectionModel() yet, DEBUG_ASSERT

Running into assertions because of this is annyoning.
Adding this to getSelectedRows() fixes it finally:

    if (getTrackModel() == nullptr) {
        return {};
    }

I'll post a followup soon.

¹ this alone is weird, even considering the configured callbacks.
Over the years I've been using many regular mappings into MIDI Through Port and with a lot of them signals were emitted (which causes mayn things to happen in the GUI) but none so far has triggered the Load controls 🤷‍♂️

[Swiftb0y]

IMO the controller mapping instantiation is already happening way too early. Lots of controller mappings already work around data races on startup by delaying their serato status sysex in the init... Since controller mappings expect all CO's to be available the instant they're initialized, the only realistic solution is to create them at the very end.

[ronso0]

I agree (but I won't work on it)
Anyway (and especially until that has been implemented), it definitely doesn't hurt to verify we have a track model.

Followup is #13623