Consolidate seed-geometry widgets + fix stale-disabled checkbox bug (UXP2.5) — v0.5.1 - #36
Merged
Merged
Conversation
…XP2.5)
Geometry Opt, Frequency, and UV-Vis (TD-DFT) each built their own
near-identical seed-geometry dropdown + refresh button + note. Only one of
the three panels is ever visible at a time, so there was no benefit to three
widget instances.
Now there is one real widget group (app._seed_dd / _seed_refresh_btn /
_seed_note); the three historical per-calc-type attribute names
(_geo_seed_dd, _freq_seed_dd, _tddft_seed_dd, and the button/note
equivalents) are kept as aliases onto the same object rather than renamed
everywhere. That collapses the actual duplication - three widget instances,
three refresh wrappers, three change handlers, three observer/click
bindings, each down to one - without forcing a rename across every
consumption site and every existing test that addresses these widgets by
their historical names.
The one behavioral difference the three handlers had is preserved by making
the single on_seed_changed calc-type-aware: it gates _freq_preopt_cb only
when the active calc type is Frequency or UV-Vis, and leaves it alone for
Geometry Opt (and everything else) exactly as UXP2.4's test already
guaranteed.
Two real bugs surfaced and fixed while touching this code, neither
introduced by the consolidation:
- on_calc_type_changed only ever set _freq_preopt_cb.disabled = True and
never reset it to False on a plain calc-type switch. Pick a seed on
Frequency, switch to Single Point, and the checkbox stayed disabled with
no way back short of returning to the now-hidden Frequency panel and
clearing the seed. It now re-evaluates .disabled from the seed value on
every switch into/out of Frequency/UV-Vis and resets it for every other
calc type.
- _set_molecule only re-filtered the Frequency/UV-Vis seed dropdowns on a
molecule change, because Geometry Opt's seed dropdown didn't exist yet
when that code was written. One shared dropdown means the single refresh
call now covers Geometry Opt too.
Two existing tests needed a one-line addition (setting calc_type_dd.value)
because they exercised the old tddft-specific handler by setting the
dropdown value directly, relying on which widget you touched to imply
intent - a safe assumption before this change, not after. Two more tests
that asserted the pre-consolidation invariant outright were rewritten to
assert the new one, with the reasoning inline. Three new regression tests
cover the stale-disabled fix.
Suite: 1971 passed / 17 skipped (was 1968). pre-commit clean.
Version bump to 0.5.1 + CHANGELOG entry, since this ships a real bug fix
(the stuck-disabled checkbox) alongside the cleanup.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
M-UX2 UXP2.5. Bumps to 0.5.1 — good real-world test of whether the
release: [published]trigger fires on its own this time (see the release.ymlsaga on
v0.5.0).What changed
Geometry Opt, Frequency, and UV-Vis (TD-DFT) each built their own
near-identical seed-geometry dropdown + refresh button + note. Only one of the
three panels is ever visible at a time, so there was no benefit to three
widget instances — this collapses them to one.
Design choice: one real widget group (
app._seed_dd/_seed_refresh_btn/_seed_note); the three historical per-calc-type names(
_geo_seed_dd,_freq_seed_dd,_tddft_seed_dd, and the button/noteequivalents) are kept as aliases onto the same object, not renamed
everywhere. That removes the actual duplication — three widget instances,
three refresh wrappers, three change handlers, three observer/click bindings,
each down to one — without a rename sweeping every consumption site (
_do_run)and every existing test that addresses these widgets by their historical names.
The one real behavioral difference the three handlers had is preserved by
making the single
on_seed_changedcalc-type-aware: it gates_freq_preopt_cbonly when the active calc type is Frequency or UV-Vis, andleaves it alone for Geometry Opt (and everything else) — exactly what UXP2.4's
test already guaranteed.
Two real bugs found and fixed, not introduced by this change
on_calc_type_changedonly ever set
_freq_preopt_cb.disabled = True(via the per-calc-type seedhandlers) and never reset it to
Falseon a plain calc-type switch. Pick aseed on Frequency, switch to Single Point, and the checkbox stayed disabled
with no way back short of returning to the now-hidden Frequency panel and
clearing the seed. It now re-evaluates
.disabledfrom the seed value onevery switch into/out of Frequency/UV-Vis, and resets it for every other
calc type.
_set_moleculeonly re-filtered the Frequency/UV-Vis dropdowns, becauseGeometry Opt's seed dropdown didn't exist yet when that code was written.
One shared dropdown means the single refresh call now covers Geometry Opt
too — switching molecules while already on that panel no longer leaves it
showing the previous molecule's matches.
Test changes — why, not just what
(
app.calc_type_dd.value = "UV-Vis (TD-DFT)") because they exercised the oldon_tddft_seed_changedby setting the dropdown value directly, relying onwhich widget you touched to imply intent. Safe before this change, not
after — with one shared dropdown, calc type is the only signal left.
test_geo_seed_geometry.pyasserted the pre-consolidationinvariant outright (
app._geo_seed_dd is not app._freq_seed_dd) — rewrittento assert the new one, with the reasoning inline rather than silently
flipped.
TestPreoptCheckboxDoesNotGetStuckDisabled) cover thestale-disabled fix specifically: re-enabling on switch to Single Point, the
checkbox staying gated when carrying a seed between Frequency and UV-Vis,
and Geometry Opt still hiding it regardless of a pending seed.
Testing
Full suite: 1971 passed, 17 skipped (was 1968 — net +3 from the new
regression tests).
pre-commit run --all-filesclean.Version
Bumped to 0.5.1 with a CHANGELOG entry, since this ships the stuck-checkbox fix
as user-visible behavior, not just internal cleanup.
🤖 Generated with Claude Code