Port Hufnagel font switching to production Verovio worker#1376
Port Hufnagel font switching to production Verovio worker#1376zih-syuan wants to merge 25 commits into
Conversation
diva.js#master no longer exists; pin to last known good commit. See #1365. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds assets/Hufnagel.zip for Verovio fontAddCustom integration. Font source files live in zih-syuan/hufnagel-font; this branch only tracks the packaged artifact. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a Square/Hufnagel selector below Display Options. Clicking updates the label and logs to console; engine wiring (setFont / setNotationType) is the next step. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- VerovioWorker: add setFont handler (lazy-loads Hufnagel.zip as base64, calls toolkit.setOptions with full options) - NeonCore: add setNotationFont() — sends setFont to worker, clears lastPageLoaded to bypass cache on next render - NeonView: add setNotationType() — calls setNotationFont then re-renders - DisplayControls/DisplayPanel: thread neonView through setDisplayListeners → initDisplayControls → setNotationTypeControls so dropdown is wired - Interfaces/SingleView/DivaView: update setDisplayListeners signature Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…fallback Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
E99F is absent from Hufnagel.zip so Bravura fallback showed a square notation glyph. Generate a two-diamond icon via Verovio and use it as pngFallback for the distropha button in Hufnagel mode. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…back Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Verovio segfaults on staffDef notationtype="neume.hufnagel" (and "neume.square" is unverified) and doesn't render @con, so canonical Hufnagel connections (@con="e") and the real notationtype must never reach the Verovio worker directly. Add a temporary conversion shim in ConvertMei.ts (stripHufnagelForVerovio / restoreHufnagelForStorage) applied unconditionally at the two real boundaries - NeonCore.loadData() before anything reaches Verovio, and NeonCore.updateDatabase()/save()/ export() before anything is persisted or downloaded - so every load path (fresh fetch, cache hit, undo, redo) and every persistence path (save, export, download) stays consistent. See issues #1360, #1366. Co-Authored-By: Claude <noreply@anthropic.com>
Drop the hufnagel liquescent down primitive and the alternate F clef variant (fClefHufnagel) from the Insert panel, leaving one F clef button shared with square notation.
Nudge virga, virgaReversed, and custos icon offsets in the Insert panel so they align visually with the rest of the hufnagel primitives.
…handling - Upload modal now has a Notation Type choice, written into the MEI and synced to the Editing page dropdown. - Removed the auto con->ligated conversion before Verovio; it was corrupting real content. @con now passes through untouched. - Save no longer converts @ligated back to @con (only Download does), so a manually toggled connection survives a reload. Co-Authored-By: Claude <noreply@anthropic.com>
|
Is this still using the Verovio version from RISM Digital with no modification? |
|
Yes, this still uses the unmodified Verovio build from RISM Digital. This PR only changes how Neon configures the toolkit: for Hufnagel, it loads the custom font through Verovio's existing For |
| // Sets <staffDef notationtype="neume.square"/"neume.hufnagel"> on the given | ||
| // MEI text. This is the only place a freshly-uploaded/created MEI file gets | ||
| // its notation type recorded - the Edit page's notation type dropdown does | ||
| // not lock or otherwise depend on this; it can still be changed afterward. |
There was a problem hiding this comment.
Yes. staffDef@notationtype is now set in ConvertMei.ts.
| function stripStaffDefNotationTypeSuffix(mei: Element): void { | ||
| const staffDef = mei.querySelector('staffDef'); | ||
| const value = staffDef?.getAttribute('notationtype'); | ||
| if (value === HUFNAGEL_NOTATIONTYPE || value === SQUARE_NOTATIONTYPE) { | ||
| staffDef.setAttribute('notationtype', 'neume'); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is only called once. I don't see why make this a function.
There was a problem hiding this comment.
Fixed. I removed the function and inlined the logic in stripHufnagelForVerovio().
| // Restores a bare staffDef notationtype back to "neume.hufnagel"/"neume.square" | ||
| // given the notation type Neon already knows it's displaying. | ||
| function restoreStaffDefNotationTypeSuffix( | ||
| mei: Element, | ||
| notationType?: string, | ||
| ): void { | ||
| if (notationType !== 'hufnagel' && notationType !== 'square') return; | ||
| mei | ||
| .querySelector('staffDef') | ||
| ?.setAttribute( | ||
| 'notationtype', | ||
| notationType === 'hufnagel' ? HUFNAGEL_NOTATIONTYPE : SQUARE_NOTATIONTYPE, | ||
| ); | ||
| } |
There was a problem hiding this comment.
I don't see why making this a function as well. you can just append 'neume.' when setting it.
There was a problem hiding this comment.
I removed the helper and now set notationtype directly as neume.${notationType}.
| * Set click listeners for Notation Type dropdown (Square / Hufnagel). | ||
| * Currently only updates the label and logs to console; engine wiring is next. | ||
| */ | ||
| export function setNotationTypeControls(neonView: NeonView): void { |
There was a problem hiding this comment.
You do not need NeonView here. You can dispatch the notationtypechange event (already happening below) and have DivaView/SingleView listen for it and call setNotationType themselves, since they already hold a neonView reference. That avoids threading the param through initDisplayControls, setDisplayListeners, DivaView.didLoad, and SingleView.onSVGLoad.
There was a problem hiding this comment.
Fixed. DisplayControls now only dispatches notationtypechange. DivaView and SingleView listen for the event and call setNotationType() through their own neonView references, so the neonView parameter is no longer threaded through the display controls
|
|
||
| .idea | ||
|
|
||
| scripts/gen-hufnagel-grouping/ |
There was a problem hiding this comment.
Removed. It was only for a local glyph-generation scratch directory and is not needed by Neon.
| "diva.js": "github:DDMAL/diva.js#5f530483f653db68af1cbb92bc8cc967b51763c3", | ||
| "http-server": "^14.1.1", | ||
| "jsonschema": "^1.2.4", | ||
| "jszip": "^3.10.1", |
There was a problem hiding this comment.
This can be dropped. See comments above.
There was a problem hiding this comment.
Dropped. jszip and @types/jszip have been removed as direct dependencies.
| "dependencies": { | ||
| "d3": "^5.11.0", | ||
| "diva.js": "github:DDMAL/diva.js#master", | ||
| "diva.js": "github:DDMAL/diva.js#5f530483f653db68af1cbb92bc8cc967b51763c3", |
There was a problem hiding this comment.
package.json referenced #master, but that branch no longer exists, so installs that resolve the dependency afresh fail. The existing yarn.lock pointed to commit 5f530483, which was already known to work with Neon, so I pinned package.json to that commit as the temporary workaround described in issue #1365 .
|
|
||
| const cache = new Map<string, Promise<FontData>>(); | ||
|
|
||
| async function loadFontData(zipUrl: string): Promise<FontData> { |
There was a problem hiding this comment.
You do not need jszip here. You can ship Hufnagel.woff as a flat asset instead of a zip and fetch + base64-encode it directly, since this only ever pulls out the font bytes. That drops the JSZip dependency and the unzip step from the worker, and since the font is static, you could even precompute the base64 at build time instead of re-fetching/encoding it every session. Also duplicated verbatim in VerovioWorker-dev.js, worth sharing one helper.
There was a problem hiding this comment.
Updated. GlyphIcons.ts now loads flat .woff2 and bounding-box XML assets, so it no longer uses JSZip. I kept Hufnagel.zip in the Verovio workers because fontAddCustom expects the complete custom-font ZIP rather than only the font bytes. The production and dev workers now share a single base64-loading helper.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| title: string, | ||
| mei: File, | ||
| bg: File, | ||
| notationType: string, |
There was a problem hiding this comment.
Why do you still need notationType here? I don't think you need to touch this file.
| ); | ||
| const notationType = selectedNotationType?.value ?? 'square'; | ||
|
|
||
| handleUploadAllDocuments(currentFolder, notationType) |
There was a problem hiding this comment.
Why do you need notation type during upload?
| ); | ||
| return ( | ||
| createManifest(id, newName, mei, image) | ||
| createManifest(id, newName, mei, image, notationType) |
There was a problem hiding this comment.
Same as above. Why do you need notation type in manifest?
| } | ||
| }); | ||
|
|
||
| ['square', 'hufnagel'].forEach((type) => { |
There was a problem hiding this comment.
This looks weird. I recommend you make notation type a type.
| * HTML for each insert tab (neume, grouping, clef, system, and division). | ||
| */ | ||
| export const insertTabHtml: Record<string, string> = { | ||
| primitiveTab: `<p class="insert-element-container"> |
There was a problem hiding this comment.
Is this still used? Is it better for buildPrimitiveTabHtml to handle this as well?
| } | ||
| } | ||
|
|
||
| function activateCurrentOrFirst(insertHandler: InsertHandler): void { |
There was a problem hiding this comment.
Is this a better naming for this function? It's not clear from the naming what it does. Also add a docstring maybe?
| * only entries marked as dirty will be updated. | ||
| */ | ||
| async updateDatabase(): Promise<void> { | ||
| async updateDatabase(notationType?: string): Promise<void> { |
There was a problem hiding this comment.
I don't think this is necessary. You can store the converted MEI in the database. You only need to restore it when exporting MEI.
Related issues
Related to #1367 (font switching implemented; dropdown-freeze and switch-warning behavior still open for discussion, not closed by this PR)
Closes #1368
What this does
fetchHufnagelBase64,setFontcase) fromVerovioWorker-dev.jsto the production workerVerovioWorker.js, so the feature actually works once live on neon.simssa.ca.Testing
yarn cypress:test: all passing (44 passing, 6 pending — unrelated to this change)VerovioWorker.js(not the dev version) loads, and Hufnagel font renders correctly