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

Draft
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 force-pushed the refactor-add-playliststatsdao branch from 70bb549 to fe863bc Compare April 27, 2024 19:37
@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.

@cr7pt0gr4ph7
Copy link
Contributor Author

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.

Ok, then @ronso0 or someone else can merge this PR whenever they're ready.

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

Thank you, very much appreciated! I've got a few more changes lined up, including quite a big one related to keyboard navigation in the track info dialog (not yet ready, though). I'd appreciate a second pair of eyes on that, I'll ping you when it's ready.

Copy link

github-actions bot commented Sep 7, 2024

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Sep 7, 2024
@daschuer
Copy link
Member

daschuer commented Sep 9, 2024

Unfortunately a conflict has developed.

@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Sep 10, 2024
@cr7pt0gr4ph7 cr7pt0gr4ph7 marked this pull request as draft October 18, 2024 08:23
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build library stale Stale issues that haven't been updated for a long time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants