Embedart plugin: clearart improvements

[Morikko]

1/ Only clearart if an embedded art is present

clearart command always writes a file to reset the images whatever if the file contains an embedded art. This MR changes to first check if an embedded art is present, and only if so, update the file.

Use case example: to remove all the embedded arts in my library:

beet clearart

Note: There is no query option to filter songs with an embedded art.

It is equivalent to this:

kid3-cli -c 'timeout off' -c 'filter '\''not %{picture} equals ""'\''' -c 'select all' -c 'set picture ""' /path/to/dir

The benefits:

1. It saves some writing cost if the file has no embedded art
2. To not wrongly updating the FS stats (last updated at). Many software rely on that to detect a change. For example a backup software. In the worst case, the backup could double in size if there is no content deduplication. Else the backup would be still slower due to the deduplication.
3. Breaking COW FS. Some do not have live deduplication like btrfs, if a file is rewritten without reflinking the blocks then the blocks are duplicated leading to duplicate space usage with snapshots.

The downside is that it adds an extra read if the write is required. However, a read is still done before writing to get all tags, so most OS may correctly cache it and the overhead should be small.

2/ Auto clearart on import

Add a new config property called clearart_on_import. It is disabled by default for retro-compatibility. If set to true, the embedded arts are removed when an import is done.

This was already asked on discourse: https://discourse.beets.io/t/automatically-clear-artwork-on-import/1001

[github-actions]

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.

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• The mtime-based assertion in test_clear_art_with_yes_input can be flaky on filesystems with coarse timestamp resolution—consider adding a short sleep or manually bumping the mtime to ensure the modification time actually increases.
• Rather than conditionally registering import_task_files based on the config at init, register the listener unconditionally and guard execution inside the handler—this avoids needing to unload/reload plugins for config changes.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The mtime-based assertion in test_clear_art_with_yes_input can be flaky on filesystems with coarse timestamp resolution—consider adding a short sleep or manually bumping the mtime to ensure the modification time actually increases.
- Rather than conditionally registering import_task_files based on the config at init, register the listener unconditionally and guard execution inside the handler—this avoids needing to unload/reload plugins for config changes.

## Individual Comments

### Comment 1
<location> `beetsplug/_utils/art.py:209-212` </location>
<code_context>
             return real_path


+def clear_item(item, log):
+    if mediafile.MediaFile(syspath(item.path)).images:
+        log.debug("Clearing art for {}", item)
+        item.try_write(tags={"images": None})
+
+
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Consider handling exceptions when clearing art from items.

If item.try_write encounters an error, such as permission issues or file corruption, it will fail silently. Adding a try/except block with logging will help detect and handle these failures.

```suggestion
def clear_item(item, log):
    if mediafile.MediaFile(syspath(item.path)).images:
        log.debug("Clearing art for {}", item)
        try:
            item.try_write(tags={"images": None})
        except Exception as exc:
            log.error("Failed to clear art for {}: {}", item, exc)
```
</issue_to_address>

### Comment 2
<location> `beets/test/helper.py:367` </location>
<code_context>
    def create_mediafile_fixture(self, ext="mp3", images=[], target_dir=None):
        """Copy a fixture mediafile with the extension to `temp_dir`.

        `images` is a subset of 'png', 'jpg', and 'tiff'. For each
        specified extension a cover art image is added to the media
        file.
        """
        if not target_dir:
            target_dir = self.temp_dir
        src = os.path.join(_common.RSRC, util.bytestring_path(f"full.{ext}"))
        handle, path = mkstemp(dir=target_dir)
        path = bytestring_path(path)
        os.close(handle)
        shutil.copyfile(syspath(src), syspath(path))

        if images:
            mediafile = MediaFile(path)
            imgs = []
            for img_ext in images:
                file = util.bytestring_path(f"image-2x3.{img_ext}")
                img_path = os.path.join(_common.RSRC, file)
                with open(img_path, "rb") as f:
                    imgs.append(Image(f.read()))
            mediafile.images = imgs
            mediafile.save()

        return path

</code_context>

<issue_to_address>
**issue (code-quality):** Replace mutable default arguments with None ([`default-mutable-arg`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/default-mutable-arg/))
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.26%. Comparing base (ea2e7bf) to head (c125918).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6156      +/-   ##
==========================================
+ Coverage   68.24%   68.26%   +0.02%     
==========================================
  Files         138      138              
  Lines       18815    18824       +9     
  Branches     3167     3170       +3     
==========================================
+ Hits        12840    12851      +11     
+ Misses       5302     5300       -2     
  Partials      673      673              

Files with missing lines	Coverage Δ
beetsplug/_utils/art.py	55.31% <100.00%> (+1.47%)	⬆️
beetsplug/embedart.py	73.38% <100.00%> (+1.35%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Morikko]

I will add documentation and a changelog entry but I would like first to know if you would accept the proposed changes.

[Serene-Arc]

Hi, thanks for the PR! I've been absent from the maintainers team for a while so I'm trying to get situated on a couple PRs. To be clear, the changes with this PR is that it checks if the file actually has embedded art before clearing it?

Can you describe what issues this is causing regarding copy-on-write filesystems?

[Morikko]

Thanks for looking at it. I updated the description to be more clear. For the point 1/, I ended up using kid3-cli so I do not need it any more but I am still okay to adapt the implementation. For example, I could also add it behind a CLI flag or extension setting if it is necessary to keep the old behavior (by default).

The second improvement is still of an interest for me to avoid manual modification with kid3 before importing.

[Morikko]

@Serene-Arc Let me know what do you think about the changes. If those look promising, I could add the changelog and documentation.

[Serene-Arc]

Looks good!

[Serene-Arc]

@Morikko sorry, busy time of year! These changes are nice and simple and look well-implemented to me. Go ahead with the documentation and changelog.

[Morikko]

Done !