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

SkinLoader: Skip skin directory if "skin.xml" is missing #3933

Merged
merged 1 commit into from Jun 1, 2021

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Jun 1, 2021

This is a small fix that is useful on it's own, but also improves
forward compatibility with the new QML skin system.

If you work on a QML skin and have it configured in the settings, then
switch to a 2.3-based branch, the skin obviously won't load. But if the
skin directory exists (e.g. because you have unstages files in it),
Mixxx will refuse to start at all instead of falling back to the default
skin. The only way to fix this is editing mixxx.cfg by hand or
removing the directory.

This fix adds an additional check if the skin.xml exists, and skips
the skin if not. That allows Mixxx to fall back to the default skin.

This is a small fix that is useful on it's own, but also improves
forward compatibility with the new QML skin system.

If you work on a QML skin and have it configured in the settings, then
switch to a 2.3-based branch, the skin obviously won't load. But if the
skin directory exists (e.g. because you have unstages files in it),
Mixxx will refuse to start at all instead of falling back to the default
skin. The only way to fix this is editing `mixxx.cfg` by hand or
removing the directory.

This fix adds an additional check if the `skin.xml` exists, and skips
the skin if not. That allows Mixxx to fall back to the default skin.
@Holzhaus Holzhaus added this to the 2.3.0 milestone Jun 1, 2021
@Holzhaus Holzhaus added this to In progress in 2.3 release via automation Jun 1, 2021
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

@ronso0 what do you think?

2.3 release automation moved this from In progress to Needs Review Jun 1, 2021
@ronso0
Copy link
Member

ronso0 commented Jun 1, 2021

nice. works and LGTM!

@ronso0 ronso0 merged commit 7319513 into mixxxdj:2.3 Jun 1, 2021
2.3 release automation moved this from Needs Review to Done Jun 1, 2021
@uklotzde
Copy link
Contributor

uklotzde commented Jun 1, 2021

@Holzhaus Some conflicts

@Holzhaus
Copy link
Member Author

Holzhaus commented Jun 1, 2021

@Holzhaus Some conflicts

Just discard all changes from this fix, this was already fixed on main. But I can take of the merge, too.

@Holzhaus
Copy link
Member Author

Holzhaus commented Jun 1, 2021

@Holzhaus Some conflicts

Just discard all changes from this fix, this was already fixed on main. But I can take of the merge, too.

Ah, the other issues are from the preview rescaling PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2.3 release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants