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

Dynamic Movie/Show Images from TMDB #43

Merged
merged 5 commits into from
Apr 26, 2023
Merged

Conversation

xBiei
Copy link
Contributor

@xBiei xBiei commented Jan 11, 2023

📑 Description

Well, because I hate the trakt logo and the other 2 logos for shows and movies because they look ugly and unrelated to whatever I'm watching, and discord now accepts urls in RPC images I figured we should use TMDB resources to improve the quality of this app.

Also, other than being able to use dynamic images, this PR won't change the dependencies at all.
And of course, if the app failed to use dynamic images, it'll fallback to the old ones.
Closes #25

✅ Checks

  • code: My pull request adheres to the code style of this project
  • docs: Added / updated
  • tests: All the tests have passed

@DareFox
Copy link

DareFox commented Jan 13, 2023

Kinda awkward to have two same PR (#42) 😅

@rollingmoai
Copy link

Duplicate of #42

@xBiei
Copy link
Contributor Author

xBiei commented Jan 21, 2023

Yeah I know lol
Both are doing the same thing so it probably doesn't matter which is used
I just PR-ed the one I use

@rollingmoai
Copy link

rollingmoai commented Jan 21, 2023

Side note: Have you even checked existing PRs before making yours? It's like reinventing the wheel 😅

@xBiei
Copy link
Contributor Author

xBiei commented Jan 21, 2023

Side note: Have you even checked existing PRs before making yours? It's like reinventing the wheel 😅

I did actually. Submitted it anyway. :3

Edit: The reason I submitted mine is because I think it's better in some ways. You can check the code yourself. No extra dependencies, Readme updated and actually faster.

@afonsojramos
Copy link
Owner

afonsojramos commented Feb 2, 2023

Sorry for the absence, it seems I wasn't properly notified about both PRs.
I'll evaluate which one to merge today, and thank you both for you contributions @xBiei @DareFox!

While you're here @DareFox (since @xBiei already voiced their opinion), do you believe one has advantages over the other?

Side note: I actually also tried to fix #25, but I was using IMDB which for some reasons was causing some issues, which I never got around to. Eventually, I just pushed everything I had locally as I was switching computers, and then never got around to continuing the work. So again, thanks for your contributions!

@afonsojramos
Copy link
Owner

afonsojramos commented Feb 7, 2023

@xBiei @DareFox The way I see it, there are two clear distinctions between the two PRs.

The advantage to @DareFox's PR I suppose is a two way sword. Because it uses tmdb crate which makes the code cleaner, however we are adding a dependency for just two URLs.

This PR has caching for the image links, which I think is a great advantage, doesn't use any additional crates

So, in the end, I think I will close #42, and move forward with this one.

What I would improve on would be to also have a "disposable" tmdb token for quick usage, what do you think?

I still need to further test this implementation however, especially to test the situations where the image is not found.

Note: There are some clippy annotations that I would also fix.

@xBiei
Copy link
Contributor Author

xBiei commented Feb 7, 2023

What I would improve on would be to also have a "disposable" tmdb token for quick usage, what do you think?

Yeah I agree, that would be good for the UX.

I still need to further test this implementation however, especially to test the situations where the image is not found.

If there's no url response, it'll use the media's type as url which discord fetches from the uploaded images to the app in their portal.

Note: There are some clippy annotations that I would also fix.

I'll check that out later

@rollingmoai
Copy link

rollingmoai commented Feb 7, 2023

What I would improve on would be to also have a "disposable" tmdb token for quick usage, what do you think?

Yes, that would be a good idea alongside built-in client IDs for Trakt and Discord (#46)

@DareFox
Copy link

DareFox commented Feb 7, 2023

The advantage to @DareFox's PR I suppose is a two way sword. Because it uses tmdb crate which makes the code cleaner, however we are adding a dependency for just two URLs.

This PR has caching for the image links, which I think is a great advantage, doesn't use any additional crates

So, in the end, I think I will close #42, and move forward with this one

I agree, dependency just for url fetch is kinda ambigous and this PR is better. Also caching could save potential problem with burner token limit usage

What I would improve on would be to also have a "disposable" tmdb token for quick usage, what do you think?

It would be very cool, it will make less hassle with configuration

Also, I have one question with this PR (I can't test rn, because my setup is borked): Is poster cropped or has original size?
If not cropped, then Discord crops it and adds blur to it. I fixed it with this commit, and maybe you could apply this fix here too

@xBiei
Copy link
Contributor Author

xBiei commented Feb 7, 2023

Also, I have one question with this PR (I can't test rn, because my setup is borked): Is poster cropped or has original size?

If not cropped, then Discord crops it and adds blur to it. I fixed it with this commit, and maybe you could apply this fix here too

Yeah I noticed it is cropped and kinda blurry.
I'll check it out.

Co-authored-by: Afonso Jorge Ramos <afonsojorgeramos@gmail.com>
@xBiei
Copy link
Contributor Author

xBiei commented Apr 26, 2023

Looks like I got too busy and forgot about this lol.
It should be fine now.

@afonsojramos afonsojramos merged commit b98090c into afonsojramos:main Apr 26, 2023
@afonsojramos
Copy link
Owner

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.

Suggestion: Dynamic image
4 participants