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

allow setting custom thumbnails #361

Closed
wants to merge 8 commits into
base: master
from

Conversation

6 participants
@wazari972

wazari972 commented Oct 14, 2015

Hello there,

this patch adds a field in the "edit link" page to allow setting a custom thumbnail for the link. If no address is provided, the behavior remains as before. If a link is provided, the thumbnail is generated with the existing functions.

The use-case is for creating a personal "virtual library" based on Shaarli, with a link to the book page and its cover as thumbnail.

what do you think about it?
I looked at #153 and it seems to be compatible, but I didn't apply that patch on my tree.

best regards,

Kevin

@nodiscc

This comment has been minimized.

Member

nodiscc commented Oct 14, 2015

I think this should be optional as we don't want to clutter the editlink dialog. This looks like a good fit for a plugin. @wazari972 Did you have a look at the plugin system proposition. Though I don't think it is possible to add a field through the plugin system yet.

@ArthurHoaro is this right?

@kpouget

This comment has been minimized.

kpouget commented Oct 15, 2015

hello,

yes, that makes sense. I tried to look at some of the plugin pull requests, but the "files changed" page seems ...cluttered ? https://github.com/shaarli/Shaarli/pull/280/files and #284 both seem to contain the whole patch to integrate plugins, so I'm not sure where to look ...

if there is a patch more or less ready for plugin integration, I can try to adapt my pull request to it. I can also (try to) add the ability to add fields to the editlink dialog, that looks like a good thing to have to me

@ArthurHoaro

This comment has been minimized.

Member

ArthurHoaro commented Oct 15, 2015

Though I don't think it is possible to add a field through the plugin system yet.

Actually, it is. See here and here.

@kpouget plugin PRs contains the whole patch because I need to rebase each one individually.

@nodiscc nodiscc added the in progress label Oct 15, 2015

@wazari972

This comment has been minimized.

wazari972 commented Oct 17, 2015

Hello, I've updated my code with @ArthurHoaro plugin system (cf last commit).

I kept everything else untouched, because I think it doesn't really belong/fit in the plugin system, that's a modification of the core functions.
The only remaining template modification is thumbnail($value['url']) to thumbnail($value['url'], isset($value['thumb']) ? $value['thumb'] : False), but I think that's the only simple way. Another solution would be to change it to thumbnail($value) ($value is $link in index.php). That would be more generic, but involves more modifications to index.php and the templates.

@ArthurHoaro

This comment has been minimized.

ArthurHoaro commented on index.php in 43093e9 Oct 20, 2015

Coding style:

  • avoid ternary operator when it's not use for affectation, to improve readability.
  • we use lowercase booleans.
@ArthurHoaro

This comment has been minimized.

Member

ArthurHoaro commented Oct 20, 2015

Well, I don't really like the idea of a plugin affecting the core code. But as you said, it's not easy to do otherwise because computeThumbnail is called everytime and thumbnails aren't stored in datastore.php.

Other contributors opinion would be nice.

Also could you rebase your PR?

@nodiscc

This comment has been minimized.

Member

nodiscc commented Oct 20, 2015

Regarding custom thumbnails and the discussion on gitter (which plugins do we enable by default):

Is it possible to move thumbnailing to a plugin? This would allow several options, just by enabling one plugin:

  • Use the default, legacy thumbnailer (need to port current thumbnailer to a plugin)
  • Use an improved generic thumbnailer (need to adapt this patch to work as a plugin)
  • Use custom thumbnails (proposed here)
  • Use favicons (Based on you favicon extractor?)
  • Disable thumbnails (no thumb. plugin enabled)
  • Others (website screenshots...)

The legacy thumbnailer plugin would be enabled by default.

I don't really like the idea of a plugin affecting the core code

Me neither, that's why I suggest making thumbnailers independent from the core.

@wazari972

This comment has been minimized.

wazari972 commented Oct 20, 2015

Hello,

I've fixed the coding style, I just let the ternary operator in the template, I don't think I can add a variable there.

Also could you rebase your PR?

not sure what you mean here ... ?

regarding the architecture, you'll let me know what you decide.

Does the plugin system already support adding/binding information to a link? that would be one more step to reduce the footprint of my patch on the core structure

@rubykat

This comment has been minimized.

rubykat commented Nov 9, 2015

Setting custom thumbnails is a feature I want so much that I already hacked my own version. (See https://github.com/rubykat/Shaarli/tree/craftmarks). I know my hack is unsuitable for merging back into main, which is why I haven't mentioned it before. Figured it would be better implemented as a plugin, if that were possible.

The downside is, so far as I can see, custom thumbnails require altering core code, because you need to store the URL of the custom thumbnail -- whereas the existing thumbnail code deduces the desired thumbnail URL from the bookmark URL (for selected sites where that is possible, such as youtube).

@virtualtam virtualtam added this to In Review in Refactoring Jul 29, 2017

@virtualtam virtualtam modified the milestone: 0.10.0 Aug 3, 2017

@virtualtam virtualtam moved this from In Review to In Progress in Refactoring Oct 22, 2017

@ArthurHoaro

This comment has been minimized.

Member

ArthurHoaro commented Jul 5, 2018

Closing as this PR is way outdated.

@ArthurHoaro ArthurHoaro closed this Jul 5, 2018

@virtualtam virtualtam moved this from In Progress to Done in Refactoring Jul 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment