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

DlgPrefInterface: Fix crash if no skin is available #3918

Merged
merged 1 commit into from
May 27, 2021

Conversation

Holzhaus
Copy link
Member

Only happens when neither the configured not the default skin is available (so that mixxx needs to shut down anyway), but should be fixed nonetheless.

@Holzhaus Holzhaus added this to the 2.4.0 milestone May 27, 2021
@github-actions github-actions bot added the ui label May 27, 2021
@ronso0
Copy link
Member

ronso0 commented May 27, 2021

is just DlgPrefInterface not built or do we shut down with a warning?
indeed it makes no sense to start mixxx when skins are missing, not even to adjust other settings.

@Holzhaus
Copy link
Member Author

Holzhaus commented May 27, 2021

As before the refactoring, SkinLoader calls reportCriticalErrorAndQuit() when no skins are found. However, this actually just queues shutdown. The construction of the whole MixxxApplication continues as normal. Only after everything is constructed and we're about to start the event loop, the "queue" is checked and we're actually shutting down.

So yes, we actually just abort the preferences construction, but it should work. I could also add pointer checks to the preferences implementation everywhere, but I doubt this would be worth it.

Instead, we should fix our skin loading logic in mixxx.cpp, because it we're detecting skins multiple times (first time for Mixxx itself, second time for the preferences). This leads to repeated error messages if the configured skin is missing. We should exit early if there is no valid skin missing, and the SkinLoader should remember the currently loaded skin. Then we can avoid executing the fallback logic in the preferences again, because we already know if the configured skin is working or not.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

That works good. LGTM
Thank you.

@daschuer daschuer merged commit 37bf204 into mixxxdj:main May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants