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

Preferences: Remove all references to MixxxMainWindow #4109

Merged
merged 6 commits into from Jul 21, 2021

Conversation

Holzhaus
Copy link
Member

This is necessary if we want to use the preferences in QML without creating a QWidgets-based MixxxMainWindow instance.

We can't have any references to the `MixxxMainWindow` class in
`DlgPrefInterface` if we want to use the preferences for QML skins that
don't have a `MixxxMainWindow` instance. The using the primary screen is
a sensible fallback.
Instead of letting the `DlgPrefInterface` class hold a reference to the
`MixxxMainWindow` and call its `setToolTipsCfg` method directly, we just
update the config object and emit a signal that the main window can
connect to.
Instead of calling `rebootMixxxView` directly, we can just emit a
signal. This allows removing any references to the `MixxxMainWindow` in
`DlgPreferences`.
This is similar to the previous commit, it basically removes the
necessity to hold a pointer to the main window.
@Holzhaus Holzhaus added this to In progress in QML GUI via automation Jul 16, 2021
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1037258891

  • 0 of 33 (0.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 28.627%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/util/screensavermanager.cpp 0 33 0.0%
Files with Coverage Reduction New Missed Lines %
src/engine/readaheadmanager.cpp 2 88.79%
Totals Coverage Status
Change from base Build 1033452289: -0.02%
Covered Lines: 20097
Relevant Lines: 70203

💛 - Coveralls

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Oh, we didn't notice that we pulled in a massive amount of dependencies for just a single aspect? Great that you found this low hanging fruit!

I am currently not able to test it. But I would approve the refactoring as such if we didn't miss anything.

@uklotzde
Copy link
Contributor

I didn't experience any issues so far. Thank you! LGTM

@uklotzde uklotzde merged commit 9e357a4 into mixxxdj:main Jul 21, 2021
QML GUI automation moved this from In progress to Done Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants