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: generate multi-image thumbnails for multi-image tweets #373

Merged
merged 6 commits into from
Oct 15, 2022

Conversation

leon-richardt
Copy link
Contributor

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

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Feature Description

This PR closes #347.

For tweets with multiple images, link preview thumbnails no longer just display the first image but a collage of all images. The images are arranged in a 2×2 grid.

Reading Twitter's API documentation, I would have expected to receive thumbnail URLs for attached videos as well but it seems that these are not returned from the endpoint. Thus, no thumbnails for videos will be included in the collage. Maybe we will be able to change this with the v2 endpoint.

Design Decisions

Implementing this feature required some larger-scale design decisions. I will describe and justify these decisions in this section.

Serving Generated Values

Previous to this PR, thumbnails served by the Chatterino API referred to an image resource identified by some URL. The only "contribution" of the API was scaling the image down to the dimensions specified in the configuration file. With this PR, we want to serve an image that does not exist anywhere else: a collage of multiple images. In my opinion, this did not fit into the existing architecture very well.

The solution I opted for consists of introducing an additional route: generated/. This route serves data that has been generated by the Chatterino API itself and does not directly reference resources elsewhere.

cache.DependentCache

To implement the above, I defined a new type of cache: cache.DependentCache. A DependentCache stores []byte values under a key. However, this key is directly associated with a parent key in a regular cache.Cache. When the parent key is deleted, all child keys in registered dependent caches are deleted as well.

Currently, the only DependentCache implementation is PostgreSQL-based. I decided not to implement the interface for the in-memory cache since it is currently not used anywhere in the code, and I would rather not spend time implementing the interface if it ends up never being used. If you require the interface for an in-memory cache as well, let me know and I may be able to provide assistance in implementing it.

Commit/Rollback Mechanism

Child keys may be inserted into the dependent cache before their parents exist. (This is the case for collages, for example.) If an error occurs before the parent key is inserted in the parent cache, the child keys may be orphaned. This would mean they would never get cleaned up. The commit/rollback mechanism makes sure that child keys only remain in the cache if the parent is successfully inserted.

Additionally, a mechanism has been implemented that cleans up any values in dependent caches that have been inserted longer than 24 hours ago. This is meant as fail-safe to prevent caller errors from clogging up the database.

Collage Layout

Ideally, I would have liked to use vips_join() for the collage. However, at the point of writing, govips does not support all parameters of the C library in its binding. Thus, it is not much better than using ImageRef.ArrayJoin(). This has the effect that tweets with three images lead to somewhat ugly thumbnails. In a future govips version, using ImageRef.Join() could yield a nice QoL improvement.

Review Notes

I hope the random formatting changes sprinkled throughout are okay; they were introduced by my autoformatter.

Points a reviewer can focus on:

  1. Do you agree with the architectural decision of serving generated values via a new route?
  2. Is the extraction of key generation into an own interface a good solution to resolve the circular dependency?
  3. Do you agree with the design of the dependent_values table and the PostgreSQLDependentCache overall?
  4. Is there a nicer way to pass down the collage cache to the TweetLoader?

Testing

Make sure you provide a valid Twitter API token.

Tweets to test this with:


Thank you!

@leon-richardt leon-richardt marked this pull request as ready for review October 15, 2022 11:56
This will be needed for the next commit. Different components may need
to know the cache key format of a specific cache. Without extracting the
functionality into an own interface, users can run into a circular
dependency. (This happened to me.)

Split into its own commit to allow for easier reviewing.
@leon-richardt leon-richardt changed the title Feat/twitter/image collage feat: generate multi-image thumbnail for multi-image tweets Oct 15, 2022
@codecov
Copy link

codecov bot commented Oct 15, 2022

Codecov Report

Merging #373 (4ead9b9) into master (b855cd0) will decrease coverage by 2.77%.
The diff coverage is 32.05%.

@@            Coverage Diff             @@
##           master     #373      +/-   ##
==========================================
- Coverage   48.74%   45.97%   -2.78%     
==========================================
  Files          98       99       +1     
  Lines        3159     3589     +430     
==========================================
+ Hits         1540     1650     +110     
- Misses       1576     1893     +317     
- Partials       43       46       +3     
Impacted Files Coverage Δ
internal/caches/twitchusernamecache/cache.go 0.00% <0.00%> (ø)
internal/db/pool.go 0.00% <ø> (ø)
internal/resolvers/oembed/resolver.go 0.00% <0.00%> (ø)
pkg/cache/db.go 1.49% <0.00%> (-2.12%) ⬇️
pkg/cache/key_provider.go 0.00% <0.00%> (ø)
pkg/cache/memory.go 2.12% <0.00%> (-0.23%) ⬇️
pkg/utils/image.go 0.00% <0.00%> (ø)
pkg/utils/url.go 0.00% <0.00%> (ø)
internal/resolvers/default/link_resolver.go 41.75% <28.57%> (-5.90%) ⬇️
internal/resolvers/twitter/tweet_loader.go 54.50% <44.51%> (-33.26%) ⬇️
... and 18 more

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

Split into its own commit for easier reviewing. See PR description for
more context.
Various formatting changes from my autoformatter. Extracted into its own
commit for easier reviewing.
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Only nitpick I found during code review, will give it a test as well

internal/resolvers/twitter/tweet_loader.go Outdated Show resolved Hide resolved
@leon-richardt leon-richardt changed the title feat: generate multi-image thumbnail for multi-image tweets feat: generate multi-image thumbnails for multi-image tweets Oct 15, 2022
@pajlada pajlada enabled auto-merge (squash) October 15, 2022 12:59
@pajlada pajlada merged commit dbc34af into Chatterino:master Oct 15, 2022
@leon-richardt leon-richardt deleted the feat/twitter/image-collage branch October 15, 2022 13:04
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.

Tweet Link Thumbnails: Compose Multiple Images into Collage
2 participants