Repository navigation
Keep orbital handles and atom annotations consistent through Undo - #546
Merged
Merged
Conversation
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.
Motivation
Native Qt testing found that Undo could leave orbital handles at the previous bond length and erase a loaded atom charge when that charge had no drawn mark. Adding a plus mark to annotation-only nitromethane and undoing it removed the nitrogen's original +1 annotation from the saved document. Python 3.13 CI also crashed while creating the sixth canvas in the paper-size tests; those tests recreated QApplication between cases.
Changes
Verification
Full local gate (
bash scripts/check.sh) on exact head1aa74bf5bf77ee821fa8b2013d68070f223ff0d5passed: 16,996 tests, 428 skips and 1,036 subtests on macOS 26.6.2 arm64 / Python 3.13.14 / Qt 6.11.2 without RDKit. Line coverage 94.40%, branch coverage 86.51%. Ruff, format, mypy, architecture/contract checks and four serial Cocoa workflow files passed. Source unchanged.Qt-only wheel and sdist rebuilt from this committed head passed
verify_dist.py,twine check, five clean-installed CLI cases and the installed-wheel CI smoke. The wheel was built from the sdist. Both exclude experimental web files. All 443 wheel member payloads and all 450 sdist file payloads match the a4 artifacts byte-for-byte; archive metadata gives the new builds distinct archive hashes.Lifecycle evidence on macOS 26.6.2 / Python 3.13.14 / Qt 6.11.2 under coverage: the unchanged original file passed its 26 cases but created and released an application in each of the 12 canvas contexts; the repaired file passed all 26 using one shared application. The same guards with only the ownership line removed failed 12 cases and passed 14. Nine related files passed 155 tests plus Ruff, format and mypy; selected coverage was 39.08% lines and 16.76% branches, without RDKit.
All 446 app files and the packaging inputs are byte-identical to a4bcc50. Its bounded actual native Linux Qt rerun remains applicable: Mark-tool and keyboard Undo restored exact whole-document state; Mark Redo made only expected changes; active orbital handles followed 20↔60 without reselection and after reopening; native CDXML matched the validated bytes, and visible-mark export refusal preserved both existing output and document bytes. This does not turn the earlier broader checklist into evidence for the final candidate. Mac-specific Ctrl-click, a frozen installer and ChemDraw interoperability remain unverified.
Independent source review accepted the test-lifetime repair subject to new exact-head Linux Python 3.13 CI. The precise native object that faulted is unknown. The observational probe changes allocation timing, and the new guard deliberately calls gc.collect() after confirming native canvas deletion; neither is claimed to reproduce the original Linux crash or to be timing-neutral.
Original a4 CI run 37223611269 remains recorded as failed: Python 3.13.15 hit a native segmentation fault (exit 139) in
test_sheet_sizes.py; five other checks passed. It was not rerun. The original failure log and exact runner/dependency versions are preserved.New exact-head CI run 37226530647 passed all six checks on its first attempt for
1aa74bf5bf77ee821fa8b2013d68070f223ff0d5. Linux Python 3.12.14 and 3.13.15 each passed 17,013 tests, with 99 skips and 1,036 subtests; coverage was 94.34% lines and 86.51% branches. Both passed all 26 sheet-size tests. The Python 3.13 job used the same Ubuntu image, Python and installed dependency versions as the original failure. RDKit 2026.3.6 selected tests passed 9,008 cases with one skip and 303 subtests; selected coverage was 72.27% lines and 54.78% branches. Windows native packaging, wheel smoke and secret scanning also passed. The independent review's exact-head CI condition is satisfied.Checklist
Notes for reviewers
The annotation command runs after mark removal during Undo so that mark-derived synchronization cannot erase a loaded value. The reverse replay preserves the existing forward behavior. Generic mark deletion, cut, eraser and rebind semantics are unchanged. The Qt-only distribution boundary is preserved. Merged as
04ed9e26fbb848268026cc1ea21987032d8616ef. Exact-main CI and macOS/Windows platform gates passed. Chemvas 0.24.0 is published on GitHub Releases and PyPI; downloaded artifacts matched the release workflow byte-for-byte, and clean-installed CLI and headless CDXML checks passed.