Skip to content

AlbumCoverManager::SaveCoverToFile() not working as intended? #2039

@diracsbracket

Description

@diracsbracket
  • I have checked the FAQ for answers.
  • [ X] I have checked the Changelog that the issue is not already fixed.
  • [ X] I believe this issue is a bug, and not a general technical issue, question or feature requests that can be discussed on the forum.

Describe the bug
AlbumCoverManager::SaveCoverToFile() seems not to be working as intended. It relies on the following loop that iterates over the cover_types_ list which is initialized as cover_types_ = AlbumCoverLoaderOptions::LoadTypes();

Song song = GetSingleSelectionAsSong();
if (!song.is_valid() || song.art_unset()) return;   // <-- checking song.art_unset()) here causes undesired behavior in the loop below

for (const AlbumCoverLoaderOptions::Type cover_type : std::as_const(cover_types_)) {
    switch (cover_type) {
      case AlbumCoverLoaderOptions::Type::Unset:
          return;                                                  // <-- song.art_unset() should be checked here instead to avoid early exit

The problem is that AlbumCoverLoaderOptions::LoadTypes() always loads the default full list of types, in this order:

AlbumCoverLoaderOptions::Type::Unset
AlbumCoverLoaderOptions::Type::Embedded
AlbumCoverLoaderOptions::Type::Manual
AlbumCoverLoaderOptions::Type::Automatic

This list is the default when CoversSettings::kSettingsGroup\CoversSettings::kTypes is checked. However, this setting never
seems to be set anywhere in the Strawberry code, and therefore always the same default list is returned.

In the above switch in the for loop the Type::Unset case is always met first, causing the function as a whole to always exit early, doing nothing else.

Incidently, the same kind of logic seems to be applied in AlbumCoverChoiceController::ShowCover():

for (const AlbumCoverLoaderOptions::Type type : std::as_const(cover_types_)) {
    switch (type) {
      case AlbumCoverLoaderOptions::Type::Unset:{
        if (song.art_unset()) {
          return;
        }  
    ...
  }
 ...
}

The difference between both loop implementations is that in the latter song.art_unset() is checked within the case label, while in the former,
this is done at the top of the function if (!song.is_valid() || song.art_unset()) return;, which seems to be what causes the unintended behavior:
the song.art_unset() check should also be performed within the case section, to avoid early exit.

To Reproduce
Open the album cover manager, select a cover and choose Save cover to file. Nothing happens.

Expected behavior
The selected cover should be saved to file.

Screenshots:
If applicable, add screenshots to help explain your problem.

System Information:

  • Operating system: Debian 13 (Trixie)
  • Strawberry Version: Version 1.2.18-3-g7bf1bc0c

Additional context
Add any other context about the problem here.

Metadata

Metadata

Assignees

Type

No type
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions