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

Fetchart preview/General thoughts on user interaction by plugins #1917

Closed
wants to merge 4 commits into from

Conversation

wisp3rwind
Copy link
Member

As a disclaimer, I'm absolutely new to Beets, and maybe I'm utterly mistaken in some of the following. The main value of this PR might turn out to be for me to write down and clarify some thoughts..

My initial intention was to introduce a new configuration option for fetchart to optionally not choose a cover automatically. Instead, it would collect all matches from all sources, display a list with some detail such as origin and size, and prompt the user to select one, optionally previewing it in an image viewer of choice.
For my usage pattern, this would be relevant both during import and on manual execution.
The code attached is some untested refactoring which might lead to less code duplication when actually implementing this.

The problem that comes up however, is that Beets currently is not designed to handle user interaction at more than one stage (i.e. apart from match selection in the user_query stage) during import. Due to parallel execution of the pipeline, I see two options to prevent mutiple stages from writing to the commandline simultaneously:

  • Modify beets/util/pipeline.py to be able to specify that two distinct pipeline stages are mutually exclusive.
  • Add some locking mechanism in beets/ui/commands.py:TerminalImportSession.choose_match. This seems more appropriate to me, for example in view of a GUI that might want to collect all pending choices.

Are there any thoughts or hints on how best to solve the issue? Would a change like this have any chance of being merged?
I intend to develop this at least to some working state as I get the time, although that might take a bit.

@sampsyo
Copy link
Member

sampsyo commented Mar 23, 2016

Interesting proposal! This idea has come up before and would obviously be useful. (So would, in general, more flexibility in user interactions during import.)

Letting multiple threads interact with the user seems pretty complicated to get right. Fundamentally, independent of the implementation strategy, ordering would get weird. For example:

  1. The importer shows the metadata match for album 1.
  2. Show the match for album 2.
  3. Show the match for album 3.
  4. Show the album art choices for album 1.
  5. Show the match for album 4.
  6. Show the album art choices for album 2.

And so on. This isn't exactly the best user experience—clearly, the ideal world would still do all the user interaction in one shot.

So, maybe the simplest implementation would just be to do album art selection synchronously with the UI stage if interaction is required? I can't, at the moment, think of a way to avoid strange ordering effects otherwise.

We could make this less painful, performance-wise, by also letting fetchart speculatively fetch album art for metadata matches before they're presented to the user.

Also, the refactoring part of this PR looks interesting by itself. How would you feel about splitting those changes off into a separate PR so we can test and merge them separately (and sooner)?

@wisp3rwind
Copy link
Member Author

I can certainly split the refactoring off, it's mostly missing adapted tests, apart from that, it should be fine. I can open a new PR as soon as I've done so. It does not add any new functionality whatsoever, though.

You're right about the ordering, and I can't think of an alternative either. I would be fine with the interleaving, but it worsens usability.
I'm not sure about pre-fetching album art though. I'd at least like to provide the dimension of each picture. In the current implementation, this requires downloading the complete file. With many sources enabled and for 20 or more matches per album, this has the potential to consume a significatn amount of bandwidth.
If it were possible to download only the image header, and fetch the whole image only just before opening it, this could work without introducing too much delay.

The whole idea has turned more complex than I thought at first; I guess I' need to get more familiar with the entire codebase before being able to implement it.

Notes to me
other PRs that probably should be merged before implementing plugin interaction. I only skimmed them, so this is only the very first impression:

@sampsyo
Copy link
Member

sampsyo commented Mar 25, 2016

Good PRs without any new functionality are my favorite PRs. 😃

Yeah, it's a pretty complicated conceptual problem regardless of the implementation—it bears some pondering.

@msdos
Copy link

msdos commented May 10, 2019

I came here from #3262.

What about adding an --all parameter to https://beets.readthedocs.io/en/v1.4.7/plugins/fetchart.html#manually-fetching-album-art? Not exactly automatic, but still useful. Problem is that you'll have multiple files, how to handle the art_filename config?

@wisp3rwind
Copy link
Member Author

What about adding an --all parameter to https://beets.readthedocs.io/en/v1.4.7/plugins/fetchart.html#manually-fetching-album-art? Not exactly automatic, but still useful. Problem is that you'll have multiple files, how to handle the art_filename config?

The goal here was never to retain all of the files, but to do an interactive selection. However, as I mentioned in the other issue, I found that the current automatic behaviour is usually good enough and that I prefer to fix up the occasional add choice manually later. The infrastructure for tracking several images does not exist in beets right now, search the issues for the attachment proposal about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants