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

Fix for all folders using aunique when a non-standard key is used #4559

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Serene-Arc
Copy link
Contributor

Description

This is a tentative quick and dirty fix for an error that I've come across. When attempting to add the key composer to the keys of the aunique function in the configuration file, a strange error occurs: every folder and file will trigger the aunique function (so that the aunique string is included in the folder name). This happens even when there are no conceivable duplicates, or even any other items in the library at all.

Going through the code, this seems to be because, when searching for the composer field, the album object at that point returns no value for the composer field. It does before hand but that information is dropped along the way inside the wrapper_func function. Because of that, the final key field used to create the query at this line is empty, and so beets thinks the album needs disambiguation because the length of ambiguous_items is 0, instead of 1.

So, my fix is just to include 0 and negative numbers in that if statement. It's a quick and dirty fix and works but I think the better solution would be to find out why the other fields for the album information have been dropped and to fix that. Any hints for where to go or help would be much appreciated!

To Do

  • Documentation. (If you've add a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst near the top of the document.)
  • Tests. (Encouraged but not strictly required.)

@stale
Copy link

stale bot commented Mar 25, 2023

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 25, 2023
@sampsyo sampsyo removed the stale label Mar 25, 2023
@Serene-Arc Serene-Arc force-pushed the disambiguator_key_error branch from 6aa50db to fc57604 Compare June 14, 2023 00:21
@Serene-Arc
Copy link
Contributor Author

So I have spent the last couple of hours going through the code, trying to figure out what was happening and why. I figured it out and my findings are below.

The aunique function is called because the composer field is empty, meaning that this line is not triggered, because there are 0 returned albums. The composer field is empty because composer is not a real field for an album, and is not returned from here. This means that there are no albums that can possibly fit the defined criteria, and it returns 0 items.

Now, this might be expected, except that the path specifications do allow for the composer field to be part of the path, even in the 'album section', because the path doesn't actually distinguish what parts go where. The use case for this came from the beets-audible plugin, which sets the composer to the same one for all tracks. This meant that when I was trying it out, I didn't see the different tracks (if they had different composers) going into different folders. To me, all of the files for a particular audiobook where being labelled with the narrator and it worked out fine, but this isn't a universal case.

Ultimately I don't think this is a bug per se, at least in the way that I thought it was. beets does, however, fail silently if the user attempts to use an album disambiguation key that is not an actual album key. In this case, that triggers the aunique numbering for every single album, regardless of whether or not it's deserved, because the search fields are looking for something that no album can have.

I'm not really sure on the way forwards. I'll be submitting changes to the beets-audible plugin to fix this issue, since it's on that side of things, but here I see two options:

  • Tell the user and fail the import process if there's a key is track specific, not album specific
  • Remove track-specific keys from the search process so it works as best as possible and tell the user they've tried to use the wrong key

@stale
Copy link

stale bot commented Oct 15, 2023

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 15, 2023
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.

2 participants