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

Lastgenre: Fix track-level handling, multi-genre keep, force behaviour, logging #4982

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Lastgenre: Fix track-level handling, multi-genre keep, force behaviour, logging #4982

wants to merge 13 commits into from
+124 −25

Conversation

JOJ0
Copy link
Member

@JOJ0 JOJ0 commented Oct 29, 2023 •
edited
Loading

Description

Edit 2023-09: The original idea of this PR was:

Several fixes I had in the queue for months. Some of it required fixes in the library code which are through by now.

  • Fix the force option: Don't always overwrite comma-separated multi-genres, compile a list and keep what's in the whitelist.
  • Fix lastgenre -A in combination with config option source: track - Tracks receive the album's genre even when this option is set
    • When an album-level genre is set already, single tracks should don't fall back to receiving the album's genre.
  • Adjust log-level and message when lastgenre handles tracks to look similar to when handling albums.

Edit 2023-09: Additional option keep_allowed

During review and discussions it turned out that besides the existing force option a second option would be required to really achieve a typical (expected) behaviour of a force option. This is what we came up with (copied over and slightly edited from #4982 (comment)):

Two config options, force and keep_allowed, i.e. 4 possible settings:

Case 1

Overwrite all. Only fresh last.fm genres remain.

force: yes
keep_allowed: no

Case 2

Add new last.fm genres when empty. Present tags stay untouched.

force: no
keep_allowed: no

Case 3

Add new last.fm genres. Keep whitelisted genres in present tags.

force: yes
keep_allowed: yes

Case 4 (default)

Add new last.fm genres when empty. Keep whitelisted genres in present tags.

force: no
keep_allowed: yes

To Do

  • Documentation of new option and new default force behaviour.
  • Documentation: Clarify that -a is implicit
  • Changelog.
  • Fix existing tests.
  • Implement Case 1
  • Implement Case 2
  • Implement Case 3
  • Implement Case 4

@JOJ0 JOJ0 requested a review from sampsyo November 2, 2023 16:27
@JOJ0 JOJ0 marked this pull request as ready for review November 2, 2023 16:27
@JOJ0
Copy link
Member Author

JOJ0 commented Nov 2, 2023 •
edited
Loading

I'd request a review from you @sampsyo since I think you initially created it. Also @rain0r would be good since 5 years ago they added the -A option. Hi @rain0r , you wanna take a look? :-)

In short: I think I fixed the plugin to now really reflect what's documented. Any nitpicking in my code or functionality-wise is appreciated.

One question already. Here we do not state that a -a/--album option exists: https://beets.readthedocs.io/en/latest/plugins/lastgenre.html#running-manually

When I started out with using this plugin I was confused a verry long time about this option. As far as I understand it now: It doesn't do anything since it is default. So why keep it? Or is having a -a option that is the default anyway a common thing in beets? I know we have a lot of -a commands which streamlines usablity, and that is a very good thing! Usuall they change behaviour to not do something with items but with albums. I'm just not sure about this one....do we have such a pattern anywhere else? So, just leave it? Should I add some words to the docs?

I think the both of you decided these options should look like that around here: #3220 (comment)

JOJ0 added a commit to JOJ0/beets that referenced this pull request Nov 2, 2023
@JOJ0
Add changelog for beetbox#4982
ceed54a
@sampsyo
Copy link
Member

sampsyo commented Nov 3, 2023

Thanks for the extra context, @JOJ0!

About the existence of -a (the default mode) specifically: it's not too uncommon… for example, the beet import command has several flags that are opposites of each other, one of which is the default. Of course, it's important in that case because the default mode can be set in the config, so the user needs a way to override the default in either direction. That's not the case here, so maybe it at least makes sense to add "(default)" to the -a option's help string, or to remove it altogether?

sampsyo
sampsyo reviewed Nov 3, 2023
View reviewed changes
Copy link
Member

@sampsyo sampsyo 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 the ping!! Here are a couple of straightforward comments.

beetsplug/lastgenre/__init__.py Outdated Show resolved Hide resolved
beetsplug/lastgenre/__init__.py Outdated Show resolved Hide resolved
beetsplug/lastgenre/__init__.py Outdated Show resolved Hide resolved
@JOJ0 JOJ0 marked this pull request as draft November 8, 2023 08:08
@JOJ0 JOJ0 force-pushed the lastgenre_fixes branch 2 times, most recently from 1e81209 to 89ae925 Compare November 16, 2023 12:33
JOJ0 added a commit to JOJ0/beets that referenced this pull request Nov 16, 2023
@JOJ0
Add changelog for beetbox#4982
c8966e5
@JOJ0
Copy link
Member Author

JOJ0 commented Nov 17, 2023 •
edited
Loading

I'd like to pull out this conversation #4982 (comment) into a new thread, to make it more obvious for others as well. I think it could be a broader discussion of where this plugin should go. Basically we were talking about the current force: no behaviour being weird as well as the new behaviour I am initially proposing with this PR. I gave all this some thought and came up with this idea. Let's discuss it:

So from my point of view, the main problem with the current behaviour when force is disabled, is that it's not really what a user would typically expect. So what could we do to make force: no more predictable?

The following idea would require a new config setting as well as a whole new branch of behaviour (Case 3):

Case 1

force: yes
overwrite all, only fresh last.fm genres remain

Case 2

force: no

keep any string in present genre tag, only write last.fm genres when empty

Case 3

force: yes
keep_allowed: yes

keep present genres when whitelisted and add new last.fm genres (this is a new branch of behaviour and needs to be coded, I think there is open feature requests for it. Update: Something was feature-requested, but it might not be exactly as I'm proposing here: #4750)

Case 4

force: no
keep_allowed: yes

cleanup only - keep present genres when whitelisted but don't add new last.fm genres; Only when genre is empty, add last.fm genres.

That last combination is weird though....but it's what I proposed for force:no before!

Which of these would now make sense to be the new default? The new force: no (Case 2) would be the least invasive IMO...

@sampsyo brainstorming request 🧐

@JOJ0 JOJ0 changed the title Lastgenre: fix track-level handling, fix multi-genre keep, streamline singleton log Lastgenre: Fix track-level handling, multi-genre keep, force behaviour, logging Nov 17, 2023
@JOJ0
Copy link
Member Author

JOJ0 commented Nov 17, 2023

Some more context / cross-linking:

The initial reason why I got my hands dirty with this plugin was when I realised that comma separated multi-genres where not recognized: #4751 (comment)

Here @arsaboo requests a feature that goes in direction of Case 3 above: #4750

@arsaboo
Copy link
Contributor

arsaboo commented Nov 17, 2023 •
edited
Loading

So, we have two config options - force and keep_allowed, i.e., 4 options in all. Given that, keep_allowed is no in cases 1 and 2. Thus, here's a slightly modified behavior in the 4 cases above:

Case 1: overwrite all, only fresh last.fm genres remain

force: yes
keep_allowed: no

Case 2: Since keep_allowed is no, we only write last.fm genres when empty. There may be incorrect genres in pre-existing tags even after this, as this option is not touching pre-existing tags

force: no
keep_allowed: no

Case 3: keep present genres when whitelisted and add new last.fm genres

force: yes
keep_allowed: yes

Case 4: keep any string in the present genre tag; only write last.fm genres when empty. This will not touch pre-existing genre tags.

force: no
keep_allowed: yes

Thus, Case 4 seems like the best default choice. It does not affect existing genre tags and updates the empty ones. Case 3, on the other hand, is the most useful one (at least for me).

@sampsyo
Copy link
Member

sampsyo commented Nov 17, 2023

This brainstorming honestly sounds great, y'all. It is indeed really weird that the force: no mode can still update old genres; keeping all nonempty genres seems like it should at least be an option. I feel less specific about what the default should be, but I like your idea about decoupling the two aspects of the behavior (when to override existing, nonempty data and what to do to old data) into two different options.

@JOJ0
Copy link
Member Author

JOJ0 commented Nov 18, 2023

Ähem I might be slow or too tired already. Which of those 4 cases are now different from my proposal @arsaboo ? Sorry I must have missed it! Help! :-)

@arsaboo
Copy link
Contributor

arsaboo commented Nov 18, 2023

Not different....just a little more explicit about the force and keep_allowed config options. I think we have an agreement about the options.

@JOJ0
Quickfix lastgenre always overwriting multi-genre
5953544
@JOJ0
Fix track-level genre handling in lastgenre plugin
cbc6e0f 
When `lastgenre.source: track` is configured,

- `lastgenre -a` _should not_ fall back to the album level genre (by
  making use of the with_album=False kwarg of the Libary's get method).
- `lastgenre -a`, when finally storing the genres of _an album_, should
  _not_ also write the tracks genres (by making use of the inherit=False
  kwarg of the Album's store method.
@JOJ0
Streamline lastgenre singleton log with album log
a4c2f83 
It was rather confusing that the lastgenre plugin, when handling
singletons, sometimes showed that it applied genres from last.fm and
sometimes didn't (it did only in debug log). This streamlines the
behaviour:

- Change debug to info log.
- Streamline wording.
- Display details about the track.
@JOJ0
Handle dups of existing genres in lastgenre plugin
86ab2a8 
When handling existing comma-separated genres in the _get_genre method
of the plugin, make sure duplicate genres are removed.
@JOJ0
Prevent album genre inherit only when source:track
ce52443 
is configured.
@JOJ0
Fix lastgenre source:track handling during imports
a2ea959
@JOJ0
Another round of lastgenre logging nitpicks
336b51e 
- Printing out album/item in default format could lead to unreadable
  clutter depending on the user's configured formats.
- The album's name and the individual tracks' title should be just
  sufficient to provide context as well readability.
- Log like this while importing as well as in standalone runs.
@JOJ0
Refactor lastgenre keep_allowed to list comprehension
d04ddd6
@JOJ0
Use separator as configured instead of hardcoding
2ea0591 
in lastgenre plugin.
@JOJ0
Use provided deduplicate function for keep_allowed
960c479 
generation, instead of a simple set(). This way we keep the original
order of genres.
@JOJ0
Add lastgenre keep_allowed options (-k/-K)
f38c27f 
- Default to False.
- During PR#4982 discussions we came to the conclusion that the
  following behaviour would be a good new default choice:
  - Keep whitelisted existing genres
  - Only Fetch last.fm genres for empty tags.
  - To get this we also have to change the default of the force
    option!!!
  - Resulting in "force: no" and "keep_allowed: yes"; see Case 4 in
    PR#4982 description
- Options are not put to use yet, just defined and defaults set!
@JOJ0
Docs for lastgenre keep_allowed/force
cc2e139 
Keep both options' "Configuration" chapter texts as compact as possible,
while linking to a new chapter that describes all 4 possible
combinations in detail.
@JOJ0
Implement --force and --keep-allowed behaviours
c12b26b 
- Retrieving, filtering and deduplicating present genres of Items/Albums
  via separate methods.
- Implement all four cases of behaviour as described in PR#4982
- Issues:
  - There is quite some unnecessary spliting of genres from strings into
    lists and the other way round happening throughout the plugin.
  - In the case where existing genres get "augmented" with last.fm
    genres, we might end up with _more_ genres than the configured
    limit.
@JOJ0 JOJ0 marked this pull request as ready for review September 17, 2024 16:38
@github-actions GitHub Actions
Copy link

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

@JOJ0 JOJ0 marked this pull request as draft September 17, 2024 16:39
@JOJ0
Copy link
Member Author

JOJ0 commented Sep 17, 2024 •
edited
Loading

Hi @arsaboo! I finally managed to find time to almost finish this PR. The general behaviour and docs of the new config options combinations are finished. If you want to, an "early" review would be super helpful. Since it probably also for you is a long time ago it might be interesting what you think if you read through the docs. Is it 100% clear what force/keep_allowed options do? Certainly but only if you have the time, some playing around and checking if it also really works that way would be great. Thanks a ton!

@arsaboo
Copy link
Contributor

arsaboo commented Sep 17, 2024

@JOJ0 this is AWESOME 🎉🎉

The docs look reasonably clear. I will play with this. The debug logs are great to see what is going on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sampsyo sampsyo sampsyo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JOJ0 @sampsyo @arsaboo