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

avoid issue with stars_up/_down ControlObjects #12591

Merged
merged 1 commit into from Jan 26, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Jan 19, 2024

Supposed to fix one half of #12560

Moved the COs from WStarRating to BaseTrackPlayerImpl, and the skin parser now connects this extra signal.
The reason for #12560 is not clear to me, but this is safer because the only responsibility of the widgets is now to listen to change request signals.

The first commit is picked from #12582

edit @Swiftb0y mentioned a potential cause https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/help.20with.20unique_ptr.20.2F.20ControlObjects.20not.20being.20destroyed

@ronso0 ronso0 changed the title Stars co crash workaround workaround for stars_up/_down issue Jan 19, 2024
@ronso0 ronso0 changed the title workaround for stars_up/_down issue avoid issue with stars_up/_down ControlObjects Jan 25, 2024
…skin reload

If owned by WStarRating, and controls have actually been used, and the skin is destroyed,
it seems that only the ControlObjects are destroyed but not the underlying ControlDoublePrivates.
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.

This looks good. What is the reason not always set up the stars_up/down controls in the BaseTrackPlayerImpl() constructor? This way it is always available for controllers and keyboards.

@ronso0
Copy link
Member Author

ronso0 commented Jan 26, 2024

You mean for all players, not just decks?
These controls are useful if there's a star widget that gives visual feedback. No widget, no controls needed. That was my intention.

@daschuer
Copy link
Member

You mean for all players, not just decks?

That was my first idea actually but I am unsure. We can move alternative to the Deck() constructor.

The other aspect is, that we normally instantiate the COs without GUI. Here the skin takes control which creates a asymmetry compared to the "track_color" CO for instance.

@ronso0
Copy link
Member Author

ronso0 commented Jan 26, 2024

Verifying the track color is easier.
Otoh, setting the rating currently requires a StarRating instance (in WStarRating) to verify values. An instance only because only that knows the current max rating, though this flexibility is actually not required since WStarRating always sets it to 5.

So, yeah, the controls could be created unconditionally but for that to be useful some of the rating logic has to moved to BaseTrackPlayer.

Tbh I'm not really up to that right now, but I'll definitey look into it after this was merged.

@daschuer
Copy link
Member

Ah OK that the issue, the star logic is anyway in the widget. Yes we need to change that on the long run to be prepared for QML.
But for now its good enough. Thank you.

@daschuer daschuer merged commit 0299924 into mixxxdj:2.4 Jan 26, 2024
12 of 13 checks passed
@ronso0 ronso0 deleted the stars-co-crash-workaround branch January 26, 2024 23:16
@ronso0
Copy link
Member Author

ronso0 commented Jan 26, 2024

Okay, thanks for reviewing.

@ronso0
Copy link
Member Author

ronso0 commented Jan 27, 2024

I took a look and it was less work than expected. #12648

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.

None yet

2 participants