-
Notifications
You must be signed in to change notification settings - Fork 336
Album version #1398
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
base: public/9.1
Are you sure you want to change the base?
Album version #1398
Conversation
Signed-off-by: darrell-k <[email protected]>
Please provide a summary of the changes. I tried to understand that thread, but it's pretty long, with some back and forth etc. In particular the introduction of |
What Copilot thinks this does (which means the aforementioned This pull request adds support for an album "version" field in LMS (Logitech Media Server). Here’s a summary of the key changes: Database Schema Changes:
Backend (Perl) Changes:
Frontend/UI Changes:
Migration Scripts:
Overall: |
On the persistent id changes, I forgot there is an issue, which I have just updated with the bare bones of a specification, which needs more thought. The new column is to give the user somewhere to store their own id if they don't want to rely on MusicBrainz. There was a consensus that this was a good idea in the Forum thread. I don't think it does any harm adding it now. On the album version changes, it seems that Copilot can read ;) |
Sorry, meant to link the issue #1372 |
Please add a summary to the PR description. I do understand what Copilot found in the changes. But it doesn't describe the motivation behind the change. And I won't read >85 postings which go in all kinds of directions (eg. the misleading |
I've updated the PR description in the first comment above. |
Signed-off-by: darrell-k <[email protected]>
Just pushed a fix to this, I had broken the album matching when version IS NULL. Don't know how you want to proceed with this re the persistent id column: should we remove that from this PR (did you see mike's reply at #1372 ?) |
I'm sorry for being slow. I'm currently travelling and find it hard to do serious reviews on a small screen... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one question I have: why would we store the version in a separate field if we added it to the album title all the time anyway? I think it should rather be treated like the year, which can be shown or not, which is its own property where the user can customise the display etc.
Reasons mentioned in the forum thread:
See https://forums.lyrion.org/forum/user-forums/logitech-media-server/1765250-extending-lyrion-s-use-of-tag-metadata-to-enhance-to-browse-interact-with-your-music?q=version for the discussion on these things, with the filter in the URL it's only a couple of pages of comments. |
PS. I can't think of a scenario where you have multiple versions of the same album but don't want to show the version text. |
Ok, my initial reading that the version would included in the album title everywhere was wrong. The raw query would return it as an additional field. Shall we bet on the request to have this field be displayed optionally? 😁 As for the schema change: I'd prefer to keep things apart, even if it means we might have yet another full rescan. Otherwise we end up with columns no-one remembers what they were supposed to be. Like eg. the |
I'm sorry, I just created a conflict with this PR... you'll have to rename the schema files to 27. And I must go back to the forum to warn people about the commit I pushed earlier today. Didn't think much, stupid me. |
Signed-off-by: darrell-k <[email protected]>
Signed-off-by: darrell-k <[email protected]>
Signed-off-by: darrell-k <[email protected]>
Signed-off-by: darrell-k <[email protected]>
Coming back to this, I've removed the tracks table change (lms_persistent_id) and resolved the conflict. I'll test again, since it was so long ago. |
New column albums.version and associated processing (see thread at https://forums.lyrion.org/forum/user-forums/logitech-media-server/1765250-extending-lyrion-s-use-of-tag-metadata-to-enhance-to-browse-interact-with-your-music#post1769954 )
The reason for this change is to allow users to (optionally) use an additional tag "VERSION" to distinguish different versions of the same album. This will enable future UI enhancements, for example to group together different versions of the same album. It should also make metadata matching simpler as albums.title can contain only the actual album name.
I've also made the database changes for the forthcoming enhancements to the Record Label PR, and changes to persistent track ID (see same thread above for discussion on that), to prevent forced rescans when these are done.