Skip to content

Conversation

eternalfrustation
Copy link

@eternalfrustation eternalfrustation commented Feb 27, 2024

As proposed in the previous PR (#466), this PR implements displaying cover art from mpd using the readpicture command

Copy link
Owner

@JakeStanger JakeStanger left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to rewrite this. There are some issues I've raised with your implementation which will need addressing, but it's mostly just simplifying things and refactoring.

In future, can you please have a read of the contributing guide - it's very short but has a few requirements I ask people to stick to in order to make both our lives easier.

The build is also failing currently, which will need to be addressed of course.

As suggested previously, This commit uses message passing
to send the image from the client to the main thread
while retaining the original way of setting the album art.
Removed the patch section for ahash from Cargo.toml
Due to the previous commit, cargo could not find the
proper ahash version, ran cargo update to fix that.
Now using gdk's image parser for parsing the mpd's
thumbnails that are read from readpicture command
@eternalfrustation
Copy link
Author

I have done all the requested changes

@eternalfrustation
Copy link
Author

What kind of Clippy rules are you using

Copy link
Owner

@JakeStanger JakeStanger left a comment

Choose a reason for hiding this comment

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

Looking a lot better so thanks for the work, there's still a few changes that need to be made. Once those are done I'll give it another review with a fine comb and we should be about there.

@JakeStanger
Copy link
Owner

What kind of Clippy rules are you using

The defaults:

run: cargo clippy --all-targets --all-features

Only sending the GetAlbumArt signal when track changes and
the cover_art path does not exist.
@JakeStanger
Copy link
Owner

Please fix the merge conflict before I review

@JakeStanger JakeStanger added M:Music Related to the Music module T:Module Enhancement Improvements to an existing module. A:Client Relating to service/client code. labels Apr 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A:Client Relating to service/client code. M:Music Related to the Music module T:Module Enhancement Improvements to an existing module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants