Item14092: attach.pattern.tmpl needs a hook for plugins to add properties.

-- PhilippeKehl - 11 Jun 2016

Plugins that use beforeUploadHandler() or afterUploadHandler(), such as my PhotoGalleryPlugin, may want to introduce additional properties (request parameters) to manipulate uploads.

The PatternSkin doesn't provide a hook to add <form> elements but one instead must override the whole properties definition from the attach.pattern.tmpl template. This will be a problem when the PatternSkin's definition of the checkboxes changes and the plugin that overrides the section doesn't update.

I think the fix is as easy as adding a %TMPL:P{"propertieshook"}% to the properties definition that plugins can use to inject their own stuff. Using %TMPL::PREV% this can even work for multiple plugins. This can be documented in the attach.pattern.tmpl file directly.

So attach.pattern.tmpl adds %TMPL:P{"propertieshook"}%:

%TMPL:DEF{"properties"}%<div class="foswikiFormStep">
---+++ %MAKETEXT{"Properties"}%

<input type="checkbox" class="foswikiCheckbox" id="createlink" name="createlink" %ATTACHLINKBOX% /><label for="createlink">%MAKETEXT{"Create a link to the attached file"}%</label> <span class="foswikiGrayText">%MAKETEXT{"Images will be displayed, for other attachments a link will be created."}%</span>

<input type="checkbox" class="foswikiCheckbox" id="hidefile" name="hidefile" %HIDEFILE% /><label for="hidefile">%MAKETEXT{"Do not show attachment in table"}%</label> <span class="foswikiGrayText">%MAKETEXT{"Attachments will not be shown in topic view page."}%</span>
%TMPL:P{"propertieshook"}%</div>%TMPL:P{"changepropertiesaction"}%%TMPL:END%
%{
    The "propertieshook" allows skins to add properties, e.g. by plugins that
    use the beforeUploadHandler() or afterUploadHandler() functions to handle
    additional properties. Use the %TMPL:PREV% to make those _add_ to the
    "propertieshook" placeholder (instead of replacing it) so that multiple
    skins can add their properties. E.g.:

    %TMPL:DEF{"propertieshook"}%%TMPL:PREV%

        <input type="checkbox" class="foswikiCheckbox" id="someprop"/><label for="someprop">%MAKETEXT{"Some property description."}%</label>
    %TMPL:END%
}%

And plugins or other skins (e.g. attach.photogallery.tmpl) can do something like this:

%TMPL:DEF{"propertieshook"}%%TMPL:PREV%

<input type="checkbox" checked class="foswikiCheckbox" id="exifrotateimage" name="exifrotateimage"/> <label for="exifrotateimage">%MAKETEXT{"Rotate JPEG images based on EXIF tags (using <code>exiftran</code>)."}%</label>

<input type="checkbox" checked class="foswikiCheckbox" id="setexifdate" name="setexifdate"/> <label for="setexifdate">%MAKETEXT{"Set file upload date to EXIF exposure date for JPEG images."}%</label>
%TMPL:END%

Pull request filed: https://github.com/foswiki/distro/pull/13

-- PhilippeKehl - 11 Jun 2016

MichaelDaum ... Could you review this? Does it rise to the level of a FeatureRequests or is it simple enough to add on a task.

-- GeorgeClark - 12 Aug 2016

Makes sense. No beauty, but if it helps...

I would have less fear to redefine all of properties in a skin overlay.

An alternative approach being used in other parts of the skin is to split up properties like this:

%TMPL:DEF{"properties"}%%TMPL:P{"properties::start"}%%TMPL:P{"properties::createlink"}%%TMPL:P{"properties::hidefile"}%%TMPL:P{"properties::end"}%%TMPL:END%

%TMPL:DEF{"properties::start"}%
<div class="foswikiFormStep">
---+++ %MAKETEXT{"Properties"}%
%TMPL:END%

%TMPL:DEF{"properties::createlink"}%
<input type="checkbox" class="foswikiCheckbox" id="createlink" name="createlink" %ATTACHLINKBOX% /><label for="createlink">%MAKETEXT{"Create a link to the attached file"}%</label> <span class="foswikiGrayText">%MAKETEXT{"Images will be displayed, for other attachments a link will be created."}%</span>
%TMPL:END%

