Repository navigation
Connect the browser adapter to existing Chemvas editing services - #542
Conversation
dhsohn
left a comment
There was a problem hiding this comment.
I reviewed f3b69f9 against the goal of keeping identical behaviour and results when switching between Qt and the browser.
What holds
- Native path: moved code in
features.graph,features.selection,features.rendering,bond_tool_logic,text_tool_logic,main_window_configandshell.icon_designbehaves the same in Qt (geometry, ordering, history recording, item lifecycle). The ADR claim that Qt-specific boundaries preserve native defaults holds. - Server: ~7,500 random edits (bond, ring, style, move, delete, atom, undo/redo) never changed document, history or revision on failure. Previews are non-mutating. Host/Origin, launch token, asset allowlist, size limits and hostile JSON are handled correctly.
tests/test_web_adapter.pypasses (127). The full suite on this head shows the same 25 failures asfb277d81on my macOS offscreen run (calculation CLI, note workflows, order-dependent widget leak), so no new failures.
Main gap: target resolution and input live in JS
Which atom/bond a pointer hits, bond-release snapping, ring target choice, label trimming/layout, the keyboard map and wheel/zoom are decided in the browser instead of by the existing owners. The same input therefore produces different documents in Qt and the browser (inline comments have reproductions). I think the structural fix is for the browser to send raw points/modifiers/keys and for BrowserStructureAdapter to resolve targets with the same functions BondTool and the insert controller use.
Test evidence
The differential tests call builder/service functions directly with exact coordinates and shared IDs, and every compared structure is unlabelled, so none of the gaps above can fail them. Tests I would add:
- drive
BondTooland the browser with the same pointer sequences, including near-miss clicks and releases onto a bond; - bond geometry next to O, Cl and NH2 at bond lengths 20 and 40;
- ring target choice via
_template_structure_target_ids; - multi-select delete compared with Qt
delete_selected_items; - the same key sequence over hovered items on both sides;
- hit testing at two zoom levels and two bond lengths.
Nit outside the diff: ui/tools/text_tool.py:60 pos = QPointF(*target.pos) is now unused because apply_text_input reads target.pos.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed d3957ef.
- Dotted bonds: the browser now receives the native
dotted_bond_dotscentres and radius through the existingBondPathPrimitiveslot, andtest_dotted_geometry_matches_native_pathscompares them against the actual Qt path items for single/double/outer styles, two bond lengths, chain and ring. That is a genuine Qt-vs-browser comparison. - Style/order, numeric validation and first-request session slots: verified in the threads above.
tests/test_web_adapter.pypasses (177) on this head.
Still open from the first review: target resolution in JS (scene.mjs hit areas, ring target, release snapping), label trimming/layout, keyboard map, wheel/zoom, the duplicated ring-preference rule, history memory, lost-response resync and idle session expiry.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed 368c302 and 070d821.
- Bond gestures: the browser now sends raw press/release points and the adapter reuses
resolve_bond_press_target,resolve_bond_snap_targetandresolve_bond_endpoint_targetwith shared radius constants;snap_angle_stepcomes fromCanvasToolSettingsState, which is also the only owner Qt reads. The new differential test drives the realBondToolacross four cases, two lengths and two snap steps. Remaining gap: presses 0.35–0.528 × bond length from a bond (details in the scene.mjs thread). - Native side: only the literals 0.35/0.2 became
BOND_PICK_RADIUS_RATIO/BOND_SNAP_RADIUS_RATIO; behaviour is unchanged. - Context option styling: the new CSS variables (
--border-soft,--checked-text,--surface-app,--surface-input) all come fromPALETTEthroughui_css, so no colours are duplicated. tests/test_web_adapter.pypasses (193) on this head.
Still open: Text/Benzene targets from SVG hit areas, the 0.528 press band, label trimming/layout, keyboard map, wheel/zoom, the duplicated ring-preference rule, history memory, lost-response resync and idle session expiry.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed c7b05f4.
- Bond press band, ring target and ring-preference rule: verified in their threads.
- Native side: 0.528 and the template gate 0.35 became
STRUCTURE_BOND_PICK_RADIUS_RATIO/TEMPLATE_BOND_GATE_RATIOin hit testing, selection and insertion with the same values;_ring_contains_edgemoved intofeatures.graphwithout a behaviour change. - SVG atom hit circles now use
atom_pick_radiusfrom the same metrics. tests/test_web_adapter.pyandtests/test_ring_label_clearance.pypass (247) on this head.
Still open: Text tool DOM targets, label trimming/layout, keyboard map, wheel/zoom, history memory, lost-response resync and idle session expiry.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed 33846df.
- History storage and per-edit render cost: verified in the thread.
- Lost-response recovery: works; one UX follow-up inline.
- Session locking: per-session
RLockplus theclosedflag closes the race betweencloseand a concurrent dispatch; new sessions still register only after a successful first dispatch and must start at revision 0. tests/test_web_adapter.pypasses (226) on this head.
Still open: Text tool DOM targets, label trimming/layout, keyboard map, wheel/zoom and idle session expiry.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed a0b58fd.
- Native side:
_hydride_layout,_token_anchor_layout,_attachment_at_endand_open_directionmoved intofeatures.annotations.label_layout.atom_label_presentationunchanged; nothing still calls the old methods. - Transport: 4xx rejections other than 409 no longer resync, and stale revisions now return 409. That resolves my earlier comment on the catch block.
- Label and annotation tests pass (1,340, 3 skipped for RDKit) on this head.
New findings are inline: the pt → scene-unit size depends on the Qt platform DPI, labelled bonds are still trimmed by 5 units, and every response now costs a second full-document request.
Still open: Text tool DOM targets, label trimming, keyboard map, wheel/zoom and idle session expiry.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed bba7a9a.
- Native side:
ZOOM_MIN/MAX/STEP, the wheel base 1.0015 and the angle→pixel divisor 2 moved intomain_window_configwith the same values;CanvasPointerControllerandinput_view_accessbehave as before. - Browser: plain wheel scrolls, zoom limits and toolbar steps use the shared policy, and the browser-only pan gesture is removed.
- Zoom, wheel, pointer and architecture tests plus
tests/test_web_adapter.pypass (763, 3 skipped) on this head.
Two small parity notes are inline. Still open: Text tool DOM targets, label size units, label trimming, the extra labels round trip, keyboard map and idle session expiry.
| if (event.ctrlKey) { | ||
| if (!dy) return view; | ||
| // Browser deltas have the opposite sign to Qt's wheel deltas. | ||
| return zoomView(view, viewport, policy.wheel_base ** (dy * policy.angle_per_pixel), policy, event.position); |
There was a problem hiding this comment.
Zoom per mouse-wheel notch is larger than Qt's (P3).
Qt gets angleDelta().y() == 120 per notch, so one notch zooms by 1.0015^120 ≈ 1.197. Here the exponent is pixel delta × 2. Chrome reports about 100 px per notch, which gives 1.0015^200 ≈ 1.350, i.e. about 35% per notch against Qt's 20%. The / 2 in Qt converts angle to scroll pixels; it isn't the inverse mapping for browser pixel deltas, whose per-notch size varies by browser and OS. Normalising discrete wheel input to Qt's 120-per-notch angle (for example, treating deltaMode === 1 lines and large integral pixel steps as notches) would bring the step in line; trackpad and pinch deltas can stay proportional.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed 88a73e0.
- Font units: SVG runs use integer pixel sizes that match Qt under the app's pinned 96 DPI (I corrected my earlier 4/3 comment in its thread).
- Labels round trip: no document payload; layouts are cached per presentation and translated per atom.
place_browser_labelsvalidates size, label tuples (≤2,000) and metric values before placing runs. tests/test_web_adapter.pyandtests/test_application_cli.pypass (401) on this head.
Still open: Text tool DOM targets, label-box bond trimming, macOS zoom modifier, zoom per wheel notch, keyboard map and idle session expiry.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed 9dc846c.
- Native side:
handle_bond_hotkeynow delegates tobond_shortcut_style. I checked each branch against the removed code: thedouble_eitherrefusal set, Shift+B/H/D, l/c/r with order ≠ 2 (still unhandled, now by falling through to the fuse checks, which don't match those keys) and 1/2/3/b/w/h/d give the same results.handle_generic_hotkeymaps the same six keys throughTOOL_HOTKEYS, and X still sets the default single style. - Browser: Escape, X, tool hotkeys, hovered-bond style keys and the macOS zoom modifier now follow the desktop.
- Shortcut, caps-lock, keyboard-focus and web adapter tests pass (543) on this head.
One conflict is inline. Still open: Text tool DOM targets, label-box bond trimming, zoom per wheel notch, atom/bond growth hotkeys and idle session expiry.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed fbcf389 (tests and CI only).
test_browser_font_pixels_match_pinned_native_fontruns underoffscreen_application, which setsAA_Use96Dpiand, on Windows,QT_FONT_DPI=96and thewindowsplatform. The comparison therefore uses the same pinned configuration asapplication.main. The newisValid()assertion keeps a missing Arial from passing silently.- The Windows native packaging job, including the new step, passed on this commit.
- Nit:
qt_platform()already selectswindowson win32 and overrides the environment, so the step'sQT_QPA_PLATFORM: windowsis redundant (harmless). - macOS CoreText isn't covered, because tests there run offscreen. I checked it by hand under cocoa with
AA_Use96Dpi: 10 / 7.2 / 11 / 8.5 pt resolve to 13 / 9 / 15 / 11 px, which matchesbrowser_font_pixels. The occasionalplatform.ymlmacOS run would be the place to pin it if you want CI coverage.
No new findings. Open items are unchanged from the previous review.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed b177389.
- Native side:
label_non_carbon_atomsnow callsatom_label_service.add_or_update_atom_label(..., record=False, allow_merge=False, show_carbon=False)directly. That is exactly whatatom_label_access.add_or_update_atom_labelforwarded with its defaults.handle_atom_hotkey/handle_bond_hotkeyonly split into event normalisation plushandle_*_text.mirrored_local_pointsand thepoint_factoryparameters keepQPointFas the default.ROTATE_ARROW_ANGLES/NUDGE_*have no other references.
- Browser growth:
run_recorded_additions_actionis adapted by discarding the whole candidate when the action declines. That matches the native rollback, andtest_hover_growth_declined_after_mutation_discards_whole_candidatecovers a decline after partial mutation.test_repeated_hover_fusion_matches_native_occupied_sidescompares repeated fusion against the real canvas. - Shortcut, keyboard, growth, structure-build, atom-label and web adapter tests pass (849) on this head.
- Nit: moving the Qt imports into four methods also rebuilds the rotate/nudge key maps on every key event. A module-level lazy constant would keep the service importable without Qt and avoid that. It's harmless as is.
No new findings. Still open: Text tool DOM targets, label-box bond trimming, zoom per wheel notch, Enter/charge-mark hover keys, delete under the pointer and idle session expiry.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed e727e4b.
- Native Delete:
hover_delete_targetreproduces the removed branch exactly. A bonded labelled atom has its label stripped without clearing the hover. Otherwise the atom or bond is deleted afterclear_hover_highlight._atom_has_bondmoved with it. - Native prompt:
atom_label_prompt_initial+apply_atom_label_promptkeep the old initial text and the empty →C/show_carbon=Falserule.record=Trueis now passed explicitly, which is the service default, so the updated mock expectation reflects no behaviour change. - Browser: Enter opens the native prompt for the atom resolved on the server, Cancel leaves the document alone, and Delete without a selection uses the same hover target.
- Delete, input-controller, shortcut, keyboard, atom-label and web adapter tests pass (892) on this head.
One gap that applies to every browser edit is inline; I should have caught it in the first review. Still open: Text tool DOM targets, label-box bond trimming, zoom per wheel notch, charge-mark hover keys and idle session expiry.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed 408207a.
- Native side:
scene_pos_in_sheetkeepsQRectF.containssemantics (edges inclusive, null/empty rect allows everything).OFF_SHEET_EDIT_GUIDANCEmoved intosheet_setup_logicunchanged. - Browser:
- The sheet size now comes from
sheet_dimensions_px, which also removes the earlier A3/A5/Legal half-unit rounding difference. - The paper is centred like the desktop sheet.
- Pointer, benzene, atom, prompt, hover-key and hover-delete edits reject off-sheet positions.
- Bond previews cancel when they leave the sheet, while Select and the eraser stay exempt.
- The sheet size now comes from
- Sheet, pointer, insert/input-controller and web adapter tests pass (767) on this head.
No new findings. Still open: Text tool DOM targets, label-box bond trimming, zoom per wheel notch, charge-mark hover keys and idle session expiry.
dhsohn
left a comment
There was a problem hiding this comment.
I re-reviewed 79169a4 (Qt-side extraction; the browser isn't connected yet, as the doc note says).
glyph_convex_hullis the removed hull code unchanged.glyph_contour_clip_tmatches the removed_glyph_line_clip_tloop:- Contours are divided by 64 and re-multiplied for the intersection test, which is exact in binary floating point.
- The offset-band branch uses the same unscaled coordinates.
- Per-outline
containshits collapse tostart_inside/end_insidewithout changingmin/max.
- Clipping, clearance, scene-geometry, renderer and ring-label tests pass (651, 153 subtests), and ruff is clean on both files.
One small behaviour difference is inline. Still open: browser glyph contours for bond trimming, Text tool DOM targets, zoom per wheel notch, charge-mark hover keys and idle session expiry.
A calculation plan does not draw, so a document carrying one no longer opens read-only. The desktop's snapshot rule, keep the plan only while its component references match the graph, moves into a shared domain function with its warnings; the desktop snapshot and the browser's edit finalization both call it, so an edit that breaks the plan leaves it out of that version with the desktop's warning and Undo restores it.
The browser opened documents read-only when an atom's charge or radical annotation did not match its marks, a guard the desktop does not have: it loads such documents and resynchronizes an atom's annotation only when its marks change, through the same functions the browser calls. The guard, and its reason that also named isotopes the model cannot hold, is removed.
A loaded arrow setting beyond a slider's default range now widens the browser slider, as the desktop does, instead of clamping it. The arrow label preview drops its invented Arial on white for the interface font on the paper surface, at a preview size now shared with the desktop dialog.
A new desktop canvas copies the active canvas's bond length, sheet setup and template tool and text settings; the browser started from defaults. The template field lists move to the Qt-free window config, and the browser's New copies the same document settings from the open drawing.
The depth-cache rules (valid projection, current 3D coordinate, ring centre, offset normal, bond length rescale, saved record) lived inside Qt modules. They are now plain functions in domain/document/perspective.py so the browser adapter can apply the same rules.
The drawing now supplies the desktop's 3D geometry ports from the stored depth points, so ring double bonds project as on the desktop. Moves and transforms carry points through the shared move controller, a bond length change rescales them, and each accepted edit saves only the points that still project onto a live atom. The Perspective Rotation tool stays unconnected.
Group membership, connection preflight, bond and merge extensions, the Edit > Group plan and saved-record conversion move from Qt modules into features/groups. The growth service checks group connections itself, and the group box's padding, corner radius and dash pattern become shared constants, so the browser adapter can apply the same rules.
The adapter keeps the desktop's group state with candidate record identities as scene record ids. Edit > Group and Ungroup (Ctrl+G, Ctrl+Shift+G) use the shared plan, connection preflights refuse joining two groups, new bonds and label merges extend the owning group, and Align and Distribute treat a group as one object. The browser completes a selection to whole groups, toggles a group as one unit and draws the desktop's dashed group box.
An upright image's rotation, which orbits its box's center, moves into features/annotations beside the mirrored box position, and the selected image outline's padding becomes a shared constant. Validated image sources are remembered by the identity of their immutable base64 text, so a document revalidated after each edit does not decode its rasters again.
Images render at their document boxes and stacking depth, and select, move, delete, rotate, flip, stack, align and group like the desktop. Image sources travel only on open and on Save's export answer; other session responses carry a content reference, which the browser fetches once as a Blob URL. Atom input becomes a session query, so no request sends the document back.
The Insert Image placement, centred in the visible part of the sheet at no more than 70% of it, becomes inserted_image_box in the image domain. The Image Properties dialog's labels, ranges, decimals and messages move into IMAGE_PROPERTIES_SPEC, which the Qt dialog now reads.
Insert Image sends the chosen file with the visible scene rect and the adapter applies the desktop's validation, budget and placement, then the new image is selected. Image Properties follows the shared spec, sends only the changed fields and keeps the aspect lock on the pixel ratio.
## Motivation SMILES insertion needed to reuse the existing placement and commit behavior without importing Qt in the browser runtime. ## Changes Added the browser insertion preview and one-click commit using the existing RDKit conversion and placement services. Preserved chemical annotations, single-step undo/redo, cancellation, and stale-response handling. Kept RDKit optional and updated browser documentation and CI coverage. ## Verification Passed make check on Linux with Python 3.12: Ruff, formatting, mypy, and 463 separately executed Python test files. Passed node --test tests/web_adapter.test.mjs (85 tests), git diff --check, and patch application against the baseline. Nine platform-specific cases were skipped. Native macOS/Windows and real browser screen interaction were not verified; the browser environment blocked loopback access.
Keep pending caret styles cumulative, clear them on navigation, and prevent late note-markup replies from overwriting newer input. Preserve formatted undo/redo snapshots and conservatively handle unmatched or replacement input. Add regression and native-persistence coverage with browser documentation. Validation: Linux/WSL make check passed across 463 isolated test files: 16576 passed, 404 skipped. Ruff, format and mypy passed. The browser adapter passed 8230 tests with 11 RDKit skips, including the 115-test Node suite. Astra verified the unchanged candidate on Windows Edge and Linux Chromium (19/19 each). Independent Opus 5.5 high review found no blocking findings. The local contract validator was 841bb55; CI pins 38581a7737cd0521bb3d50b59115eefe3c9254ca and must validate that revision. Local skips are not passes. Physical IME/AltGr, native context menus and spellcheck, and macOS remain unverified; this is not release acceptance.
Motivation Pending note text could be omitted from saved copies or discarded after an unsuccessful save. Changes Committed pending notes before saving copies and refused export when the note edit did not complete. Protected New, Open, and unload with the shared pending-note comparison. Added eight behavior regressions. Verification The frozen patch passed the Node web adapter suite: 151 passed, 0 failed. make check passed 465 isolated test-file suites. Retained browser checks covered refused edits, saved downloads, reopening, and cancelled unload. No deployment was performed.
## Motivation I found that browser drawing snap indicators and held-gesture snap reach needed the native screen-space rules. ## Changes Connected native marker geometry to Arrow and Line previews and used the current view scale for each snap request. Added native and browser regressions while retaining the published note-safety changes. ## Verification make -j1 check: 465 test files, 16,615 passed and 404 skipped; lint, format and type checks passed on the exact result tree. Browser gesture checks passed. Zoom-only cached previews and cross-zoom late replies remain outside the verified scope.
## Motivation A preview requested before a zoom could arrive after the view scale changed. ## Changes Discard late Line and Arrow replies when their requested scale differs from the live scale, without automatic re-requests. Add separate delayed-reply regressions. ## Verification The exact result tree passed make -j1 check with CHECK_JOBS=1. Focused delayed-preview tests, the complete browser adapter suite, Python adapter tests and real Edge pointer/keyboard checks passed. This is a browser safety rule, not native GUI equivalence or complete browser integration. Cached-preview display differences and individual fit, actual-size and resize regressions remain outside this change.
## Motivation The browser preview needed a downloadable figure using the existing native export path. ## Changes Added whole-document plain SVG export at original scale, a browser download action, and matching documentation. Added figure-export and timeout-cleanup regressions. ## Verification The exact source candidate passed make -j1 check with CHECK_JOBS=1: 16617 passed and 404 skipped. The Windows Node suite passed 167 tests. Actual Edge checks passed 24 assertions, including a 2016-byte SVG equal to the Linux offscreen native export. All 1085 source files were matched byte-for-byte and mode-for-mode before commit. Windows-native Qt equivalence, other figure formats and scopes, packaging, and release-platform checks are outside this local verification. CI is pending. Session-lock latency and timeout-test sensitivity remain accepted low-priority limitations.
## Motivation I found that the confirmed-quit path left two closed windows alive until interpreter teardown because no event loop delivered their deferred deletion. ## Changes Delivered pending DeferredDelete events while QApplication is alive. Added a regression asserting both real windows are destroyed after cleanup. Preserved the original seven behavior tests. The flush is application-wide and can expose previously queued deletion defects. ## Verification The regression failed with the old fixture on Python 3.13.15 on both the branch and CI merge snapshot, then passed after this cleanup fix. Related application lifetime and recovery tests passed. make check passed across 467 test files, Ruff, format and mypy. The native CI SIGSEGV was not reproduced locally, so its causal link remains unconfirmed. New-commit CI must verify the failing job.
Keep compatibility schemas and validator project-local, preserve native case-alias save conflict checks, and add conservative GUI/CLI CDXML export with tested fail-closed boundaries.
Preserve coverage reporting and volatile macOS restoration protection alongside project-local compatibility validation.
Motivation
The browser adapter connects the existing Chemvas editor to browser input and SVG presentation while retaining Qt as the complete desktop editor. Chemical selection Copy, Cut and Paste now use the canonical selection payload, and accepted edits receive automatic recovery drafts that survive tab reloads and server restarts.
Changes
.chemvasdrawings and downloads saved copies, MOL selections and plain SVG figures through existing exporters. Save remains a downloaded copy, so its dirty marker and recovery draft remain. Optional SMILES insertion uses RDKit.Full browser/Qt parity remains incomplete. Remaining object handles, panels, Perspective Rotation, overwrite-save and additional publication-export formats are not connected. Browser clipboard transport is plain text; direct Qt custom-MIME clipboard exchange is not implemented. Font engines, dialogs, input methods and some browser interactions differ. Uncommitted note text and edits whose draft write failed are not guaranteed recoverable. Unknown interrupted-write staging files are retained to avoid deleting another program's files; a cleanup warning can linger after a successful retry until the next successful draft write. Qt retirement remains outside this change.
Verification
make checktarget viascripts/check.sh), Python 3.13.14, two file-isolated workers plus four serial Cocoa workflows: Ruff, formatting and mypy passed; 16,885 tests and 1,036 subtests passed, 424 skipped. Line coverage 94.40%; branch coverage 86.52%. RDKit was not installed locally; optional backend and packaging checks remain with their CI jobs.All six checks passed on commit
1e1df77b: Linux Python 3.12 and 3.13 common suites, RDKit smoke tests, Windows packaging, wheel smoke test and secret scanning. Linux common coverage is 94.34% lines for both Python versions, with 86.52% branches on Python 3.12 and 86.53% on Python 3.13; the selected RDKit scope is 70.03% lines / 52.07% branches. CI run.Approved for merge and local browser use.