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

add TEMPLATE_DOKUWIKI_PAGETOOLFLOAT_DISPLAY event to template #236

Merged
merged 2 commits into from Aug 1, 2013

Conversation

splitbrain
Copy link
Collaborator

This adds a custom event to the 'dokuwiki' template that allows plugin
authors to easily integrate custom pagetool buttons into the template.

This adds a custom event to the 'dokuwiki' template that allows plugin
authors to easily integrate custom pagetool buttons into the template.
@selfthinker
Copy link
Collaborator

In general it would be very good to have something like this. But there are 3 issues with this approach:

  1. The name TEMPLATE_DOKUWIKI_PAGETOOLFLOAT_DISPLAY should not have the "float" bit in it, because that the page tools float is purely a style and behavioural thing.
  2. I'm not sure if there is a better and realistic solution, but I'd prefer this to be in the core and not in the template. It would also make other templates more future-proof if we provided some tpl_pagetools() (and tpl_sitetools() and tpl_usertools()). That way in can also be integrated into the tpl_actiondropdown(). The only bad thing about adding it to the core is that template authors would have either less freedom in ordering the links or adding custom stuff, or they would do it like they are doing it now and would lose on having these events and they'll have potentially more to maintain.
  3. Although the page tools are the most important into which other plugins would want to insert their own links, the site tools and user tools should have such an event as well.

@splitbrain
Copy link
Collaborator Author

The name was chosen this way because it's not a core event. It's an event for this one particular template and in there the pagetools float. Other templates will have different pagetools and I wanted to make that absolutely clear.

IMHO this should not be in the core.

Template authors should have full design control and can decide what and how things are exposed via action hooks.

Plugin authors should decide if and which templates they want to support out of the box by implementing the appropriate template specific hooks and implementing the according styles to match the template's design.

Additional hooks can be added later.

@selfthinker
Copy link
Collaborator

The page tools only float for you, because you have CSS enabled. And as there are only one set of page tools, there is no harm in dropping the "float".

If there is something specific for this template, then I agree, it should only be in this template. But many templates have the tools split up in exactly those categories, because they make sense.

Why should a plugin be able to hook into one template and not another? It will get really messy when plugins need to support 50 different hooks which are doing all the same, e.g. TEMPLATE_DOKUWIKI_PAGETOOLS_DISPLAY, TEMPLATE_ARCTIC_PAGETOOLS_DISPLAY, TEMPLATE_VECTOR_PAGETOOLS_DISPLAY, TEMPLATE_MONOBOOK_PAGETOOLS_DISPLAY, TEMPLATE_DOKUBOOK_PAGETOOLS_DISPLAY, TEMPLATE_STARTER_PAGETOOLS_DISPLAY, TEMPLATE_LCARS_PAGETOOLS_DISPLAY, ...

I agree that other hooks don't need to be part of this PR.

@Klap-in
Copy link
Collaborator

Klap-in commented Jul 21, 2013

Sofar i experienced adding the pagetools manually by the wiki administrator as messy. So i like integration in core.

But because templates differ, i think that an implementation in the core requires that the images that are supplied should have a default, but that a wider set of template specific images can also be supplied via the plugin ...

@selfthinker
Copy link
Collaborator

The images are template-specific, so shouldn't be set (or be made available to overwrite) by the core. I agree that a generic default image makes sense (even independent of this PR).

But template-specific images provided by plugins? Or plugin-specific images provided by templates? Both sound dodgy, although the latter a bit less... And if both provide them, which one wins?

@Klap-in
Copy link
Collaborator

Klap-in commented Jul 21, 2013

I should prefer configuring images per plugin, so that a plugin manager has only one place to update it.
When people like special images for their template then they share their images(at least) hopefully at the plugin page. Like now the 'add-button' instructions are.
I think that the including in the template is too far from the 'user' of the images.

And when the plugin author maintains his/her stuff, it will probably be included in the plugin too.

@Chris--S
Copy link
Collaborator

Chris--S commented Aug 1, 2013

  1. change event name to 'TEMPLATE_'.str_toupper($conf['tpl']).'_PAGETOOLS_DISPLAY'.
  2. add a comment, if your template is similar enough to dokuwiki, you should replace $conf['tpl'] with 'DOKUWIKI'

also made it more secure against copy'n'paste
Chris--S added a commit that referenced this pull request Aug 1, 2013
add TEMPLATE_DOKUWIKI_PAGETOOLFLOAT_DISPLAY event to template
@Chris--S Chris--S merged commit 7a1c09e into master Aug 1, 2013
@selfthinker
Copy link
Collaborator

Just thinking further about this... Does it make sense to have the same event in main.php and detail.php? I'd say 98% of all plugins would not want to add anything to the pagetools of the detail page. If you'd want to support it, they should get a different name, as those two are not the same...

splitbrain added a commit that referenced this pull request Feb 6, 2017
This will open up the discussion from #236 again and I'm not sure how to
solve this best.

The TEMPLATE_PAGETOOLS_DISPLAY event is very specific to the dokuwiki
template in theory. In practice many other templates implemented not
only the same event but also use the same HTML (and often even the same
CSS). Which makes the event more like a core event.

This branch now changes the HTML the event expects back from handlers.
When merged it would immeadiately break all plugins implementing this
event (and by broken I mean the layout/design of the template breaks).

Since the expected data changes, I would argue this should be a new
event or at least be implemented in a way to not break the design by
installing an old plugin and by giving the plugin a chance to know if
it's running on the old or the new code.

This is what this commit does. By changing the view names, old plugins
should not display (because they hopefull do not handle those views) but
gives them an easy way to handle the old and new views in one plugin.

However, I'm not perfectly happy with it, yet.

The way I implemented the new SVG handling is by means of a new class
dokuwiki\template\dokuwiki\tpl which provides a pageToolAction() method.
Plugins could use this method to easily return the proper HTML for the
pagetool items and we could adjust this method later on to make
adjustments to the pagetools without breaking anything again.

However this method is template specific again. While it would possible
for plugins to use this method even when the wiki runs another plugin
emitting the event, it would be a bit weird.

A better way would be for the event to not expect HTML at all, but
instead a data structure of the data currently passed to
pageToolAction(). Templates could then decide to implement the event,
but still render the data in a completely different way...

OTOH this means plugins are no longer free to add whatever they want
into the pagetools. We once argued plugins might want to add submenus or
other fancy stuff there. But in fact no plugin ever did.

If we decide to go this new way of making the event more of a first
class citizen of template development, then we would probably have to
move parts or all of the tpl class back to the core.
@splitbrain splitbrain mentioned this pull request Feb 6, 2017
5 tasks
splitbrain added a commit that referenced this pull request Apr 9, 2020
splitbrain added a commit that referenced this pull request Apr 9, 2020
show display value in diffs. fixes #236
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