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

show preferences when configured skin failed to load #3845

Closed
wants to merge 1 commit into from

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented May 8, 2021

WIP
Addressing a 9 year old TODO
Actually this should happen only with a broken skin from the user directory.
Though, in that case the user has no chance to get back to a working state from within the GUI.
(only by editing mixxx.cfg)

Candidate for 2.3, though when no one wants to squeeze that in right now it can wait for 2.3.1 We got along without it until now.
rebase after #3839 is merged

@github-actions github-actions bot added the ui label May 8, 2021
@Holzhaus
Copy link
Member

Holzhaus commented May 8, 2021

Alternative: We could switch back to the default skin instead of opening preferences. This saves some work for the user.

@Swiftb0y
Copy link
Member

Swiftb0y commented May 8, 2021

We'd have to inform the user of that behavior with a popup or similar, otherwise they might think its a bug.

@Swiftb0y
Copy link
Member

Swiftb0y commented May 8, 2021

The ideal would probably to have a similar dialog like we have for misconfigured audio settings. So they have the choice between "Exit", "Reconfigure" or "Continue with Default".

@ronso0
Copy link
Member Author

ronso0 commented May 9, 2021

...which requires a more elaborate dialog instead of the standard Ok / Cancel // Yes / No
https://github.com/mixxxdj/mixxx/pull/3212/files#diff-848ff12a6bbb8c39f28e2a6be33ca2bbbe31f53e6ba550b2f6df7b8bdfaecf1fR1666

(this fix is adopted from the unconfigured Mic/Passthrough situation)
Considering the TODO has been sitting there for 9 years we may merge this quick fix now to ease this situation for a few newbie skin developers -- and polish the UX in a follow-up (2.3.1). Or --regardless of 2.3-- merge when dialog UX is perfect.

I don't mind. I could deal with it for years. Actually this fix is only for convenience, it neither affects skin developers (as they know what files they were messing with), nor users downloading modded skins (those skins should work to a degree, at least skin.xml should -which is the only file to be corrupt to make skinloader fail).

@Swiftb0y
Copy link
Member

Swiftb0y commented May 9, 2021

Yes we can with the current feature otherwise this PR will just suffer from feature creep again.

@ronso0
Copy link
Member Author

ronso0 commented May 14, 2021

btw another place where we should trigger the same popup
https://github.com/mixxxdj/mixxx/blob/2.3/src/mixxx.cpp#L1553-L1568
to reduce duplication we should probably refactor rebootMixxxView() to be used in initialize()

@ronso0
Copy link
Member Author

ronso0 commented May 26, 2021

This is fixed in another way by #3891

@ronso0 ronso0 closed this May 26, 2021
@ronso0 ronso0 deleted the skin-config-after-fail branch May 26, 2021 21:57
@Holzhaus
Copy link
Member

Holzhaus commented May 26, 2021

This is fixed in another way by #3891

Sorry, I somehow forgot about this PR. Didn't want to make your work obsolete.

@ronso0
Copy link
Member Author

ronso0 commented May 26, 2021

don't worry. Currently I have neither the motivation nor the time to get this small fix into a mergable state.
it was just a drive-by fix anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants