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

Fix for PictrsImageMode::None #4604

Merged
merged 4 commits into from
Apr 10, 2024
Merged

Fix for PictrsImageMode::None #4604

merged 4 commits into from
Apr 10, 2024

Conversation

kroese
Copy link
Contributor

@kroese kroese commented Apr 8, 2024

Currently the option PictrsImageMode::None is not working since it is totally ignored. This pull fixes that.

kroese and others added 2 commits April 8, 2024 04:35
Co-authored-by: SleeplessOne1917 <28871516+SleeplessOne1917@users.noreply.github.com>
@kroese
Copy link
Contributor Author

kroese commented Apr 8, 2024

Even with this patch applied, the behaviour is still not consistent with older Lemmy versions.

In the past, for federated posts the thumbnail URL was set to the originating server when cache_thumbnails was disabled, for example: https://lemmy.ml/pictrs/image/25f1ebec-b06e-4bc1-9633-05c1d5f5e805.png?format=webp&thumbnail=256

But now, for federated posts the thumbnail links directly to the image on the external website (which can be much larger than 256x256).

So returning image_url works fine for locally created posts, but for federated posts it should return the link to the image on the other instance (if we want to keep the behaviour the same as it was previously).

Copy link
Member

@Nutomic Nutomic left a comment

Choose a reason for hiding this comment

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

Youre right I must have missed this during the rewrite.

So returning image_url works fine for locally created posts, but for federated posts it should return the link to the image on the other instance (if we want to keep the behaviour the same as it was previously).

I believe this is fixed by #4593, can you test it?

@kroese
Copy link
Contributor Author

kroese commented Apr 8, 2024

By looking at the code, this should be fixed by #4593 as it adds an check of post.thumbnail_url.is_none(); which should solve the problem.

But since that pull modifies a lot of files, it will be very complicated to test unless a 0.19.4-beta.3 tag is pushed, so that I can use my automatic build scripts for a new docker image.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Good catch.

@kroese I'll try to get another beta release out within a few days, after we merge my thumbnail fixes in the PR linked above.

@dessalines dessalines merged commit d5622a6 into LemmyNet:main Apr 10, 2024
1 check passed
@kroese
Copy link
Contributor Author

kroese commented Apr 10, 2024

This problem with the wrong image url's is still present in 0.19.4-beta.3, and not fixed by @dessalines PR. The thumbnail urls still aren't set to the pictrs of the remote instance, but to the original image.

I think the problem is that do_generate_thumbnail is set to false which causes the function to just return the URL of the (original) image that it would have fetched when it was true. But in this case it should return the thumbnail that was already generated by the remote instance. Those are two different URL's.

Nutomic added a commit that referenced this pull request Apr 11, 2024
Nutomic added a commit that referenced this pull request Apr 11, 2024
dessalines pushed a commit that referenced this pull request Apr 17, 2024
* Untangle thumbnail generation logic (ref #4604)

* prettier

* test cleanup

* fix tests

* also consider opengraph image for local thumbnail generation
@kroese kroese mentioned this pull request May 23, 2024
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.

None yet

4 participants