WeBWorK: option to display all tasks upon page load

[Alex-Jordan]

You can test this by making the sample chapter. Only change should be that I made the order of some options in Scaffold::Begin() more logical (imho).

Then if you change the publisher file to use task-reveal="all" and build again, the behavior of tasks will change. It will still be the case that only the first one is actually open upon page load. But they will all be clickable to open them up.

[rbeezer]

Visual review looks good, but it needs a few more things.

• Please catch erroneous attempts to set the publisher variable by echoing the bad value, stating the correct ones, and informing of the default value that will be used instead. See examples throughout.

• Prior is more reliable if the default is set early as a variable and used in error messages, and the otherwise.

• Pubisher variables do not go in without complete documentation. Add to reference section (in the terse style used there) and make cross-references to/from the material you have already written. See examples throughout.

[Alex-Jordan]

Have yet to work on the last bullet. But I wanted to run this most recent commit by you. I added utilities to simplify setting of a variable value from a pubfile attribute, when a default exists, and there is a finite list of options none of which contain a space. Does it help to keep this as a prototype and some other PR could unify other such value setting?

[Alex-Jordan]

If the core idea is worth keeping, I could streamline it before it propagates. Like instead of having a default, we could assume the first option from options is the default.

And maybe there is a way to unify attribute and attribute-location.

[Alex-Jordan]

Now with more documentation.

[rbeezer]

I like this machine, but have a lot of suggestions.

• I'd like to avoid named templates, since they have an implicit context. I'm getting rid of them as I come across them. So pass in the actual attribute via a select and match on @* for a bit of type-safety. That makes the @attribute parameter redundant.

• This is for developers only, so I think it is safe to make the first item in the list be the presumed default. Just put into careful code comments once this is stable. Another parameter gone.

• I think we want flexibiity with @attribute-location so lets leave that as-is.

• Isn't test="$pubvalue = $default or not($pubvalue)" unnecessary? First half happens in the next when, and second half happens in otherwise?

• options-list template is nicely done, but with tokenize() I think it is simple enough to incorporate in the primary template. (And I'd like to move to this, another gradual thing I'm doing). In -common find <xsl:template match="list-of"> (a named template!). See $elements formed with tokenize(). My line 6855 has

  <xsl:variable name="by-name-elements" select="boolean(.//*[local-name(.) = $elements])" />

  where a string (local-name(.)) is compared with the tokenized list ($elements) to see if the string is one of the tokens. Much like a Python in operator, but with a less-strict notion of equality. See, e.g., http://zvon.org/xxl/XSLTreference/Output/operator_equal.html

[Alex-Jordan]

I'm getting new GitHub behavior that I do not care for!

If I am here composing a message and I switch tabs (I mean the tabs within this PR page) to look at the code changes for a sec, and then switch back, everything I had drafted is gone.

So I had things drafted here that are just gone. Do you get the same behavior today?

Too fed up to rewrite everything. I will just make a best call where I had clarification questions.

[Alex-Jordan]

Here is a consequence of using a modal template instead of a named template. A particular publisher attribute may or may not exist in the publisher file tree. (Maybe it's just omitted from the publisher file.) We want the corresponding publisher variable to always exist. So when you are building the corresponding variable, you cannot simply match on a certain attribute, because that attribute may not exist.

• The variable template could have an xsl:choose to do something different when the attribute does not exist. This lengthens/complicates the code since all the xsl:variable would have to get this xsl:choose.
• A first pass on the publisher file tree could add empty string attributes for everything. That sounds like a big deal conceptually, but I imagine it would not be too many lines of code.

Do you have a preference, or see a better approach?

[Alex-Jordan]

I've found a nice (depends on perspective?) way to do this using the evaluate function from xmlns:dyn="http://exslt.org/dynamic".

[Alex-Jordan]

What I have working has top level structure like this:

<xsl:variable name="webwork-task-reveal">
    <xsl:variable name="attribute-path" select="'webwork/@task-reveal'"/>
    <xsl:apply-templates select="$publication" mode="set-pubfile-variable">
        <xsl:with-param name="attribute-path" select="$attribute-path"/>
    </xsl:apply-templates>
</xsl:variable>

Note that it applies templates to $publication. We don't know if $publication/webwork/@task-reveal exists, so I can't match on that. But inside the template matching the publication root from $publication, I can test on the parameter $attribute-path to see if the attribute was actually there in the publication file. And proceed accordingly.

No named templates anywhere. The "machine" only needs developers to individualize things like the name="webwork-task-reveal" and select="'webwork/@task-reveal'" above, and further down there is a centralized template for listing the defaults that pair with 'webwork/@task-reveal'.

Also this uses exlt's dyn:evaluate to turn a string like `'webwork/@task-reveal' into the actual attribute node when that is needed.

Any red flags?

[Alex-Jordan]

OK, ready for a review.

• No named templates.
• Uses a key and a separate file to map paths to publisher file attributes to an official list of allowed values.
• Uses dyn:evaluate() to turn a string like 'webwork/@task-reveal' into the corresponding Xpath expression.
• Uses str:tokenize() as suggested.

[rbeezer]

Publisher file generally makes a variety of global variables. That's why we can poll attributes which may not exist. PR has a variable being created (webwork-task-reveal) and then immediately defines a local variable, attribute-path. Why not define the putative attribute itself as a variable? Can it become the subsequent select if it is empty? (And is the existing variable strictly necessary?)

dyn:evaluate() reminds me of exsl:node-set() which I try to use sparingly. And I think I now know (as of two days ago) how to remove many of the most important uses of those (i.e. ones I used to think would be hard to remove). So I'd like to reduce reliance on these ad-hoc extensions.

I'm still not 100% comfortable with keys, but do want to make more use of them, for convenience and for efficiency. The localizations were once "stored" in a variable. Can you experiment with moving the mapping into a variable that lives inside the stylesheet? Just to make things more self-contained. I think you do a "for-each" on the variable to switch context in order to process. Or see a sort-of internal document() in -common by searching for mb:programming. The mb was because I was copying something from long, long ago. It may not be needed, or it could switch to pi:.

<xsl:when test="following-sibling::*/following-sibling::*">

I'll say this borders on unreadable code. At least I do not understand why it "works" (I understand the purpose). Since $options is manufactured, we know exactly what it looks like (rather than being author's source with weird stuff in it). THEN the position() function is no longer dangerous. The filters/tests like position() = 1 or position() = last or position() > 2 are possible. And the tests nearby can use token in place of * makes for more type safety.

