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

Remove TrackId from Cue #3016

Merged
merged 1 commit into from
Aug 18, 2020
Merged

Remove TrackId from Cue #3016

merged 1 commit into from
Aug 18, 2020

Conversation

xeruf
Copy link
Contributor

@xeruf xeruf commented Aug 14, 2020

Just a quickie I found while talking with Jan, trying to understand db sync :)

The Cue shouldn't know it's owner, the Track, and it doesn't even need to - voila 🎉

I am kinda wondering whether we need the unsetId/resetId method at all, since the Track is about to be deleted anyways when this is called. But I don't know enough about the TrackRecord lifecycle for that.

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.

Thanks for fixing this low-hanging fruit that got overlooked.

Nevertheless please avoid modifying unrelated code.

src/track/track.h Outdated Show resolved Hide resolved
@xeruf
Copy link
Contributor Author

xeruf commented Aug 17, 2020

Sorry, old habits. I am used to cleaning up stuff along the way, mostly worked on personal projects previously :)

@xeruf xeruf force-pushed the cue-trackid branch 2 times, most recently from f5179a9 to b3f7fa1 Compare August 17, 2020 19:08
@xeruf
Copy link
Contributor Author

xeruf commented Aug 17, 2020

@Holzhaus @uklotzde ready :)

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.

CI builds are still failing.

src/library/dao/cuedao.cpp Show resolved Hide resolved
@xeruf xeruf force-pushed the cue-trackid branch 4 times, most recently from 2b1cbd7 to b3f7fa1 Compare August 18, 2020 15:03
@xeruf
Copy link
Contributor Author

xeruf commented Aug 18, 2020

Sorry, I didn't trust them lately as I have seen too many CI failures ^^

And sorry for all the force-pushing, that was somewhat accidental...

Should be all fine now.

@uklotzde
Copy link
Contributor

You could have noticed the build failures locally by enabling -DWARNINGS_FATAL=ON that was introduced recently by @Holzhaus. Highly recommended!

The final changeset is clean and matches the goal. Please avoid changing unrelated code in the future.

Thank you for fixing this legacy issue. Sometimes a different viewpoint is needed to spot obvious design flaws.

LGTM

@uklotzde uklotzde merged commit a90062c into mixxxdj:master Aug 18, 2020
@xeruf xeruf deleted the cue-trackid branch August 19, 2020 08:35
@xeruf xeruf mentioned this pull request Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants