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

Refactor: Extract the CountsDuration table logic into PlaylistStatsDAO #13177

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cr7pt0gr4ph7
Copy link
Contributor

SetlogFeature and PlaylistFeature (as well as BasePlaylistFeature) contain nearly identical code for creating a summary table of the track number and total duration of all playlists (both history playlists and normal playlists).

This PR unifies the code into a single PlaylistStatsDAO class, which has the added advantage of moving DB-layer code from the *Feature classes to a more appropriate location.

@cr7pt0gr4ph7 cr7pt0gr4ph7 changed the title Refactor: PlaylistStatsDAO to Refactor: Extract the CountsDuration table logic into PlaylistStatsDAO Apr 28, 2024
@ronso0
Copy link
Member

ronso0 commented Apr 28, 2024

Thank you for this PR!

Please note that there is already #4846 which also does a playlist backend refactoring.
I'd recommend to first check that PR and wait how we proceed with that before investing more time here.

@cr7pt0gr4ph7
Copy link
Contributor Author

@ronso0: Thanks for the heads up! I am not especially partial to the code in this PR, it was a byproduct of understanding the playlist duration calculation logic while working on #13183 (which, in the end, was implemented in a wholly different way instead), and none of my other code depends on it.

So it's your call whether to merge this PR, or close it and wait for #4846 to finish - I am fine either way.

@cr7pt0gr4ph7
Copy link
Contributor Author

It seems that #4846 was last updated in 2022, so I don't know whether to expect any progress on that.

My new PR #13183 calculates the total track time of the Auto DJ playlist (like #4846) but also takes intro/outro cues, transition times etc. fully into account by reusing the calculation logic that is used for performing the actual transitions,
so if merged, it would fully supersede #4846.

This PR is independent of that and is just a "nice to have" from a code quality perspective, so I'll leave it open for now. Feel free to close it in the future if it isn't needed anymore.

@ninomp
Copy link
Contributor

ninomp commented May 24, 2024

Hi! Author of #4846 here. I'm currently struggling to find time to finish #4846, so feel free to go ahead with your work. When your PR gets merged, I'll close mine.

Also, If you are interested, I can review your PR(s) and give you my comments/ideas.

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

3 participants