Plugin config2

[a4z]

suggestion for #201 ,

please review, and say if this is OK.

I would like to add a likewise option for enable-plugin-pfd, to be able to load the default pdf plugin without adding it to the custom plugins, but before doing any further adjustment it would be create to get some feedback

[a4z]

and I feel somehow innocent for the stalled build failure of Job #567.1 , also because all other seem to be fine

[a4z]

re-triggered build, now everything is green :)

[a4z]

now I destroyed the tests again :(
sorry, havn't thought that on the pull request when I did the update in my branch

but it's just an additional empty text line that is now there so I think I can fix the tests

interesting that jruby has no problem with this!

[a4z]

again green, all plugins that are in reveal js can now be turned off or on,
of there are other plugins, adding the in the same way will work

[mojavelinux]

In AsciiDoc, a blank value is equivalent to "true", so I would recommend:

:omit-plugin-marked:

Then you just check if it is set (e.g, attr? 'omit-plugin-marked'). The value doesn't matter.

[obilodeau]

I made the change suggested by @mojavelinux. Thanks for the reminder about attributes by the way.

I gave this a serious look and one last thing I have to say before I merge is about consistency.

Everything we have done so far that was not already an AsciiDoc[tor] standard has been namespaced using the revealjs_ prefix. Additional plugin configuration is no exception: https://github.com/asciidoctor/asciidoctor-reveal.js#additional-plugins

Now we introduce new omit-plugin-[name] and enable-plugin-[name] that don't follow this convention.

May I rename the attributes to: revealjs_plugin_[name]_disable and revealjs_plugin_[name]_enable?

Thoughts, concerns?

[mojavelinux]

I made the change suggested by @mojavelinux. Thanks for the reminder about attributes by the way.

👍

May I rename the attributes to: revealjs_plugin_[name]_disable and revealjs_plugin_[name]_enable?

Since these states are mutually exclusive, that's when I'd propose switching to a single attribute with a true boolean value (:revealjs_plugin_[name]: disable|enable).

The empty attribute convention in AsciiDoc is more for when you want to just flip on a built-in setting or flip it off, modifying it from its default state. But since in this case we don't know all the possible plugin names upfront, and some are on by default and others not, I think an explicit value might actually be more appropriate and user-friendly.

[a4z]

Thanks for all the feedback!

for me the names don't matter, as long as I have the functionality,
(just need to adopt 2 or 3 talks I already made with my branch)
also if I add a true as initial value or not for some attribute(s)
this is the reason why I asked for feedback, I don't know the conventions, but would like to deliver thing that need as less adoption as possible.

we know the available plugins upfront, those are that delivered by reveal.js itself
this PR doesn't handle anything other than the default reveal.js plugins shipped with reveal.js

an, also somehow, important question, at least for me is, do you want me to make the changes?
if so, please be prepared for some, maybe even longer, waiting time, don;t know currently how my schedule looks like, but for sure not as I wish it would

however, the, now available, print-pdf functionality is great.
but there is, will be, an follow up issue with the page numbering, and this is something I would love to be fixed as soon as possible because currently my backup/presentation pdfs lose the page number.

[obilodeau]

:revealjs_plugin_[name]: disable|enable changes pushed with documentation updates. I added a test case where we disabled and enabled one plugin too.

I'm ready for the merge at this point. But before we merge, shouldn't we disable marked and markdown plugins by default? Who is ever going to combine both given how intrusive we are in the toolchain, making these plugins realistically unusable...

[mojavelinux]

@a4z don't worry, we're figuring this all out together. Sharing insight about conventions is where I happen to think I can add the most value, so I try to chime in when I can.

we know the available plugins upfront, those are that delivered by reveal.js itself this PR doesn't handle anything other than the default reveal.js plugins shipped with reveal.js

Got it. That makes sense.

I guess where my comment was headed is that using AsciiDoc boolean-stlye attributes (e.g., revealjs_plugin_[name] for on and !revealjs_plugin_[name] for off) is tricky. That's because in order to unset an attribute, it has to be set in the first place, which a converter isn't really set up to do. And that's why I think using an explicit enable|disable value is more workable.

[a4z]

amazing, thanks Oliver for implementing those changes!

and the :revealjs_plugin_[name]: enabled/disabled is so much nicer
it is a pleasure to see you and Dan working on this and creating such improvements!

I'm ready for the merge at this point. But before we merge, shouldn't we disable marked and markdown plugins by default? Who is ever going to combine both given how intrusive we are in the toolchain, making these plugins realistically unusable...

I would love to see this because the natural thing in the context of asciidoctor-reveal.js is disabling those both plugins.
And even if they do not harm if they are not disabled, it is extra loading time and text in browser, so I like to disable them, and this is a bit of default attributes boilerplate in every presentation.