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

improve: cover (ui) #115

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

improve: cover (ui) #115

wants to merge 16 commits into from

Conversation

daiyam
Copy link
Contributor

@daiyam daiyam commented Jan 18, 2025

This PR does:

  • in the album details:
    • hide the default context menu on the cover
  • in the album context menu:
    • fix the Search for artwork for the album and not the song
  • in the info popup:
    • add a Search for artwork button
    • improve status text (In medadata, cover.jpg)
    • allow to delete the cover.jpg file
    • remember the directory of the last selected cover.jpg file
    • the search button get their icons replace with the loading one when searching
    • can't search artwork when there is a artwork in the metadata or it haven't been saved
    • filter possible files to .jpg or .png

@daiyam daiyam changed the title enhance: cover improve: cover Jan 19, 2025
@daiyam daiyam changed the title improve: cover improve: cover (ui) Jan 19, 2025
@daiyam daiyam changed the title improve: cover (ui) [wip] improve: cover (ui) Jan 19, 2025
@daiyam daiyam changed the title [wip] improve: cover (ui) improve: cover (ui) Jan 19, 2025
@daiyam
Copy link
Contributor Author

daiyam commented Jan 19, 2025

I wanted to talk about the support for .webp file but lofty only support png, jpg, tiff, bmp and gif (https://github.com/Serial-ATA/lofty-rs/blob/main/lofty/src/picture.rs)

.webp is natively supported by (https://community.mp3tag.de/t/support-for-webp-image-previews/51592):

  • mp3tag
  • foobar2000

Maybe we could support it and do a conversion to jpg since .webp is becoming common.

@basharovV
Copy link
Owner

basharovV commented Jan 21, 2025

allow to delete the cover.jpg file

Since it's not explicit that seems a bit ambiguous. The button still says "Overwrite file" when you're actually
deleting/replacing the cover in the folder. Also, you should use invoke("delete_files") which will move to trash / recycle bin instead of actually deleting the file.

@@ -313,6 +370,7 @@
previousScope = hotkeys.getScope();
hotkeys.setScope("track-info");
document.addEventListener("paste", onPaste);
document.getElementsByClassName("artwork-container").focus();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getElementsByClassName returns an array, so you need to call focus on the first element

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm removing the line, I think it's a debug line, a bad one at that...

result = null;

result = await fetchAlbumArt(
$rightClickedAlbum,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this will always fetch the artwork for the previously $rightClickedAlbum even when on another track. It's probably not set to null properly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't reproduce the issue. On which view are you?

Copy link
Owner

@basharovV basharovV Jan 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reproduce: 1) open info for any album, then 2) open info for single track in library without cover and click Fetch art. It will fetch the artwork for the previously viewed album instead of the current track.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But let's fix that separately from this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fixed. I'm passing all the arguments when a call to the popup occurs.

@daiyam
Copy link
Contributor Author

daiyam commented Jan 23, 2025

Since it's not explicit that seems a bit ambiguous. The button still says "Overwrite file" when you're actually deleting/replacing the cover in the folder.

I haven't changed the behavior. It replaces the artwork in the metadata.
I just made sure that the artwork file is removed since it would take priority on the metadata one.

Also, you should use invoke("delete_files") which will move to trash / recycle bin instead of actually deleting the file.

Fixed. Thx

@basharovV
Copy link
Owner

basharovV commented Jan 29, 2025

I haven't changed the behavior. It replaces the artwork in the metadata.

Ah sorry I didn't get it at first.

I just made sure that the artwork file is removed since it would take priority on the metadata one.

In the unlikely case that you want some tracks in a folder with encoded art, while others use cover.jpg/png - removing the file will stop showing the artwork for those tracks. I'd avoid deleting it unless you're actually editing the entire album. The encoded artwork will anyway take priority.

@daiyam
Copy link
Contributor Author

daiyam commented Jan 29, 2025

In the unlikely case that you want some tracks in a folder with encoded art, while others use cover.jpg/png - removing the file will stop showing the artwork for those tracks. I'd avoid deleting it unless you're actually editing the entire album. The encoded artwork will anyway take priority.

Now, it deletes the artwork file only when an album (with all tracks) is being edited.

@basharovV basharovV added this to the 0.13.0 milestone Jan 29, 2025
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.

2 participants