Skip to content

Fix initial album fetch + cleanup#232

Merged
htkhiem merged 6 commits intohtkhiem:mainfrom
ShadiestGoat:fix-initial-album-covers
Mar 14, 2026
Merged

Fix initial album fetch + cleanup#232
htkhiem merged 6 commits intohtkhiem:mainfrom
ShadiestGoat:fix-initial-album-covers

Conversation

@ShadiestGoat
Copy link
Contributor

This is a fix for #230

The root cause was basically that sometimes some methods would return image names (1234.png), sometimes the full path (/home/name/.cache/euphonica/images/1234.png)

To fix this, I had a bit of a rabbit hole moment and updated a bunch of stuff to all return the same thing - the the image name. Imo its just more robust than the full path. Also, in some instances it'd get the full path, then extract the image name from there so I cleaned that up.

I also extracted some of the image loading parts into a util function, since they all do the same but slightly different. Finally, there was 1 todo in set_image_internal which I resolved in what I hope is a clean manner

One annoyance that I wanted to resolve but just couldn't figure out the borrow/lifetime thing with is load_image is exclusively used with the .external.call wrapper, and despite how much I tried i could not figure out how to make that wrapper be part of the load_image util so ://

As with before, I'm not a rust person & am still flying more or less blind so please lmk if there is something wrong!

@htkhiem htkhiem linked an issue Feb 21, 2026 that may be closed by this pull request
@ShadiestGoat ShadiestGoat force-pushed the fix-initial-album-covers branch from c9c7711 to fd4e589 Compare February 21, 2026 16:40
@ShadiestGoat
Copy link
Contributor Author

Hey @htkhiem I added the things you asked. Since the image bytes are written into a separate buffer (due to image format requirements I haven't noticed in the initial impl), from_owned can be used anyways. Please take a look when you get the time, cheers!

Copy link
Owner

@htkhiem htkhiem left a comment

Choose a reason for hiding this comment

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

(optional request) I think I've found a way to further optimise the .texture() calls for newly-downloaded images.

@htkhiem
Copy link
Owner

htkhiem commented Mar 9, 2026

@ShadiestGoat Do you think we should merge this PR as-is first then apply the above optimisation later? I think your fixes are pretty important and should be on main soon.

I've given the PR a spin and it seems to work well, including for cold starts (by deleting my cache), so I think it's ready.

@htkhiem
Copy link
Owner

htkhiem commented Mar 14, 2026

Merging now since the optional fix also seems to work well. In case I've done something wrong please open a new PR 🙏.

All in all, thanks again & hope you can keep contributing in the future!

P.S. May I add your GitHub username & contact email to the About dialog's credit section?

@htkhiem htkhiem merged commit 1739e86 into htkhiem:main Mar 14, 2026
@ShadiestGoat
Copy link
Contributor Author

Hey - sorry for no reply for a while, it was a long week at work haha. I'll check it out in main, but if you tested it & it works, then it works!

I'd gladly accept getting added to the contributors :) (perhaps just my username though, not email? thanks!)

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.

First load of album view does not display even embedded album covers

2 participants