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

feat: add tooltip support for PDF files #374

Merged

Conversation

leon-richardt
Copy link
Contributor

@leon-richardt leon-richardt commented Oct 15, 2022

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Closes #363.

This PR adds the concept of ContentTypeResolvers. With these, link resolving can be specialized by the response's Content-Type header value rather than by matching on the URL.

This concept is then implemented for the content type application/pdf. For links with this content type, a tooltip with the following information is generated:

  • Title
  • Author
  • Number of pages
  • Creation date
  • Thumbnail of first page

Review Notes

Points a reviewer can focus on:

  1. Do you agree with the way ContentTypeResolvers are created and passed to the LinkLoader?
  2. I decided to put the ContentTypeResolver interface and the PDFResolver implementation in the defaultresolver package. Would you rather have them elsewhere or in a subpackage?
  3. Do you agree with the format of the PDF tooltip? Should there be more fields?
  4. Currently, the export format of thumbnails for PDF files is hardcoded. Should there be a more general way of defining these?

Testing

Links to test this with:


Thank you!

This allows specialization of default link resolving by Content-Type
instead of by matching on the request URL.
@leon-richardt leon-richardt changed the title Feat/content type resolver/pdf feat: add tooltip support for PDF files Oct 15, 2022
@leon-richardt leon-richardt marked this pull request as ready for review October 15, 2022 22:10
@codecov
Copy link

codecov bot commented Oct 15, 2022

Codecov Report

Merging #374 (fb185bd) into master (dbc34af) will decrease coverage by 0.63%.
The diff coverage is 21.59%.

@@            Coverage Diff             @@
##           master     #374      +/-   ##
==========================================
- Coverage   45.97%   45.34%   -0.64%     
==========================================
  Files          99      100       +1     
  Lines        3589     3670      +81     
==========================================
+ Hits         1650     1664      +14     
- Misses       1893     1959      +66     
- Partials       46       47       +1     
Impacted Files Coverage Δ
internal/resolvers/default/model.go 100.00% <ø> (ø)
pkg/thumbnail/thumbnail.go 0.00% <0.00%> (ø)
internal/resolvers/default/pdf_resolver.go 12.96% <12.96%> (ø)
internal/resolvers/default/link_loader.go 49.50% <25.00%> (-6.45%) ⬇️
internal/resolvers/default/link_resolver.go 42.92% <100.00%> (+1.17%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pajlada pajlada merged commit 67a1371 into Chatterino:master Oct 16, 2022
@leon-richardt leon-richardt deleted the feat/content-type-resolver/pdf branch October 16, 2022 11:24
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.

Serve Link Info for PDFs
2 participants