-
Notifications
You must be signed in to change notification settings - Fork 9
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
Initial support for itext translations #13
Conversation
Before it slips my mind, I think it’s better to call this out here than in the committed code: I would characterize my comfort level with the I strongly considered an alternative API, where it requires an explicit contextual option to specify the active language (otherwise only returning the default translation value). I also considered an alternative where the active language state would be specified somehow in the DOM itself. Ultimately both options felt out of scope for this PR—both would break spec, even if they could be treated as cross-package implementation details—but I do think it’s worth discussing this spec awkwardness as such. |
const textElements = evaluator.evaluateNodes<XFormsItextTextElement>('./xf:text[@id]', { | ||
contextNode: translationElement, | ||
}); | ||
const translations = new Map( | ||
textElements.flatMap((textElement) => { | ||
const value = evaluator.evaluateString('./xf:value[not(@form)]', { | ||
contextNode: textElement, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good/bad news! We've finally got ourselves an obvious optimization target right here.
With form translation support, I was curious how well some of my favorite test forms are shaping up. I wasn't actually looking for performance issues, just wanted to check out behavior, see if I could spot some bugs, that sort of thing. But it turns out that this chunk of evaluations is pretty slow on forms with a lot of translations!
In a sense, we could treat this as a case of premature optimization, and see how performance shakes out without the upfront caching approach. In some quick testing, I also found it's much faster to just use more optimal direct DOM APIs to build the cache. Ultimately, though, this implicates a few optimization targets I've been anticipating in @odk/xpath
:
- the child axis step implementation is explicitly called out as non-optimal (though I suspect at least a couple of other axes will have similar performance issues, called out or not)
- per-node qualified name tests are inherently expensive
- the predicate implementation is quite naive as well
- the static, predictable structure of an XForm definition is currently opaque to the evaluator, but there's a ton of opportunity to optimize around it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thought: even if this were optimized, it's still being performed each time an XFormsXPathEvaluator
instance is constructed, which may be (is) unnecessary, redundant, or both. If we change nothing else, I'd want to at least defer building the mapping until a lookup warrants it.
The `jr:` functions were missing… from the missing list. And `version` is an Enketo thing without clear need. But support should be trivial if that turns out to be wrong
This is only part of separating the standard XPath functionality from the [ODK] XForms functionality. Separating the built-in function libraries will follow
The exact test case already exists in the xforms suite
This is a precursor to support for functions in the JavaRosa namespace (where the XForms spec explicitly calls out their prefixed usage). It’s a reasonable commit in its own right, but I’m also calling this out as a separate commit to highlight that behavior hasn’t changed in terms of current unprefixed usage of functions in the `fn`, `xf` and `enk` namespaces.
64b1a3f
to
86285dd
Compare
// TODO: | ||
// | ||
// 1. Is there any reason not to *always* provide `fn`-namespaced functions (and | ||
// is there any reason not to *always* make them available unprefixed)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a question specifically for ODK XPath (and not something more generic)? If so, I don't see any reason not to on both counts. Maybe I don't understand why this is a question!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a question of the API for function extensibility, and how much it should assume. One reason it might matter is how it could affect the design for something like #17. Or something downstream from #39, where a ContextNode
implementation might be better paired with certain, less general, fn
implementations. I don't think either of those were what I had in mind with the question, but just the more basic "here is an extensibility mechanism, is it too flexible, just flexible enough, flexible in wrong ways, ...?"
|
||
- `<value form="...anything...">` is not yet supported. It's unclear what the interface for this usage might be. | ||
|
||
- The interface for getting and setting language state is currently experimental pending integration experience, and may be changed in the future. The intent of this interface is to be relatively agnostic to outside state management, and to isolate this sort of stateful context from the XForm DOM, but that approach may also change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a great approach -- lay something reasonable down and potentially revisit.
} | ||
|
||
interface XFormsItextTextElement extends Element { | ||
readonly namspaceURI: XFormsNamespaceURI; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace
'./h:html/h:head/xf:model/xf:itext/xf:translation[@lang]', | ||
{ contextNode: xformRoot } | ||
); | ||
// TODO: spec says this may be `"true()"` or `""`, what about other cases? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://getodk.github.io/xforms-spec/#languages: "A default=”” attribute can be added to a to make it the default language"
My understanding is that there are several attributes including this one where it's the attribute presence that matters, not its value. JR ignores the value and just checks that the attribute exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the implementation here matches JavaRosa, interesting! I think that was my intuition based on the spec language around these attribute-presence cases, but it seems surprising that there isn't at least some notion of present-but-false.
On the other hand, it's conceptually very similar to HTML boolean attributes. I wonder if we should consider spec revisions for cases like this so there isn't ambiguity. As it stands, forms with any other expression in these attributes might behave unexpectedly (and each is an opportunity for JavaRosa and web-forms to diverge).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, where that pattern is used, let's make it clear they are HTML boolean attributes and link to that reference.
Agreed, and the choice-name function is equally problematic. I agree that it's something to revisit. I realized that letting this linger would likely cause pretty annoying conflicts. Sorry about that. I think there was some possible add-on work mentioned but I think it would be ideal to merge now. |
I'm going to merge now as we discussed. I do want to mention that this has jogged my memory, and I'll call out that, among other things, #14 includes some optimizations of the work here. The more I think about it, I feel less like dropping that PR and more like it should be reviewed and presumably merged with the understanding that it's probably less mature than some other aspects of the work so far. |
(Branched from #12 as it depends on some of the body/view changes. Here's the diff)
See it in action!
Screen.Recording.2023-11-29.at.4.07.11.PM.mov
About the change
The bulk of this change is in the
@odk/xpath
package. And in turn the bulk of that work is across these general themes:XFormsXPathEvaluator
constructor is now in@odk/xpath
, and the underlyingEvaluator
class no longer supports XForms extensions by default.Evaluator
includes (by default) functions in thefn
namespace (with or without prefix), and theXFormsXPathEvaluator
class includes (also by default) functions in thefn
,xf
,enk
1 (with or without prefix) and nowjr
(prefix required) namespaces.jr:itext
function itself. This is essentially just a nestedMap
lookup (i.e.Map<Language, Map<TextId, TextValue>>
). This is possible because...Evaluator
subclass, it no longer felt awkward to introduce some knowledge of XForms structure. In this case, the translationMap
s are populated on construction from the XForm DOM provided as itsrootNode
option. (There are some caveats with this approach, described in more detail in updates to the@odk/xpath
package's README.) The active language follows the default language logic from the ODK XForms spec, and defers to an outside caller to change the active language from there.The rest of the change is much more straightforward:
itext
expressions (currently just labels, as there wasn't already view support for hints).current()
in@odk/xpath
as well. I'm okay with breaking that out of this PR if preferred, but it keeps getting bounced around rebases across different branches and I'd like to land it somewhere, so I left it in this time.A note on next steps
I expect that the XPath side of support for secondary instances—i.e. the
instance
function itself—will follow a very similar pattern to the solution forjr:itext
(but all the upfront stuff is already addressed, so it should be a much smaller and more focused change).Footnotes
The
enk
namespace isolates a couple of functions/aliases where behavior differs from the XForms spec. These were included to pass the original test suite fromopenrosa-xpath-evaluator
, and we may consider removing them... even in this PR if it makes sense. ↩