[Alex-Jordan]

OK, try this out. Still using dyn. If that is a deal-breaker, read to the end.

1. We make a tree inside the style sheet that is supposed to reflect the available options for publisher variables. See line 3788.
2. This tree always has the attribute we want to work with, so the modal template can match on that with certainty.
3. Use a recursive strategy to back up the tree from this attribute in order to get a string path (that we can print later to guide people in the warning message).
4. We have this string path anyway, so we can use dyn:evaluate() to get the corresponding attribute from the actual publisher file. Do the comparison against the option list as before.

I rebased off master to catch up, just in case.

What I like about using dyn is the lack of redundancy. In this, the stylesheet only has one instance of webwork/@task-reveal. I've gone round and round, and every way I think to do this without dyn, the stylesheet would need to have webwork/@task-reveal typed twice. It feels inelegant. but without dyn, it could still work by passing the (potentially empty) publisher file attribute as a param. So what is currently:

<xsl:variable name="webwork-task-reveal">
    <xsl:for-each select="$publisher-attribute-options/webwork/@task-reveal">
        <xsl:apply-templates select="." mode="set-pubfile-attribute-variable"/>
    </xsl:for-each>
</xsl:variable>

would become something like

<xsl:variable name="webwork-task-reveal">
    <xsl:for-each select="$publisher-attribute-options/webwork/@task-reveal">
        <xsl:apply-templates select="." mode="set-pubfile-attribute-variable">
            <xsl:with-param name="attribute" select="$publication/webwork/@task-reveal"/>
        </xsl:apply-templates/>
    </xsl:for-each>
</xsl:variable>

[Alex-Jordan]

Sorry, I forgot to git rm the separate xml file.

[Alex-Jordan]

This can all be squashed of course when the time comes.

[Alex-Jordan]

Force pushed after rebasing and consolidating to one commit

[rbeezer]

Finally back to this, and I think I like it! Quite entertaining. ;-) Want to be in a better position to react if there are any hiccups, and I also have another application to test this with myself. So plan to merge in a few days. In the meantime, questions:

• Document that the pi:publisher tree is organized with defaults first? (That is buried in a routine.)
• select="document('')/*/pi:publisher" - do we know what * is? Should we not just say so?
• <xsl:template match="*" mode="quoted-list"> - match is on token, no? Say so?
• <xsl:apply-templates select="text()"/> - confused me for a bit. <xsl:value-of select="."/> instead?

Looking good. Nice work. But given eventual wide applicability, I want to make sure we get as many details right as we can.

[Alex-Jordan]

Document that the pi:publisher tree is organized with defaults first? (That is buried in a routine.)

I'm sorry, I don't understand. Line 3783 has the explanation, and that comes right before the pi:publisher tree. What is the comment that is buried that should move up?

select="document('')//pi:publisher" - do we know what is? Should we not just say so?

I changed it. I was just grabbing the pattern from mb:programming without thinking.

- match is on token, no? Say so?

Changed this too. At the time, I didn't understand if the result from tokenize was a collection of sibling nodes, or also included some kind of parent/root for them. The * was originally to capture whatever the parent would have been.

- confused me for a bit. instead?

Done. Also a remnant of a rudimentary understanding of tokenize.

In addition to the above, I don't think there was a need for the for-each, so I removed that. Tested and behaves as it should for various entries (or lack of) for task-reveal in the publisher file. All rebased off current master and force pushed.

[rbeezer]

Very nice. Looking forward to using this. I split it into some topical commits.

Document that the pi:publisher tree is organized with defaults first? (That is buried in a routine.)

I'm sorry, I don't understand. Line 3783 has the explanation, and that comes right before the pi:publisher tree. What is the comment that is buried that should move up?

I've added the fact that the default must be first. Someone adding an option might not see that buried down in the code.

Also, I edited the documentation so that the cross-reference to the reference section went right to the option in question (not the whole chapter), and then removed the duplication of the specific options at the point of the cross-reference (so they would not go out of sync).

Next steps:

• Your call on a -announce post for @task-reveal. I don't put every last change there, so this one is up to you.

• Would you like to find two or three high-profile switches to move over to this new infrastructure? So we get some good testing?

[Alex-Jordan]

I've added the fact that the default must be first. Someone adding an option might not see that buried down in the code.

Smh. While I was doing this, my head kept muddling the singular "default" option with the plural list of legal "options". I did this again reading your "with defaults first".

Your call on a -announce post for @task-reveal. I don't put every last change there, so this one is up to you.

I'll let this one go without an announcement.

Would you like to find two or three high-profile switches to move over to this new infrastructure? So we get some good testing?

Sure. I may have a few other things in line first. if you happen to be working on something where it makes sense to migrate, don't wait for me.