%TMPL:DEF{"properties::hidefile"}%
<input type="checkbox" class="foswikiCheckbox" id="hidefile" name="hidefile" %HIDEFILE% /><label for="hidefile">%MAKETEXT{"Do not show attachment in table"}%</label> <span class="foswikiGrayText">%MAKETEXT{"Attachments will not be shown in topic view page."}%</span>
%TMPL:END%

%TMPL:DEF{"properties::end"}%</div>%TMPL:P{"changepropertiesaction"}%%TMPL:END%

A skin overlay would then redefine properties and add a foobar property using this:

%TMPL:DEF{"properties"}%%TMPL:P{"properties::start"}%%TMPL:P{"properties::createlink"}%%TMPL:P{"properties::hidefile"}%%TMPL:P{"properties::foobar"}%%TMPL:P{"properties::end"}%%TMPL:END%

Just my 2cent.

-- MichaelDaum - 15 Aug 2016

George and Michael, many thanks for your help and comments!

Michael: Your proposed approach is more elegant than the current approach I take (reproducing the whole markup in my skin) but it still requires my skin to "know" what properties::* it should reproduce.

And when there are two skins (plugins) that both want to add properties, it wouldn't work (would it?). Only the "last" (highest priority) skin's props would get added.

Is it the %TMPL:PREV% that you dislike?

To me SkinTemplates and ReleaseNotes01x01 suggests this use. The only place I can find where it's used today is in templates/compare.tmpl to add CSS styles (not sure why it's done that way, though).

Given that I want to support multiple skin overlays adding properties, and not reproducing the original stuff, the only alternative approach I can think of is using some JS magic to move the added markup to the right place run-time.

I could live with redefining all of properties in my skin, no problem. But I thought providing some mechanism to add checkboxes would be beneficial.

What do you think?

-- PhilippeKehl - 15 Aug 2016

%TMPL:PREV is an excellent extension to the TMPL language. However it only is applicable extending a previous definition in a linear fashion. In the above case we'd like to extend the inner part.

Let's try another structure to facilitate %TMPL:PREV:

%TMPL:DEF{"properties"}%<div class='foswikiFormStep'>%TMPL:P{"properties::content"}%</div>%TMPL:P{"changepropertiesaction"}%%TMPL:END%

%TMPL:DEF{"properties::content"}%
%TMPL:P{"properties::title"}%
%TMPL:P{"properties::createlink"}%
%TMPL:P{"properties::hidefile"}%
%TMPL:END%

%TMPL:DEF{"properties::title"}%---+++ %MAKETEXT{"Properties"}%%TMPL:END%

%TMPL:DEF{"properties::createlink"}%...%TMPL:END%

%TMPL:DEF{"properties::hidelink"}%...%TMPL:END%

Your skin can now redefine properties::content by making use of %TMPL::PREV in a safe way:

%TMPL:DEF{"properties::content"}%%TMPL:PREV%%TMPL:P{"properties::foobar"}%%TMPL:END%

The idea here is to structure the elements in a semantic way so that they make sense on their own. A propertieshook definition will be empty in almost all cases except yours. Only then will it make sense.

-- MichaelDaum - 16 Aug 2016

PhilippeKehl, if you could restructure the pull request I'll get your changes merged into master. I'm setting this task as planned for 2.2. Thanks.

-- GeorgeClark - 16 Aug 2016

Many thanks for your comments and suggestions. I see your point and I agree.

I have restructured the the pull request accordingly. See here for the diff: https://github.com/foswiki/distro/pull/13/files

It differs slightly from Michael's code (s/hidelink/hidefile/, linefeeds). It produces the exact same HTML as before.

-- PhilippeKehl - 16 Aug 2016

George, I hope it's okay that I've now set this to "Needs merge" and "Waiting for" (you). Thanks!

-- PhilippeKehl - 06 Sep 2016

Commenting is disabled while not logged in