feat: find symbols by searching the gloss, and enter symbols using svg builder strings#44
feat: find symbols by searching the gloss, and enter symbols using svg builder strings#44klown wants to merge 112 commits intoinclusive-design:mainfrom
Conversation
Also, add `stream` (`true` or `false`) parameter to `queryChat()` function.
- `CommandTeletgraphicCompletions` is component that calls ollama with a "bag of words" and a prompt to provide sentence completions - Introduced `sentenceCompletionsSignal` that will eventually handle changes to the completions.
- removed all `console.debug()` statements
The palette of found symbols is a collection of cells that permit: - ediingt a label for each symbol - adding the symbol and its label to the input area
…ay decomposition svg builder string
- compromise was part of the ISAAC demo branch: - feat/find-by-gloss-plus-modifiers
apps/ollama/ollama.js
Outdated
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * Copyright 2024 Inclusive Design Research Centre, OCAD University | |||
| * Copyright 2024-2025 Inclusive Design Research Centre, OCAD University | |||
There was a problem hiding this comment.
Copyright can use 2026 now. :)
There was a problem hiding this comment.
:-) This is updated in PR #42. If I update it here too, what happens? I guess there is no diff so it shouldn't matter?
There was a problem hiding this comment.
On closer review, the version of ollama.js here is wrong. It is the same version as in the feat/integrate-ollama-api-with-palette from PR #42 where the UI and the ollama API were factored into separate files (ollama.js and ollamaApi.ts). I have restored the correct version for this branch.
| options: BlissSymbolInfoType & LayoutInfoType | ||
| }; | ||
|
|
||
| export function ActionGlossSearchCell (props: ActionGlossSearchCellPropsType): VNode { |
There was a problem hiding this comment.
I just noticed this component doesn't seem to be used anywhere except in its test. Can you double check? Thanks.
There was a problem hiding this comment.
It's used in GlossSearchPalette -- that's what the cells are in that palette. They are created by the makeMatchesPalette() in that file.
| */ | ||
| function indexOfIndicatorSuffix (gloss: string): number { | ||
| let index = -1; | ||
| for (let i = 0; i < INDICATOR_SUFFIXES.length; i++) { |
There was a problem hiding this comment.
Regex might be more efficient than a loop:
const suffix_regex = new RegExp(`(${INDICATOR_SUFFIXES.join('|')})$`, 'i');
const match = gloss.match(suffix_regex);
return match ? match.index : -1;
There was a problem hiding this comment.
The use of compromise() and NLP for adjusting the gloss is not part of this PR. I have removed the compromise package and all related code. I will create a separate issue for NLP processing.
src/client/ActionSearchGloss.ts
Outdated
|
|
||
| return html` | ||
| <form onSubmit=${searchGloss} class="actionSearchGloss"> | ||
| <label for=${GLOSS_ENTRY_FIELD_ID} style="color: white;">Search gloss: </label> |
There was a problem hiding this comment.
The inline style should go into .scss. Moreover, setting color explicitly may affect the auto-adaptation on various browser color theme settings.
There was a problem hiding this comment.
This and other styles have been either removed or moved to an appropriate .sccs file. Regarding being responsive to browser or system-wide colour themes: the palette has never had this capacity. It is showing up here because the new dialogs are using standard web controls. As a stop-gap, @jobara provided a patch that disables dark-mode, and the problem of vanishing text when switching to dark-mode not long occurs. That is not satisfactory in the long run since it means that if a user prefers dark-mode, it will not be available.
What should happen is that the entire palette, not just the new dialogs, adapt to changes in colour themes. That is a complex enough problem that it deserves its own issue and PR. I have created an issue for that work.
src/client/ActionSvgEntryField.scss
Outdated
|
|
||
| .actionSvgEntryField { | ||
| width: 25rem; | ||
| background: white |
There was a problem hiding this comment.
Again, setting background color explicitly may affect the adaptation to browser color theme settings.
src/client/GlobalUtils.ts
Outdated
| else { | ||
| // If removing a verb indicator, try turning the word back into a "noun" | ||
| // (gerund), e.g. "walk" -> "walking". | ||
| result = doc.verbs().toGerund().text(); |
There was a problem hiding this comment.
Why does removing a verb indicator turning the word back to a noun? Shouldn't "to walk" be "walk" now rather than "walking"?
There was a problem hiding this comment.
Why does removing a verb indicator turning the word back to a noun?
In Bliss, to make a verb, add the action indicator to a symbol that is typically a noun. The removal of the indicator will turn the word back into a noun, in most cases.
Shouldn't "to walk" be "walk" now rather than "walking"?
Yes, it should be. (Note that "walk" is a noun, in this case), But I could find no way using compromise to do that. The closest was to use toGerund() which worked sometimes.
All of this shows that the NLP used for the ISAAC demo was barely good enough for that demo, but only for that demo. As noted above, all of the NLP has been removed from this PR.
src/client/GlossSearchPalette.ts
Outdated
|
|
||
| // Update rows, columns, etc. | ||
| colIndex++; | ||
| if (colIndex > numCols) { |
There was a problem hiding this comment.
If numCols is 4, this allows colIndex to be 0, 1, 2, 3, 4. Probably use if (colIndex >= numCols - 1).
There was a problem hiding this comment.
Thank you, thank you, thank you! This little fix improves the layout immensely. I have also changed the maximum number of columns, numCols to 5 with this new check and that works.
src/client/GlossSearchPalette.ts
Outdated
| columnSpan: 1 | ||
| } | ||
| }; | ||
| jsonPalette.cells[`${match.label}-${uuidv4()}`] = cell; |
There was a problem hiding this comment.
If makeMatchesPalette is called during a re-render, every cell will get a brand new ID with the use of uuidv4(). Every re-render will recreate the cell palette from scratch. What do you think about using bciAvId instead of calling uuidv4() so every cell palette will have a fixed key.
There was a problem hiding this comment.
That's a good idea. I was concerned mainly with having a unique ID here. BCI AV IDs are unique and thus are sufficient. And, they have the added benefit of supporting a DOM query.
src/client/GlossSearchPalette.ts
Outdated
| return html``; | ||
| } | ||
| else if (matches.length === 0) { | ||
| return html`<p style="color: white;">No matches found</p>`; |
There was a problem hiding this comment.
Move the inline style into scss. Add a aria-live="polite" and an aria status to announce "No matches found."
There was a problem hiding this comment.
For now, I have removed the inline style since I don't think it's necessary to begin with. Regarding the use of aria, I'm not against it, but in this case it will only matter if someone is using an AT like a screen reader. It won't cause the adaptive-palette to change its behaviour.
There was a problem hiding this comment.
Having thought about it a bit more, adding role="status" here is a good idea. There is another case in the SVG entry dialog where it reports an "Invalid builder string". In both cases, I have added the status role to the markkup. FYI, adding aria-live=polite is unnecessary since the status role is automatically polite, (and atomic).
src/client/GlossSearchPalette.ts
Outdated
| * Create a JsonPaletteType from an array of matches based on a gloss search. | ||
| * | ||
| * @param {Array} glossMatches - Array of Bliss symbol information objects whose | ||
| * glass matches the search term. |
There was a problem hiding this comment.
You have eagle eyes. Thanks.
|
The same issue with the color contrast issue in the browser dark theme as mentioned in the pull request #42. |
|
One concern is about pushing the binary |
The `compromise` library was used in the ISAAC demo to adjust the gloss of symbols when indicators were added or removed to better reflect the grammatical change. But, it was superficial and the issue needs a deeper investigation and implementation to properly make these kinds of transformations.
The version of `ollama.js` in this branch was mistakenly copied from the `feat/integrate-ollama-api-with-palette` branch. It should be the version just before factoring it into ui vs. ollama api calls. For the latter see `.../src/client/ollamaApi.ts` in the ollama integration branch.
Yes, this was experimental based on a recommendation at one of the language development meetings. I'm reverting back to speaking the label of the button. |
… builder strings
fix: disable dark mode
- Changed "SVG Entry" title to "SVG Builder String", - `ActionGlossSearchCell`: added label to text input field, - `ActionGlossSearchCell`: added main BCI AV ID as a prefix for the SVG Builder string.
npm testwithout errorsnpm run devwithout errorsDescription
This pull request adds two dialogs to the adaptive-palette:
In addition, a palette of matches found is produced for each successful search:
Steps to test
See the "Description" above for how to use the dialogs.