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

Plugin system #52

Closed
wants to merge 10 commits into from
Closed

Plugin system #52

wants to merge 10 commits into from

Conversation

nodiscc
Copy link
Member

@nodiscc nodiscc commented Nov 8, 2014

See https://github.com/shaarli/Shaarli/pull/52/commits descriptions for an up-to-date description

@nodiscc
Copy link
Member Author

nodiscc commented Nov 8, 2014

Note that if you try to load a plugin that is not readable/does not exist, parsing will fail. We probably want to add a check. Help very welcome. done

You can see a running demo at https://telecom.dmz.se/links/ with the current plugins enabled. back to master branch.

Next step is probably moving the thumbnailing code and QR code to plugins. done
Then, #14

@nodiscc nodiscc mentioned this pull request Nov 8, 2014
@nodiscc
Copy link
Member Author

nodiscc commented Nov 9, 2014

I've added a third party plugin for the toolbar at https://github.com/nodiscc/plugin-playvideos The plugin is now added directly to this branch

@nodiscc
Copy link
Member Author

nodiscc commented Nov 9, 2014

Rebased and updated description for clarity.

To test this, git clone https://github.com/nodisc/Shaarli; cd Shaarli; git checkout plugin-system

@qwertygc
Copy link

For icon, why not use monochrome icon or font-icon (better for create new webdesign !).

@nodiscc
Copy link
Member Author

nodiscc commented Nov 23, 2014

@qwertygc if you want to propose different icons, please do. I think these icons fit the current design of shaarli well (there are no other monochrome icons in shaarli). Your propositions are welcome.

(However this is out of scope of this pull request, we can work on styling later)

@nodiscc
Copy link
Member Author

nodiscc commented Nov 26, 2014

Having 2 config arrays is probably not the best idea. I will change it to only have one PLUGINS array, template chunks corresponding to header/toolbar/linklist/footer... items should be named tpl/plugins/$pluginname/$pluginname.{header,toolbar,linklist,footer}.html. done

Updated #14 with current https://github.com/sebsauvage/Shaarli/issues status. We're getting there. We probably want some of these features in the main code (read later bookmarklet, take note bookmarklet).

@nodiscc
Copy link
Member Author

nodiscc commented Dec 1, 2014

I added a third plugin, requested at sebsauvage#167.
It adds a "Save to Wallabag" button for each link, similar to archiveorg/readityourself that are already implemented. Moving QR code buttons to a plugin is on it's way in branch qr-plugin.

