-
Notifications
You must be signed in to change notification settings - Fork 296
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
Use web-thumbnailer to retrieve thumbnails #687
Conversation
I have just read the patch comments, had a quick look at the implementation in Shaarli, and at the readme in https://github.com/ArthurHoaro/web-thumbnailer, and I would very much welcome this change. Lacking time to do a full code review though :/ Thanks |
48f0865
to
69eaabe
Compare
May I suggest to add a way for templates to retrieve thumbnails direct URL? Or an array with several useful parameters such as what computeThumbnail() function does. |
@kalvn What's your use case? |
@ArthurHoaro the idea would be to allow more flexibility in the way templates display the thumbnail (with background-image instead of for example). The current way is a bit rigid. |
97db222
to
9b49f9b
Compare
@kalvn Each links will have a PR updated and ready for v0.10! |
tpl/default/js/shaarli.js
Outdated
@@ -321,8 +321,8 @@ window.onload = function () { | |||
/** | |||
* bLazy trigger | |||
*/ | |||
var picwall = document.getElementById('picwall_container'); | |||
if (picwall != null) { | |||
var blazyElements = document.getElementsByClassName('.b-lazy'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be careful ..
https://caniuse.com/#feat=getelementsbyclassname :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know... I've used it multiple times in this file.
tests/ThumbnailerTest.php
Outdated
|
||
$this->thumbnailer = new Thumbnailer($conf); | ||
// cache files in the sandbox | ||
\WebThumbnailer\Application\ConfigManager::addFile('tests/utils/config/wt.json'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you should use a use
keyword? like you do in ConfigManager
Very nice.
add |
@nodiscc Good idea, I'll add that. And dates are now stored as |
index.php
Outdated
@@ -768,22 +769,51 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history, $sessionManager) | |||
// -------- Picture wall | |||
if ($targetPage == Router::$PAGE_PICWALL) | |||
{ | |||
if (! $conf->get('thumbnails.enabled')) { | |||
header('Location: ?'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe display a warning message to the user Picture wall unavailable (thumbnails are disabled)
. Or even better, hide the Picture wall
button from the toolbar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I did both.
* requires PHP 5.6 * use blazy on linklist since a lot more thumbs are retrieved * thumbnails can be disabled * thumbs size is now 120x120 * thumbs are now cropped to fit the expected size Fixes shaarli#345 shaarli#425 shaarli#487 shaarli#543 shaarli#588 shaarli#590
Upgrade web-thumbnailer and display thumbs right after download
I think I'm good here. I've:
As usual, clear the cache folder to test it. Regarding the milestone, since we didn't released EDIT: if you already ran the updater with previous commits, you might need to remove |
Neat, I'll test it to(day|morrow) :) |
I'm not sure. The goal behind this PR, beside having a cleaner solution, is to provide a more complete feature. I feel like this option is nice to have for people who want a lighter solution, but displaying thumbnails for any website which provides an OpenGraph image is a good default option. I don't really know what users would prefer, and this can easily be changed though, so any other opinion is welcome here.
It should display a big
Sure, I'll add those. For the last part:
Thanks again for the quick test 👍 |
index.php
Outdated
$thumbnailsMode = extension_loaded('gd') ? $_POST['enableThumbnails'] : Thumbnailer::MODE_NONE; | ||
if ($conf->get('thumbnails.enabled', Thumbnailer::MODE_NONE) !== Thumbnailer::MODE_NONE) { | ||
$_SESSION['warnings'][] = t( | ||
'You have enabled or changed thumbnails mode. <a href="?do=thumbs_update">Please synchonize them</a>.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/synchonize/synchronize
inc/web-thumbnailer.json
Outdated
{ | ||
"settings": { | ||
"default": { | ||
"download_mode": "HOTLINK", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not set the download mode to DOWNLOAD
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, there was actually an issue with WT where this setting would be ignored.
- add a default thumb size value (125x90px) - improve private vertical bar visual, especially with thumbnails - translations - add a sync thumbs button in tool and empty picwall page - fixes WT download mode in JSON config
Bunch of improvements for thumbnails integration:
I've changed a bit the way the orange bar for private links is displayed, mostly because it was truncated with floating thumbnails and no description. I went for an easier solution, and IMO it looks better. Example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @ArthurHoaro 🎉
Thumbnail generation and display went smoothly, I think we can merge this PR and handle improvements in future issues/PRs
Thank you @virtualtam and everyone for your time and reviews on this looong PR. Let's merge it! 🎉 |
hi, I have a problem with thumbnails.. my list of links and the detail of a link refers to the link should be my shaarli is updated like this
|
Hi guys! I've tried to refactor thumbnails, and then end up writing web-thumbnailer. It goes a bit beyond Shaarli needs but I still think we could use it as a dependency to retrieve thumbnail - see the readme for more info.
This PR shows how to integrate web-thumbnailer within Shaarli. So, what changes:
Fixes #345 #425 #487 #543 #588 #590
PS: a few things still need to be fixed, like a release on the WT repo.
EDIT: Fixes #712 - all thumbnails are now cached, no more hotlinking.
Fixes #1056