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

Add base implementation of attachments #591

Closed
wants to merge 22 commits into from
Closed

Conversation

geigerzaehler
Copy link

This is a proposal on how to implement attachments. For a written explanation see the wiki.

The goal is to cover #111, #559 and the use cases on the wiki.

The PR also includes a coverart plugin. It implements the use case for cover art on top of the attachment framework and serves as a guide on how to use the framework.

Although the basic interfaces are all there, there is still a lot of work to do. Incomplete implementations should all be marked by TODO or FIXME tags. In addition we have to do the following.

  • Write tests. 100% coverage and integration tests for the user interface would be awesome.
  • Extend the list command to show attachments.
  • Dispatch attachment commands from SubcommandOptionParser. This includes adapting the format_help() method to the ArgumentParser interface.

@sampsyo
Copy link
Member

sampsyo commented Mar 8, 2014

Very cool! Thanks for taking the first steps and demonstrating your intentions with code. I have a couple of questions about the direction:

  • I had been thinking we'd implement attachments on top of flexible attributes rather than with a separate SQLite table. We would tag the attachment's type either in the key (attachment-cuesheet) or in the value (cuesheet:/path/to/file.cue). Of course, this would depend on multi-valued flexible attributes. I can see this approach being just as good, but I just wanted to bring this other option up in case you hadn't considered it.
  • Any particular reason you're storing URLs in the database instead of paths? Paths would be symmetric with the Item and Album tables.

@geigerzaehler
Copy link
Author

We would tag the attachment's type either in the key (attachment-cuesheet) or in the value (cuesheet:/path/to/file.cue)

For the coverart use case we need additional information like a description or the cover art type (front, back, artist, etc.). It would be very complicated to devise a custom serialization mechanism just to put it into one column.

Of course, this would depend on multi-valued flexible attributes.

This is not available yet and I guess pretty hard to implement. Using a table is a lot less painful.

Any particular reason you're storing URLs in the database instead of paths? Paths would be symmetric with the Item and Album tables.

URLs are a superset of paths, so we don’t loose anything. In the future we might do fancy things with attachment URLs. For example point to rap genius, wikipedia and stuff like that. See also the wiki.

@sampsyo
Copy link
Member

sampsyo commented Mar 9, 2014

Using a table is a lot less painful.

Indeed; this all makes sense! 👍 😃

URLs are a superset of paths, so we don’t loose anything. [...]

Aha; sorry, I was a little behind in reading the wiki page updates.

That's true, but it's also important to consider the complexity cost of using URLs. From my perspective, the essence of the attachment functionality is the ability to track files in the filesystem—move them along with the associated music, name them according to path formats, delete them when the music is deleted, etc. If we use full URLs for attachments, we'll pay the price in if is_file(url): cases throughout the code and the additional UI complexity of explaining the generality to users.

For many of the examples I can think of where URLs would be useful, current flexible attributes would work just fine. Lyrics from Rap Genius? Use a rapgenius field. Wikipedia link? Similarly, just put the URL in album.wiki. Multiple values might be helpful in some cases, but those are coming (eventually) to attributes also.

If we get down to basics, I think the principal between an attachment and a normal flexible attribute is the file-like handling. Since this doesn't apply to URLs, I'm not sure we gain much from the added complexity.

@geigerzaehler
Copy link
Author

I’ve given this some thought and it all boils down to functionality/complexity trade off. I’ll try to argue that the additional functionality is worthwile and we can reduce complexity to a minimum.

Why do we need URLs?

Beets already has flexible attributes to attach scalar data to items. But for simple file attachments we need more—we have to attach vectors to items. When I pondered the coverart plugin I realised that this is still insufficient, because coverart needs to store the cover type (“front”, “back”, etc.) So the next step is to attach two-dimensional vectors to items: Each item has a list of attachments, and each a attachment has a list of attributes.

If we take a step back it seems natural to look at attachments as a general way of storing multi-dimensional data for items. Cover art would be using this and I’m confident users will come up with great ideas to leverage this, too.

Will the usage of URLs be overly comple?

Additional code in the core (the beets.attachments module) should not be a proplem. The implementation is rather compact and we can easily provide abstractions for path vs. URL handling. So let’s look at how the core is used by beets and plugins.

To create attachements, both beets and plugins use the factory’s create method. Since the caller specifies the URL and may omit the file scheme, no additional handling is required. The second entry point is the attachments method of the Library. Calling it might return attachments whose URL is not a simple path. To eliminate the need for the consumer to check the attachments for non-file URLs we can simply add a scheme keyword that defaults to file to the method. Plugins, but not beets, use a third entry point to interact with attachments: they provide discover and collect methods to register plugin types and add metadata. The current implementation already relieves plugins from the obligation to check if URL to passed to discover and collect is a path 1. To discover URLs plugins will need to implement a discover_url method.

I hope this adresses all the issues. Just to be clear, URLs are not absolutely necessary, but since we are designing a new API from scratch we should keep it as extendible as possible and URLs are a simple yet powerful tool to achieve this.

@ghost
Copy link

ghost commented May 25, 2016

What should we do about this?

@sampsyo
Copy link
Member

sampsyo commented May 25, 2016

Probably the best thing to do would be to regroup and work on the design. There's probably lots we can use from this PR, but we need to think carefully about the approach again before proceeding.

@jtpavlock jtpavlock marked this pull request as draft July 3, 2020 20:03
@jtpavlock
Copy link
Contributor

Should this get closed and be treated more as a reference than anything?

@stale
Copy link

stale bot commented Nov 18, 2020

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 Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants