feat(#347): adds support for select with images - engine part #399
Conversation
🦋 Changeset detectedLatest commit: 11fbc00 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| t( | ||
| 'translation lang="default"', | ||
| t('text id="static_instance-cities-0"', t('value', 'Montréal')), | ||
| t('text id="static_instance-cities-1"', t('value', 'Grenoble')) |
There was a problem hiding this comment.
I would have added another language and verified that when you switch the language the /data/city_name value updates. But maybe that's already tested elsewhere?
I think we should also consider introducing a failing test that's just like this one but there are additional itext forms (e.g. image). The results should be the same but I believe currently the jr://images url will be concatenated to the label text. That's a limitation we're accepting for now and a failing test would document it effectively.
There was a problem hiding this comment.
Ok, I see language switching is well covered at jr-itext.test.ts. I think this still adds something because it has a real calculate in the form definition and is making sure that's wired up correctly. Maybe it should live in the jr-itext test suite? How did you decide to put it here? It's certainly logical. Are there other functions tested here?
There was a problem hiding this comment.
Yes, I can add more tests for that.
How did you decide to put it here? It's certainly logical. Are there other functions tested here?
I thought, "Can the calculate work with itext?" Then I would go to the calculate test suite to check.
In L76, there's something about accessing text, but no other functions inside calculate
bind('/data/calc')
.type('string')
.calculate("instance('instance')/root/item[value = 'A'][count = /data/input]/id"),
But I can create a new test suite if it feels too weird here.
There was a problem hiding this comment.
I would have added another language and verified that when you switch the language the /data/city_name value updates.
I've added an additional test case for that here.
| for (const valueElement of domProvider.getChildrenByLocalName(textElement, 'value')) { | ||
| // | ||
| if (!domProvider.hasLocalNamedAttribute(valueElement, 'form')) { | ||
| return domProvider.getNodeValue(valueElement); |
There was a problem hiding this comment.
Fact: This is where the actual XPath query happens; it returned the first value nodes that didn't have the form attribute.
Now, it returns all value nodes, and they are evaluated in the engine (see createTextRange file)
|
|
||
| export const jr = new FunctionLibrary(JAVAROSA_NAMESPACE_URI, [...Object.values(string)]); | ||
| export const jr = new FunctionLibrary(JAVAROSA_NAMESPACE_URI, [...Object.values(nodeSet)]); |
There was a problem hiding this comment.
Fact: Currently, there can only be one function for itext. I tried leaving string and node-set, but it just keeps one. Knowing that itext will always return one data type from the XPath layer could be a good thing.
...xforms-engine/src/parse/model/SecondaryInstance/sources/ExternalSecondaryInstanceResource.ts
Outdated
Show resolved
Hide resolved
| @@ -15,19 +14,20 @@ const isOutputElement = (element: Element): element is OutputElement => { | |||
| return element.localName === 'output' && element.hasAttribute('value'); | |||
| }; | |||
|
|
|||
| export class TextChunkExpression extends DependentExpression<'string'> { | |||
| export class TextChunkExpression<T extends 'nodes' | 'string'> extends DependentExpression<T> { | |||
There was a problem hiding this comment.
Once we add all the support for output, markdown, HTML styling, etc., this could naturally drop the string and only be TextChunkExpression<'nodes'> extend DependentExpression {. I need to experiment with those scenarios to confirm.
| @@ -279,4 +297,14 @@ export class PrimaryInstance< | |||
|
|
|||
| return Promise.resolve(result); | |||
| } | |||
|
|
|||
| loadMediaResources(resourceURL: string): Promise<MediaResource> { | |||
There was a problem hiding this comment.
This class provides simple functions to access high-level information, like the active language, and could be extended to retrieve media files, such as images or videos. That's why I added the loadMediaResources function here.
To optimize performance, it avoids querying all media files during the initial LoadForm phase, as finding media URLs requires traversing a complex node tree. Instead, it leverages the engine’s XML processing phase when it creates an instance, where it already navigates the node tree to prepare data for all question types. By collecting media information during this process, the engine avoids a second, redundant tree traversal.
Later, we can revisit and find a way to retrieve all media URLs at the initial LoadForm phase.
| @@ -74,10 +138,18 @@ export const createTextRange = <Role extends TextRole>( | |||
| definition: TextRangeDefinition<Role> | |||
| ): ComputedFormTextRange<Role> => { | |||
| return context.scope.runTask(() => { | |||
| const getTextChunks = createTextChunks(context, definition.chunks); | |||
| const textChunks = createTextChunks(context, definition.chunks); | |||
| const evaluator = isEngineXPathEvaluator(context.evaluator) ? context.evaluator : null; | |||
There was a problem hiding this comment.
The type static-check enforces this validation. I tested several forms (all question types form, all note form, all select form, etc). When creating a Text Range, the evaluator always has a root node that is a Primary Instance.
But, to play safe, we are leaving this validation.
There was a problem hiding this comment.
This looks like a no-op to me! Is it just confusing the type checker or am I missing something? This is essentially saying if context.evaluator is null, use null, otherwise use context.evaluator, isn't it? Let's try hard to do away with null here. As far as I understand it, if there isn't an evaluator, nothing works so we should halt.
There was a problem hiding this comment.
The evaluator, in this case, is only used for retrieving media attachments. I've added it as part of the media context, so its role is clearer in the code.
packages/xforms-engine/src/lib/reactivity/validation/createValidation.ts
Outdated
Show resolved
Hide resolved
packages/xforms-engine/src/lib/reactivity/createItemCollection.ts
Outdated
Show resolved
Hide resolved
Sure! @lognaturel thanks for the feedback! Let me know if you have any other suggestion My comments below:
Yes, a Vue component can make the request, for example, for each select's option: Another idea I'm playing around with is letting the engine's interface do the requests, so the Vue component calls one function that retrieves all images: I'm making this as part of the front-end work.
Correct, the requests are in parallel. Going back to the previous question:
Usually, the first one is preferred, as it displays content as quickly as possible. In the select with images context, it will still be better the first option, so the user sees that it is loading (in slow connections) instead of waiting with an empty state. What do you think?
Yes, it only requests images from the active language. When a new language is chosen, it requests the images for that language.
Yes, we have a placeholder for when the image can't be loaded. |
packages/xforms-engine/src/parse/attachments/FormAttachmentResource.ts
Outdated
Show resolved
Hide resolved
Primarily because it avilds nested generics
| @@ -19,6 +19,8 @@ export interface SelectItem { | |||
| export type SelectValueOptions = readonly SelectItem[]; | |||
|
|
|||
| export interface SelectNodeState extends BaseValueNodeState<readonly string[]> { | |||
| get isSelectWithImages(): boolean; | |||
There was a problem hiding this comment.
I think this will go away, and not changes in this file
| @@ -51,6 +51,7 @@ interface SelectControlStateSpec extends ValueNodeStateSpec<readonly string[]> { | |||
| readonly label: Accessor<TextRange<'label'> | null>; | |||
| readonly hint: Accessor<TextRange<'hint'> | null>; | |||
| readonly valueOptions: Accessor<SelectValueOptions>; | |||
| readonly isSelectWithImages: Accessor<boolean>; | |||
There was a problem hiding this comment.
All these changes might be reverted and handled in the front-end side
packages/xforms-engine/src/lib/reactivity/text/createTextRange.ts
Outdated
Show resolved
Hide resolved
| @@ -118,12 +118,6 @@ export type TextOrigin = | |||
| | 'form-derived' | |||
| | 'engine'; | |||
|
|
|||
| export interface TextMediaSource { | |||
There was a problem hiding this comment.
Not part of what the client sees so shouldn't be in client.
| get audioSource(): JRResourceURL | null; | ||
| get videoSource(): JRResourceURL | null; | ||
|
|
||
| get imageSource(): JRResourceURL | undefined; |
There was a problem hiding this comment.
I ended up with undefined here mostly because of how MediaSources gets populated. Semantically I think null would be fine here.
| } else { | ||
| // translation expression evaluates to an entire itext block, process forms separately | ||
| computed.forEach((itextForm) => { | ||
| if (isEngineXPathElement(itextForm) && itextForm instanceof StaticElement) { |
There was a problem hiding this comment.
I think there are types that would let us identify it as an itext form but I couldn't immediately figure that out and I think this reads well enough for now.
| @@ -145,7 +89,8 @@ export const createTextRange = <Role extends TextRole>( | |||
| const textChunks = createTextChunks(context, definition.chunks); | |||
|
|
|||
| return createMemo(() => { | |||
There was a problem hiding this comment.
There are three nested memos here. That may be optimal or it may add overhead, I'm not sure. I think it's something we can explore in the future. My hunch is that we'll want to revise all of this again pretty soon.
| @@ -19,7 +19,7 @@ export class ItemsetLabelDefinition extends TextRangeDefinition<'item-label'> { | |||
| } | |||
|
|
|||
| readonly role = 'item-label'; | |||
| readonly chunks: TextChunkExpression<'nodes' | 'string'>[]; | |||
| readonly chunks: ReadonlyArray<TextChunkExpression<'nodes' | 'string'>>; | |||
There was a problem hiding this comment.
When I ran lint locally it flagged all of these array declarations on complex types. It didn't seem to block the build in CI though so maybe there's an inconsistency to investigate at some point. Seems like using the array types with a type with generics is consistent and idiomatic so I went with it.
c6a2271 to
ef32d5b
Compare
|
We've made a high-level decision to pass JR URLs through to the client for media. This is a departure from how external secondary instances and submission media attachments are handled but it's a deliberate choice based on the current lack of need to have the engine do any media processing. This could change in the future but is consistent with the JavaRosa/Collect split. JR URLs are exposed through the There's some kind of API test missing here: we should test that on language changes, the JR URLs on We've gone with a minimally-invasive approach. The There are some things we know are uncomfortable:
We'll continue to work towards addressing those in future work. |
e9c2ad2 to
11fbc00
Compare
There was a problem hiding this comment.
I put some notes on how I'm thinking about this work at #399 (comment)
I haven't tried this with a frontend and there are no API tests so I think it's important to hook things up before merging to do a quick sanity check, @latin-panda
Other than that, I'm comfortable with merging!
|
I thought the changes from 11fbc00 would be confusing once we squashed so I reverted them for now. We could apply them as a separate set of changes or wait until we are in that part of the code again. The latter would be my preference because I think we'll understand a lot more by then and may choose a different direction. |
| * should create a {@link: TextElementDefinition}, so that createTextChunks function can | ||
| * request the computed value to XPath and create the TextChunk. | ||
| */ | ||
| const processValueNodeChildren = (item: EngineXPathNode) => { |
There was a problem hiding this comment.
If I recall correctly, this was introduced to re-establish passable behavior for itext default forms with outputs in them. I don't think it's necessary for this PR or if we do want to do it here we should introduce a test related to it.
Closes #347
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
CI passes successfully, and manual testing of existing forms in preview.
Why is this the best possible solution? Were any other approaches considered?
Retrieving media type and media URL
This update modifies the XPath itext function to return a node-set instead of a single string, allowing retrieval of multiple
valuenodes associated with an itext. This enables support foroutputnodes, where child nodes (likeoutput) can be accessed within eachvaluenode. The engine can then iterate over these child nodes, understand their attributes, giving more flexibility in handling complex XML structures.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
No visual impact for users yet, everything should still work as expected.
Do we need any specific form for testing your changes? If so, please attach one.
I think existing forms in preview and CI are covering most of it. But what other tricky scenario is there?
What's changed
itextfrom string to node-set.