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

privat generated ui headers #12060

Merged
merged 5 commits into from Nov 1, 2023
Merged

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Oct 4, 2023

This makes the ui_dlgpreferencesdlg.h private by moving the instantiation of the DlgPreferences to CoreServices.

This fixes one issue from #11407

@daschuer
Copy link
Member Author

daschuer commented Oct 5, 2023

@acolombier Does this work for you?
Not sure why the code coverage drops. I think we can ignore that.

@acolombier
Copy link
Contributor

I'm currently away from the computer but I will test it as soon as I get back to it. Alternatively, I have managed to also remove that work around by using the forward declaration in case this change would have repercussions on other things.

@acolombier
Copy link
Contributor

acolombier commented Oct 14, 2023

It looks like that did the trick, thanks!

Edit: Actually, I'm unable to build anymore, but I don't think it is related - I did rebase main before taking your change in and it looks like the precompiled header have broken my CMake changes Building all fine now

@@ -5,6 +5,7 @@
#include "control/controlsortfiltermodel.h"
#include "controllers/controllermanager.h"
#include "moc_qmlapplication.cpp"
#include "preferences/dialog/dlgpreferences.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

This line seems to cause issue since it means that the QML library (in my feature, I have splitted mixxx-lib in two) will also need preferences/dialog/ui_dlgpreferencesdlg.h. I belive however that I could take your change in, plus the one I've made to initialize the QML signletons as part of the CoreService instead of as part of the QML application, which then removes the need for DlgPreferences in this file

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this line can actually be removed.

@daschuer
Copy link
Member Author

Ready for merge, the Coveralls failure seems to be unrelated.

src/coreservices.cpp Outdated Show resolved Hide resolved
@daschuer
Copy link
Member Author

Done

@JoergAtGithub
Copy link
Member

Code LGTM! All review comments are addressed and I tested it as part of #12139 on Win11. Thank you!

@JoergAtGithub JoergAtGithub merged commit 8a8b162 into mixxxdj:main Nov 1, 2023
11 of 13 checks passed
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.

None yet

4 participants