PICARD-1044: Add extension point for local cover art matching modes - #3385
PICARD-1044: Add extension point for local cover art matching modes#3385zas wants to merge 3 commits into
Conversation
…ternation/anchor flags
Add opt-in keyword flags to wildcards_to_regex_pattern() and thread them
through pattern_as_regex():
- allow_char_class (default True): when False, '[' and ']' are matched
literally instead of starting a character class. Useful for matching file
names that commonly contain brackets, e.g. "Album [2007].jpg".
- allow_alternation (default False): when True, '{a,b,c}' is interpreted as an
alternation, e.g. '*.{jpg,png}' matches "cover.jpg" or "cover.png".
- anchored (default False): when True the returned expression is wrapped in
'^...$'.
Defaults preserve the previous behavior. This is a self-contained utility
improvement that enables richer file-name pattern matching for callers.
Add unit tests covering the new flags and pattern_as_regex passthrough.
…join
Two latent correctness fixes in the Local Files cover art provider:
- _default_types was tuple('front'), which builds ('f', 'r', 'o', 'n', 't')
instead of the intended single 'front' type. Use ('front',).
- find_local_images() built the file path as
os.path.join(current_dir, root, filename). os.walk(current_dir) already
yields roots prefixed with current_dir, so with a relative current_dir this
double-prepends the base directory and the resulting path never exists,
silently dropping matches in sub-directories. Use os.path.join(root,
filename).
Add test/test_coverartprovider_local.py covering the regex matching path and
regression tests for both fixes (the sub-directory test uses a relative start
directory so it actually exercises the double-prepend bug).
Make the way local cover art files are selected pluggable. Instead of the Local Files provider hard-coding regular-expression matching, a "matching mode" is now a LocalCoverArtMode registered against a new extension point (picard.extension_points.local_cover_art_modes). Picard core registers the regular-expression mode; plugins can register further modes (e.g. a scripting mode) from their enable() via api.register_local_cover_art_mode(), with no change to the provider or the options page. Each mode carries everything the provider and options page need: - id (persisted in the local_cover_match_mode setting), title, description and note strings; - queue_images(provider, value) to queue the cover art images; - get_value/set_value so the mode owns its value storage (a plugin backs these with its own plugin_config); - optional example, playground flag, make_matcher (for the options-page live test) and show_doc (documentation button). Provider and options page: - CoverArtProviderLocal.queue_images() looks up the active mode and delegates to it; if the configured mode is not registered (e.g. its plugin was uninstalled) nothing is queued rather than silently falling back. - ProviderOptionsLocal is driven entirely by the registered modes: a selector lists them (hidden when only one exists) and the field label, note, placeholder, playground and documentation button adapt to the active mode. Mode switching is lossless within the dialog. Plugin API: - Expose LocalCoverArtMode and PluginApi.register_local_cover_art_mode() from picard.plugin3.api. options.py keeps only the local_cover_regex and (now string-valued) local_cover_match_mode options; the mode enum/descriptor table is gone in favor of the extension point. local_cover_match_mode defaults to 'regex' so existing configurations are unaffected. Add tests covering the extension point, the regex mode registration, the options page (selector, load/save, value round-trip), queue dispatch and the full mode lifecycle (default regex -> plugin mode selected and used -> plugin removed -> nothing queued).
phw
left a comment
phw
left a comment
There was a problem hiding this comment.
I'm really unsure about this approach, for several reasons. Mainly I think the local cover art provider has other issues, see e.g. the discussions in #1934 . The core issue here is that the cover art provider model, which has been created in a way so it can find covers for loaded releases, does not fit well for loading local files, since this is file based. Rather loading local files seems to be something that should be handled when files get loaded, similar to tags.
Adding a public API endpoint, that extends a single cover art provider, which we already do consider problematic, is I think not something we should do. It ads some commitment and removes flexibility in changing local cover art handling.
My second concern is that I don't really see the value of the specific API extension point for this. A different local cover art provider could be already registered by registering a new cover art provider. Such a provider could, if it wants to, already subclass CoverArtProviderLocal, and just re-implement queue_images.
What is left then is the generic UI that is being provided.
| # Queues cover art images for an album. Called as ``queue_images(provider, | ||
| # value)`` where ``provider`` is the CoverArtProviderLocal instance and | ||
| # ``value`` is the active mode's stored value. | ||
| QueueImages = Callable[[object, str], None] |
There was a problem hiding this comment.
Instead of object this should be CoverArtProviderLocal
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class LocalCoverArtMode: |
There was a problem hiding this comment.
Instead of a data class with callables being defined I think a proper base class would work better. The base class also can provide default implementations.
| note: str | ||
| queue_images: QueueImages | ||
| get_value: Callable[[], str] | ||
| set_value: Callable[[str], None] |
There was a problem hiding this comment.
get_value / set_value are unclear what kind of value they set. It is actually the expression that is used for matching, hence I would call this matching_expression or simply expression, and make it a property of the class.
| description=N_("Local cover art files match the following regular expression:"), | ||
| note=N_( | ||
| "First group in the regular expression, if any, will be used as type, " | ||
| "ie. cover-back-spine.jpg will be set as types Back + Spine. " |
|
We'll revisit later, not the right approach |
2 participants
Summary
Problem
There has been demand (PICARD-1044) for more flexible local cover art matching than a single regular expression — in particular, selecting cover art by a file name produced from a tagger script. Rather than hard-coding a second mode, the provider should expose an extension point so new ways to select local cover art can be added independently, including from plugins.
Solution
Introduce a
LocalCoverArtModeextension point (picard.extension_points.local_cover_art_modes):LocalCoverArtModecarrying everything the provider and options page need:id(persisted in thelocal_cover_match_modesetting), UI strings,queue_images(provider, value),get_value/set_value(each mode owns its value storage), and optionalexample,playground,make_matcher(options-page live test) andshow_doc.enable()via the newapi.register_local_cover_art_mode(...)(bothLocalCoverArtModeand the API method are exported frompicard.plugin3.api).CoverArtProviderLocal.queue_images()looks up the active mode and delegates to it. If the configured mode is not registered (e.g. its plugin was uninstalled), nothing is queued — no silent fallback to a different mode.ProviderOptionsLocalis driven entirely by the registered modes: a selector lists them (hidden when only one exists) and the field label, note, placeholder, playground and documentation button adapt to the active mode. Mode switching is lossless within the dialog.Refactoring/support also included:
wildcards_to_regex_pattern/pattern_as_regexwith opt-inallow_char_class,allow_alternationandanchoredflags (defaults preserve current behavior)._default_typestuple and a double-prepended sub-directory path). These are the same two commits as Fix local cover art default types and subdirectory path join #3384.local_cover_match_modedefaults to'regex', so existing configurations are unaffected.A working example plugin (a scripting mode) that consumes this extension point has been prototyped separately to validate the API end to end.
Verified locally: full test suite passes;
ruff check/ruff formatclean;ty checkintroduces no new errors. New tests cover the extension point, regex mode registration, the options page (selector, load/save, value round-trip), queue dispatch, and the full mode lifecycle (default regex → plugin mode selected and used → plugin removed → nothing queued).AI Usage
In accordance with the AI use policy portion of the MetaBrainz Contribution Guidelines, the level of AI/LLM use in the development of this Pull Request is:
The extension-point design, provider/options refactor and tests were developed with AI assistance; the design decisions and the result were reviewed and verified by the author.
Action
Additional actions required:
Update Picard documentation (please include a reference to this PR)
Other (please specify below)
The plugin v3 API docs (
docs/PLUGINSV3/) should documentregister_local_cover_art_mode/LocalCoverArtModebefore this is finalized.Overlaps Fix local cover art default types and subdirectory path join #3384 (the two fix commits); decide whether to land that first and rebase, or supersede.