Discussion is open on whether we should include these "default" plugins in the base install or move them to separate repos (like I did with https://github.com/nodiscc/plugin-playvideos)

@e2jk
Copy link

e2jk commented Dec 1, 2014

As long as we have a smaller number of plugins, I'd prefer to keep them as part of the default installation. Otherwise they'll most likely never get used. If one day we grow to more than 50-100 plugins, we'll have to see how to improve it (e.g. categorization, etc.), but having this problem will be a sign that the software is healthy, so I'm glad to have that problem.
Plugins should be turned off by default, but I see no trouble in having the code made available.

@nodiscc
Copy link
Member Author

nodiscc commented Dec 19, 2014

QR code has been moved to a plugin (fixes #57)
The last 2 commits break javascript functionality. I still have to investigate why. Help/tesing/debugging very welcome. Please read the full commit messages.

 * 2 new global config options: LINKLIST_PLUGINS and TOOLBAR_PLUGINS
 * These options are lists of plugins that will be loaded when rainTPL renders the link list and toolbar, respectively
 * each plugin is a raintpl template in tpl/plugins/$pluginname/$pluginname.html
  * if the plugin contains multiple files, they must be self contained in it's directory
  * this allows unpacking plugins form a zip or cloning them from git
 * add readityourself plugin, requested at sebsauvage#66
  * This plugin adds, for each link, a button to display a more readable version using an external readityourself service
 * move archive.org integration as a plugin. To enable, add 'archiveorg' to the LINKLIST_PLUGINS array
 * styling fixes for plugins
 * remove archive.org plugin styling from main CSS, use icon instead
 * match archiveorg/readityourself plugins styling with qrcode
 * Icon from http://p.yusukekamiyamane.com/icons/search/fugue/icons/clock-history-frame.png under CC-BY-SA License
 * Icon from http://p.yusukekamiyamane.com/icons/search/fugue/icons/book-open.png under CC-BY-SA License
 * uses wallabag's ?plainurl action to store in wallabag
 * configuration of base wallabag URL is done through the WALLABAG_URL global variable
 * fixes sebsauvage#167
 * remove qr specific code for linklist template
 * add html item to qrcode.linklist.html
 * add the script itself to qrcode.footer.html
     * code from https://github.com/zaius/youtube_playlist/
     * Plays all Youtube videos linked from the page in an overlay HTML5 player
     * See it's README.md
 * tpl/plugins/*/*.toolbar.html will add elements to the toolbar
 * tpl/plugins/*/*.linklist.html will add elements to the linklist
… failing

 * each plugin should have .toolbar.html and .linklist.html else RainTPL fails at finding the file and stops parsing
nodiscc and others added 3 commits December 25, 2014 01:30
 * add plugin parsing loop in footer (required to include the qrcode script)
 * move javascript in it's own qrcode.js file (work on #33) and load it from qrcode.footer.html
 * enable qr plugin by default
 * move qr icon in plugin directory
 * add empty qrcode.toolbar.html to prevent raintpl parsing failure
 * Rendering the linklist has been moved to a separate function (to allow greater customisation if the need arises)
 * A PageBuilder is now created every time we render the page. Although it is not always page settings, like we'll do with plugins. Also, this removes the potential errors when forgetting to create a pageBuilder and so on.
 * We now build a list of plugins for each location (for now: toolbar, linklist, footer) so that RTPL has no issues rendering them
 * Simplified plugins templates. Linklist templates can now access the link info through the  variable, while toolbar and footer can access global settings via .
 * This means we don't have to create empty files anymore to prevent raintpl from crashing
@nodiscc
Copy link
Member Author

nodiscc commented Dec 30, 2014

QR code has been properly moved as a plugin and the implementation is much cleaner thanks to @pikzen. I'll move the ATOM button as a plugin, and move thumbnailers to a plugin (by the way it will be a generic plugin system, not just a few hardcoded sites).

Then I'll rebase/squash my changes to get clean commits and we may proceed to merge the changes.

@Alkarex @ArthurHoaro @BoboTiG @e2jk @Sbgodin @sebsauvage @timovn what do you think?

@@ -30,7 +30,7 @@
<ul>
{loop="links"}
<li{if="$value.class"} class="{$value.class}"{/if}>
<a name="{$value.linkdate|smallHash}" id="{$value.linkdate|smallHash}"></a>
<a name="{$value.shortlink}" id="{$value.shortlink}"></a>
Copy link
Member

Choose a reason for hiding this comment

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

I think you meant shorturl, because shortlink doesn't exist.

@ArthurHoaro
Copy link
Member

Since there is no PHP involved, it's more a theme extension in my opinion. But that's pretty good and easy to handle.

There is one little thing that I don't like, it's the way you inject your variable in the wallabag plugin. You used the global Shaarli settings file and I think it should be handle by the plugin files.

If you don't want to involve PHP in plugins, you could either use JSON, YAML or even a flat key/value plugin setting file that you load around here if it exists and assign it to RainTPL.

@nodiscc
Copy link
Member Author

nodiscc commented Jan 9, 2015

Since there is no PHP involved, it's more a theme extension in my opinion

Yes mainly changes to the default shaarli template.

You used the global Shaarli settings file and I think it should be handle by the plugin files.
you could either use JSON, YAML or even a flat key/value plugin setting

Yes.
Yes.

Thanks for the review, it's in no way finished yet and I'll update this pr when i get some time (thumbnailers, a few plugins from #14, store plugin settings in plugins directory, use ini-style config, the logic should stay the same if you like it)

@nodiscc nodiscc added the cleanup code cleanup and refactoring label Jan 9, 2015
@nodiscc nodiscc added this to the 0.9beta milestone Feb 17, 2015
@nodiscc
Copy link
Member Author

nodiscc commented Feb 23, 2015

I started some new branches to deal with this as it was difficult to rebase, and the example plugins and plugin system itself should not be entangled on the same branch:

https://github.com/nodiscc/Shaarli/tree/new-plugin-system
https://github.com/nodiscc/Shaarli/tree/wallabag-plugin
https://github.com/nodiscc/Shaarli/tree/readityourself-plugin

QR and archive.org plugins are ready to roll. I'll fix the point mentioned by @ArthurHoaro (plugins should have their own config file), and add plugin parsing to the description area so that someone can possibly add a markdown rendering plugin there.

Closing this, I'll submit another pull request. Your patches to my branches are welcome, I can push them to this repo if you want.

@nodiscc nodiscc closed this Feb 23, 2015
@virtualtam virtualtam modified the milestones: 0.9.0, 0.5.0 Jul 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup code cleanup and refactoring in progress plugin bells and whistles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants