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.

Sign up for GitHub

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

Jump to bottom

Track metadata: Replace TrackRecord and beat grid at once #3870

Merged
merged 12 commits into from
May 24, 2021
Merged

Track metadata: Replace TrackRecord and beat grid at once #3870

merged 12 commits into from
May 24, 2021

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented May 17, 2021 •
edited
Loading

The last refactoring PR before fixing the state management in DlgTrackInfo. It is now possible to replace all track properties encapsulated in TrackRecord and the beat grid at once.

@uklotzde uklotzde added this to the 2.4.0 milestone May 17, 2021
@uklotzde uklotzde changed the title Track metadata: Replace TrackRecord once Track metadata: Replace TrackRecord at once May 17, 2021
@Holzhaus
Copy link
Member

Holzhaus commented May 17, 2021 •
edited
Loading

Just wanted to say that I appreciate that you went the extra step and split changes from #2282 and #2656 into bite-sized chunks, so that we don't end up with a huge, unreviewable PR that stays unmerged forever 👍

@uklotzde
Copy link
Contributor Author

Just wanted to say that I appreciate that you went the extra step and split changes from #2282 and #2656 into bite-sized chunks, so that we don't end up with a huge, unreviewable PR that stays unmerged forever +1

I was even concerned that opening so many small PRs might be distracting. On the other hand it helps to focus on a single aspect and allows to review the PR if you are not familiar with the code.

While reviewing some smaller PRs myself recently I noticed how much more rewarding this is. Both for the submitter and for the reviewer.

@uklotzde
Copy link
Contributor Author

uklotzde commented May 17, 2021 •
edited
Loading

Wait, maybe there is some redundancy now. I have to check again. There is some redundancy between replaceRecord() and replaceMetadataFromSource(), but I don't see any chance to eliminate it.

@uklotzde uklotzde changed the title Track metadata: Replace TrackRecord at once [WiP] Track metadata: Replace TrackRecord at once May 17, 2021
@uklotzde uklotzde marked this pull request as draft May 17, 2021 21:32
@uklotzde uklotzde changed the title [WiP] Track metadata: Replace TrackRecord at once Track metadata: Replace TrackRecord at once May 17, 2021
@uklotzde uklotzde marked this pull request as ready for review May 17, 2021 21:44
@uklotzde uklotzde changed the title Track metadata: Replace TrackRecord at once Track metadata: Replace TrackRecord and beat grid at once May 18, 2021
Holzhaus
Holzhaus reviewed May 20, 2021
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks. Code looks good and seems to work fine.

One issue I noticed: If I import a new track that has a Serato beatgrid, i can see the beatgrid, but it's overwritten as soon as the analysis finished. There's no way to use the Serato beatgrid without resetting the Beatgrid afterwards and then reimporting the tags. I think that's not related to this branch, but I need to double check. Maybe it even affects 2.3.

@daschuer
Copy link
Member

We have a preferences option for this.
By default it should not reanalyze. Maybe there is a bug if it is done anyway.

@uklotzde
Copy link
Contributor Author

uklotzde commented May 20, 2021 •
edited
Loading

Behavior of the existing import from source didn't change and the new function is not used yet. It will be used soon by more complex refactoring of DlgTrackInfo which fixes the workflow to allow dependent/derived fields.

Related: https://bugs.launchpad.net/mixxx/+bug/1921646

The import/export and synchronization of Serato beat grids is tricky and currently not really usable.

@Holzhaus
Copy link
Member

The bug regarding Serato BeatGrids is unrelated and fixed in #3885.

@daschuer Do you want another look at this PR? Otherwise LGTM.

@uklotzde
Copy link
Contributor Author

Found and fixed a bug that only affects main while testing https://bugs.launchpad.net/mixxx/+bug/1929311

@uklotzde
Copy link
Contributor Author

Ping. I would like to move on now. This is blocking a follow-up PR.

daschuer
daschuer requested changes May 24, 2021
View reviewed changes
src/track/track.cpp Show resolved Hide resolved
src/track/trackrecord.cpp Outdated Show resolved Hide resolved
src/track/trackrecord.cpp Outdated Show resolved Hide resolved
daschuer
daschuer approved these changes May 24, 2021
View reviewed changes
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.

Thank you.

@daschuer
Copy link
Member

Waiting for CI

@daschuer daschuer merged commit c3203cc into mixxxdj:main May 24, 2021
@uklotzde uklotzde deleted the trackmetadata branch May 24, 2021 22:36
@mixxxbot mixxxbot mentioned this pull request Aug 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Holzhaus Holzhaus Holzhaus left review comments

@daschuer daschuer daschuer approved these changes

Assignees
No one assigned
Labels
code quality library minor bug regression
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@uklotzde @Holzhaus @daschuer