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 the poster selection for audio/music based torrent #1334

Merged
merged 12 commits into from Apr 26, 2018

Conversation

3 participants
@Borewit
Copy link
Member

commented Mar 4, 2018

Addressing issue #1332

Borewit added some commits Mar 4, 2018

#1332: Improve the poster selection for audio/music based torrent,
aiming to get the cover-art selected as the torrent poster.
#1332: Improve cover image score algorithm:
- If filename score is equal, pick highest resolution
- Search for keyword occurrence in file name
Added some function comments.
Simplified code a bit.
@Borewit

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2018

@codealchemist: Alberto, can you please review this one as well?

@codealchemist

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

I'm getting this error:
image

I was able to reproduce with MP3 files only so far.
Good thing is this should get fixed by #1361 :)

@codealchemist

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

Latest merge on this branch fixed thee issues I saw before, yay!

Properly displaying poster before all metadata was downloaded:
image

Properly displaying poster after metadata was downloaded:
image

Content list background:
image

Torrent list poster looking good:
image

@codealchemist

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

I was wondering if we should also display the same background on the torrent details view (files list).
Anyway, this looks really good!
Nice enhancement @Borewit!

@codealchemist
Copy link
Contributor

left a comment

Did a quick testing of the changes and it's looking awesome :)

@codealchemist

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

Hey @feross, if you agree I think we should merge this one too and then I'll prepare the changelog on another PR.

@feross
Copy link
Member

left a comment

Didn't look through it very thoroughly, but it generally looks good to me.

If you're confident, go ahead and merge it @codealchemist. :)

@codealchemist codealchemist merged commit c6a7b7c into webtorrent:master Apr 26, 2018

2 checks passed

Node Security No known vulnerabilities found
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Borewit

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2018

That was probably not a good idea to commit on top of this pull request after the merge.
I created a new push request: #1368.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 25, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.