refactor: remove variable listeners#2525
Merged
Merged
Conversation
The action is stateless (the solution type only occurs in a parameter position), yet it was allocated on every triggerVariableListeners() call - several times per evaluated list move. One shared instance now serves every solution type via an erasure-cast accessor.
… destination entity SelectorBasedListRuinRecreateMove recorded the before-change snapshot for a newly-targeted destination entity starting at index 0, while the matching after-change notification started at the entity's first unpinned index. On undo, this left the pinned prefix uncleared and then reinserted the full original list in front of it, duplicating the pinned elements. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Timefold Solver’s internal “variable listener” infrastructure into a unified “shadow variable update” mechanism, renaming public/semipublic APIs accordingly and replacing the old listener-queue implementation with ShadowVariableSupport and event-free change handlers.
Changes:
- Rename
triggerVariableListeners()→updateShadowVariables()(with deprecation bridges) and update all usages/tests accordingly. - Replace the old variable-listener support and related notification/event classes with
ShadowVariableSupport,BasicVariableState*, and list/basic change-handler dispatch. - Add/adjust tests to cover undo ordering, list change notifications, inverse/basic state supplies, and declarative shadow behavior.
Reviewed changes
Copilot reviewed 124 out of 124 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pom.xml | Add docs module to reactor build. |
| core/src/test/java/ai/timefold/solver/core/testdomain/shadow/declarative/basicinverse/TestdataBasicInverseSolution.java | New test solution for basic-inverse declarative shadow. |
| core/src/test/java/ai/timefold/solver/core/testdomain/shadow/declarative/basicinverse/TestdataBasicInverseOwner.java | New test fact/value type. |
| core/src/test/java/ai/timefold/solver/core/testdomain/shadow/declarative/basicinverse/TestdataBasicInverseGroup.java | New planning entity for test model. |
| core/src/test/java/ai/timefold/solver/core/testdomain/shadow/declarative/basicinverse/TestdataBasicInverseEntity.java | New entity with declarative shadow. |
| core/src/test/java/ai/timefold/solver/core/impl/solver/change/DefaultProblemChangeDirectorTest.java | Update tests to new shadow-update API. |
| core/src/test/java/ai/timefold/solver/core/impl/score/stream/test/SingleConstraintAssertionTest.java | Rename test method to new API naming. |
| core/src/test/java/ai/timefold/solver/core/impl/score/stream/test/MultiConstraintAssertionTest.java | Rename test method to new API naming. |
| core/src/test/java/ai/timefold/solver/core/impl/score/stream/common/AbstractConstraintStreamTest.java | Use updateShadowVariables() before scoring. |
| core/src/test/java/ai/timefold/solver/core/impl/score/stream/bavet/BavetRegressionTest.java | Rename queue-clear method to pending-update clear. |
| core/src/test/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirectorSemanticsTest.java | Update semantics tests to new API. |
| core/src/test/java/ai/timefold/solver/core/impl/move/RecordedUndoMoveTest.java | New test for undo replay order. |
| core/src/test/java/ai/timefold/solver/core/impl/move/MoveDirectorTest.java | Add assertions for inverse relation correctness on undo. |
| core/src/test/java/ai/timefold/solver/core/impl/move/ListVariableBeforeChangeActionTest.java | New tests for list undo notifications. |
| core/src/test/java/ai/timefold/solver/core/impl/move/ListVariableAfterChangeActionTest.java | New tests for list undo notifications. |
| core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/SelectorBasedListSwapMoveTest.java | Update listener-trigger verification to shadow updates. |
| core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/SelectorBasedListRuinRecreateMoveTest.java | Add regression test for pinned-prefix ruin/recreate undo. |
| core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/SelectorBasedListAssignMoveTest.java | Update listener-trigger verification to shadow updates. |
| core/src/test/java/ai/timefold/solver/core/impl/heuristic/selector/entity/pillar/DefaultPillarSelectorTest.java | Switch mocked supply manager to ShadowVariableSupport. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/violation/SolutionTrackerTest.java | Update violation tests and terminology. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/violation/ListVariableTrackerTest.java | Update tracker tests to new handler callbacks. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/violation/BasicVariableTrackerTest.java | Rename/fix tracker test class and API usage. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/violation/AllVariablesAssertTest.java | Package move after violation refactor. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/ShadowVariableUpdateTest.java | Remove unsupported-type test; align helper behavior. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/ShadowVariableSupportTest.java | Major new/updated coverage for ShadowVariableSupport. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/inverserelation/ExternalizedCollectionInverseVariableSupplyTest.java | Delete superseded inverse-supply test. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/inverserelation/CollectionInverseVariableListenerTest.java | Delete superseded inverse-listener test. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/ExternalizedBasicVariableStateSupplyTest.java | New tests for basic-variable inverse supply. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/declarative/BasicInverseShadowVariableTest.java | New test for map-mode inverse supply path. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/cascade/SingleCascadingUpdateShadowVariableTest.java | Rename class and update to new API. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/variable/cascade/DifferentCascadingUpdateShadowVariableTest.java | Rename class and update to new API. |
| core/src/test/java/ai/timefold/solver/core/impl/domain/solution/ConstraintWeightOverridesTest.java | Update tests to new API. |
| core/src/test/java/ai/timefold/solver/core/config/solver/EnvironmentModeTest.java | Strengthen assertions around undo corruption and inverse state. |
| core/src/main/java/module-info.java | Stop exporting removed listener-support package. |
| core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolver.java | Call updateShadowVariables() after problem changes. |
| core/src/main/java/ai/timefold/solver/core/impl/solver/DefaultSolutionManager.java | Update imports to moved violation package. |
| core/src/main/java/ai/timefold/solver/core/impl/solver/change/DefaultProblemChangeDirector.java | Delegate to updateShadowVariables(). |
| core/src/main/java/ai/timefold/solver/core/impl/score/stream/test/AbstractConstraintAssertion.java | Rename terminology + pending-update clear hook. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/VariableDescriptorAwareScoreDirector.java | Update Javadoc wording to “shadow variable update”. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/stream/BavetConstraintStreamScoreDirector.java | Expose clearPendingShadowVariableUpdates() override and assert up-to-date shadows. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/ScoreDirector.java | Introduce updateShadowVariables() and deprecate old method. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/InnerScoreDirector.java | Introduce force-update API and deprecations. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/incremental/IncrementalScoreDirector.java | Assert shadows up-to-date before scoring. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/easy/EasyScoreDirector.java | Assert shadows up-to-date before scoring. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/CorruptionAnalyzer.java | Switch to ShadowVariableSupport for violation messages. |
| core/src/main/java/ai/timefold/solver/core/impl/score/director/AbstractScoreDirector.java | Replace listener-support field with ShadowVariableSupport. |
| core/src/main/java/ai/timefold/solver/core/impl/phase/AbstractPhase.java | Update corruption message terminology. |
| core/src/main/java/ai/timefold/solver/core/impl/move/VariableChangeRecordingScoreDirector.java | Record UpdateShadowVariablesAction instead of trigger action. |
| core/src/main/java/ai/timefold/solver/core/impl/move/UpdateShadowVariablesAction.java | New change action for shadow updates. |
| core/src/main/java/ai/timefold/solver/core/impl/move/TriggerVariableListenersAction.java | Delete old trigger action. |
| core/src/main/java/ai/timefold/solver/core/impl/move/RecordedUndoMove.java | Replay undo actions in reverse order. |
| core/src/main/java/ai/timefold/solver/core/impl/move/MoveDirector.java | Use updateShadowVariables() after move operations. |
| core/src/main/java/ai/timefold/solver/core/impl/move/ListVariableBeforeChangeAction.java | Avoid duplicate/no-op list notifications during undo. |
| core/src/main/java/ai/timefold/solver/core/impl/move/ListVariableAfterChangeAction.java | Notify collapsed list range after undo clear. |
| core/src/main/java/ai/timefold/solver/core/impl/move/ChangeAction.java | Replace permitted trigger action with update action. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/SelectorBasedRuinRecreateMove.java | Switch to updateShadowVariables() in execution flow. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/ruin/SelectorBasedListRuinRecreateMove.java | Switch to updateShadowVariables() and adjust list notification indices. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/selector/move/generic/list/kopt/DelegatingListVariableStateSupply.java | Adapt to new list change handler API. |
| core/src/main/java/ai/timefold/solver/core/impl/heuristic/move/AbstractSelectorBasedMove.java | Trigger shadow updates after executing move. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/violation/VariableSnapshotTotal.java | Package move for violation types. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/violation/VariableSnapshot.java | Package move for violation types. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/violation/VariableId.java | Package move for violation types. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/violation/SolutionTracker.java | Refactor to basic/list trackers and rename messages. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/violation/ShadowVariableSnapshot.java | Update violation wording and updater-class reporting. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/violation/ShadowVariablesAssert.java | Update terminology for “force update shadows”. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/violation/ListVariableTracker.java | Convert from listener/event to handler API. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/violation/BasicVariableTracker.java | Convert from listener/event to handler API and rename class. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/VariableListener.java | Delete old listener interface. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ShadowVariableUpdateHelper.java | Remove shadow-type gating; align with new support model. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ShadowVariableSupport.java | New unified dispatch/support implementation. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/nextprev/PreviousElementShadowVariableDescriptor.java | Rename “listener classes” to “updater classes” API. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/nextprev/NextElementShadowVariableDescriptor.java | Rename “listener classes” to “updater classes” API. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/nextprev/AbstractNextPrevElementShadowVariableDescriptor.java | Switch to single-source descriptor API. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ListVariableStateSupply.java | Convert to list change handler interface. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ListVariableState.java | Update comments/terminology for shadow updates. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ListVariableListener.java | Delete old list listener interface. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ListVariableChangeHandler.java | New list change handler interface. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/VariableListenerWithSources.java | Delete old listener-with-sources helper. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/VariableListenerNotifiable.java | Delete old notifiable implementation. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/VariableChangedNotification.java | Delete old notification type. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/ShadowVariableType.java | Delete old enum. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/Notification.java | Delete old notification API. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/NotifiableRegistry.java | Delete old registry. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/Notifiable.java | Delete old notifiable interface. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/ListVariableNotification.java | Delete old list notification interface. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/ListVariableListenerNotifiable.java | Delete old list notifiable. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/ListVariableChangedNotification.java | Delete old list change notification. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/EntityNotification.java | Delete old entity notification. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/EntityNotifiable.java | Delete old entity-notifiable interface. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/ElementUnassignedNotification.java | Delete old element-unassigned notification. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/BasicVariableNotification.java | Delete old basic notification interface. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/AbstractNotification.java | Delete old base notification. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/support/AbstractNotifiable.java | Delete old base notifiable. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/SourcedVariableListener.java | Delete old sourced-listener interface. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/SourcedListVariableListener.java | Delete old sourced list listener. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/listener/SourcedBasicVariableListener.java | Delete old sourced basic listener. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ListElementsChangeEvent.java | Delete old list change event record. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/inverserelation/InverseRelationShadowVariableDescriptor.java | Switch inverse tracking to BasicVariableStateDemand. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/inverserelation/ExternalizedCollectionInverseVariableSupply.java | Delete old inverse supply implementation. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/inverserelation/CollectionInverseVariableListener.java | Delete old inverse listener implementation. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/inverserelation/CollectionInverseVariableDemand.java | Delete old inverse demand type. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/IndexShadowVariableDescriptor.java | Switch to single-source descriptor API. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ExternalizedListVariableStateSupply.java | Convert from event-based to handler-based callbacks. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ExternalizedBasicVariableStateSupply.java | New combined externalized/map-mode basic inverse supply. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/descriptor/VariableDescriptor.java | Remove sink-descriptor tracking; simplify descriptor graph. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/descriptor/ShadowVariableDescriptor.java | Replace multi-source API with single-source/updater API. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/descriptor/ListVariableDescriptor.java | Update inverse lookup to single-source API. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/descriptor/BasicVariableDescriptor.java | Add state demand accessor for basic variable inverse tracking. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/VariableReferenceGraph.java | Update documentation around update ordering. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/ShadowVariablesInconsistentVariableDescriptor.java | Adapt to updater-class API. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/EntityConsistencyState.java | Update terminology re: declarative sources. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/DeclarativeShadowVariableDescriptor.java | Adapt to updater-class API. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/declarative/ChangedVariableNotifier.java | Switch inverse supply demand type. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ChangeEvent.java | Delete old change-event sealed interface. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/cascade/CascadingUpdateShadowVariableDescriptor.java | Adapt to updater-class API. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/BasicVariableStateSupply.java | New API for basic inverse state supply. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/BasicVariableStateDemand.java | New demand for basic inverse state supply. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/BasicVariableListener.java | Delete old basic listener interface. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/BasicVariableChangeHandler.java | New event-free basic change handler API. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/variable/BasicVariableChangeEvent.java | Delete old basic change event record. |
| core/src/main/java/ai/timefold/solver/core/impl/domain/solution/descriptor/SolutionDescriptor.java | Replace global shadow ordering logic with single-source traversal. |
| core/src/main/java/ai/timefold/solver/core/api/solver/SolutionUpdatePolicy.java | Update API docs to shadow-update terminology. |
| core/src/main/java/ai/timefold/solver/core/api/solver/change/ProblemChangeDirector.java | Update API docs to shadow-update terminology. |
Comments suppressed due to low confidence (2)
core/src/main/java/ai/timefold/solver/core/impl/domain/variable/violation/SolutionTracker.java:166
- The corruption report message includes an extraneous
")in the header for missing undo events, which makes the output look broken.
core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ShadowVariableSupport.java:481 - clearPendingShadowVariableUpdates() only flips the dirty flag but leaves accumulated list-change/unassigned-element state behind, which can be replayed later and cause incorrect cascading updates.
Christopher-Chianelli
approved these changes
Jul 20, 2026
Christopher-Chianelli
left a comment
Christopher-Chianelli
left a comment
Contributor
There was a problem hiding this comment.
LGTM; although I personally would call it a simplification of shadow variables. Should we do the backport of the fix to UndoRecordedMove in this PR to 1.x?
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 123 out of 123 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ShadowVariableSupport.java:499
- clearPendingShadowVariableUpdates() only flips the dirty flag but leaves listVariableChangeList/unassignedValueWithEmptyInverseEntitySet populated, so a later updateShadowVariables() can incorrectly replay stale cascading updates that were meant to be discarded.
core/src/main/java/ai/timefold/solver/core/impl/domain/variable/violation/SolutionTracker.java:166 - The corruption message header for undo events includes an extra ")" ("undo move:")") which makes the output incorrect and will break any tests/assertions expecting the exact message.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 123 out of 123 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
core/src/main/java/ai/timefold/solver/core/impl/domain/variable/violation/SolutionTracker.java:163
- The score corruption message text includes an extra
")suffix, which makes the diagnostic output confusing and inconsistent with the other headings; remove the stray characters from the string literal.
core/src/main/java/ai/timefold/solver/core/impl/domain/variable/ShadowVariableSupport.java:499 - clearPendingShadowVariableUpdates() only flips the dirty flag and leaves pending list-variable changes/unassigned elements and declarative-session change markers intact, so a later updateShadowVariables() may incorrectly process events that were meant to be discarded; clear the pending collections and recreate the declarative session to drop its queued changes.
|
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.
No description provided.