diff --git a/.gitignore b/.gitignore index 746090ed3..204669958 100644 --- a/.gitignore +++ b/.gitignore @@ -77,3 +77,4 @@ Thumbs.db # Claude Code configuration (local settings and skills) .claude/ +desktop-ui/*.xml diff --git a/core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/objects/paths/AbstractPath.kt b/core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/objects/paths/AbstractPath.kt index 0143739c0..016662d35 100644 --- a/core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/objects/paths/AbstractPath.kt +++ b/core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/objects/paths/AbstractPath.kt @@ -11,6 +11,7 @@ package cz.vutbr.fit.interlockSim.objects.paths import cz.vutbr.fit.interlockSim.context.SimulationContext import cz.vutbr.fit.interlockSim.context.SimulationContext.ReportType +import cz.vutbr.fit.interlockSim.domain.ABSOLUTE_MAX_SPEED import cz.vutbr.fit.interlockSim.domain.MINIMAL_MAX_SPEED import cz.vutbr.fit.interlockSim.exceptions.TrackOperationException import cz.vutbr.fit.interlockSim.exceptions.requireSimulation @@ -278,7 +279,7 @@ abstract class AbstractPath protected constructor( // Semaphore element: configure with previous track and switch speed if (previousTrack == null) continue if (context.isSeparatorInDirection(element, previousTrack, null)) { - val speed = previousSwitch?.allowedSpeed() ?: PathElement.ABSOLUTE_MAX_SPEED + val speed = previousSwitch?.allowedSpeed() ?: ABSOLUTE_MAX_SPEED val segment = context.getSegment(element, null, previousTrack) val segment2 = context.getSegment(element, previousTrack, null) element.setUpSpeed(segment, segment2, speed) diff --git a/core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/xml/XmlSchemaContent.kt b/core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/xml/XmlSchemaContent.kt index 40adc59f3..acecdd22d 100644 --- a/core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/xml/XmlSchemaContent.kt +++ b/core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/xml/XmlSchemaContent.kt @@ -106,12 +106,12 @@ object XmlSchemaContent { - - + + - - + + diff --git a/core/src/jvmMain/kotlin/cz/vutbr/fit/interlockSim/xml/XMLContextFactory.kt b/core/src/jvmMain/kotlin/cz/vutbr/fit/interlockSim/xml/XMLContextFactory.kt index e7ee51b8b..98ee40fd8 100644 --- a/core/src/jvmMain/kotlin/cz/vutbr/fit/interlockSim/xml/XMLContextFactory.kt +++ b/core/src/jvmMain/kotlin/cz/vutbr/fit/interlockSim/xml/XMLContextFactory.kt @@ -252,7 +252,7 @@ class XMLContextFactory : EditingContextFactory { /** * Saves an editing context to an output stream. * - * Pre-save validation (Issue #XXX, PR #358): + * Pre-save validation (Issue #80, PR #357): * - Validates InOut count before serialization * - Prevents saving invalid contexts (< MIN_INOUT_ELEMENTS InOuts) * - Returns false if validation fails @@ -293,7 +293,7 @@ class XMLContextFactory : EditingContextFactory { /** * Saves an editing context to a file. * - * Pre-save validation (Issue #XXX, PR #358): + * Pre-save validation (Issue #80, PR #357): * - Validates InOut count before serialization * - Prevents saving invalid contexts (< MIN_INOUT_ELEMENTS InOuts) * - Returns false if validation fails diff --git a/core/src/jvmMain/resources/cz/vutbr/fit/interlockSim/resource/data.xsd b/core/src/jvmMain/resources/cz/vutbr/fit/interlockSim/resource/data.xsd index bcaa28dea..e03109fca 100644 --- a/core/src/jvmMain/resources/cz/vutbr/fit/interlockSim/resource/data.xsd +++ b/core/src/jvmMain/resources/cz/vutbr/fit/interlockSim/resource/data.xsd @@ -96,12 +96,12 @@ - - + + - - + + diff --git a/core/src/jvmTest/kotlin/cz/vutbr/fit/interlockSim/objects/paths/AbstractPathSetUpSemaphoresTest.kt b/core/src/jvmTest/kotlin/cz/vutbr/fit/interlockSim/objects/paths/AbstractPathSetUpSemaphoresTest.kt new file mode 100644 index 000000000..e7f2459f7 --- /dev/null +++ b/core/src/jvmTest/kotlin/cz/vutbr/fit/interlockSim/objects/paths/AbstractPathSetUpSemaphoresTest.kt @@ -0,0 +1,248 @@ +/* Brno University of Technology + * Faculty of Information Technology + * + * BSc Thesis 2006/2007 + * + * Railway Interlocking Simulator + * + * Integration test for AbstractPath.setUpSemaphores() coverage (Issue #357) + */ +package cz.vutbr.fit.interlockSim.objects.paths + +import assertk.assertThat +import assertk.assertions.isGreaterThanOrEqualTo +import assertk.assertions.isTrue +import cz.vutbr.fit.interlockSim.context.DefaultSimulationContext +import cz.vutbr.fit.interlockSim.context.EditingContext +import cz.vutbr.fit.interlockSim.context.EditingContextFactory +import cz.vutbr.fit.interlockSim.context.SimulationContextFactory +import cz.vutbr.fit.interlockSim.context.navigation.TopologyNavigator +import cz.vutbr.fit.interlockSim.objects.cells.DynamicRailSwitch +import cz.vutbr.fit.interlockSim.objects.core.DynamicPathSeparator +import cz.vutbr.fit.interlockSim.objects.core.OrientedPathSeparator +import cz.vutbr.fit.interlockSim.objects.core.Track +import cz.vutbr.fit.interlockSim.objects.core.TrackOccupant +import cz.vutbr.fit.interlockSim.objects.tracks.TrackSection +import cz.vutbr.fit.interlockSim.testutil.KoinTestBase +import cz.vutbr.fit.interlockSim.testutil.TestFixtures +import org.junit.jupiter.api.DisplayName +import org.junit.jupiter.api.Tag +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.Timeout +import org.koin.test.inject +import java.util.concurrent.TimeUnit.SECONDS + +/** + * Integration test that exercises [AbstractPath.setUpSemaphores] via path-level setUpPath(). + * + * ## Purpose + * + * Covers both branches of line 282 of AbstractPath.kt: + * - Null branch: `previousSwitch == null` -> ABSOLUTE_MAX_SPEED (existing test) + * - Non-null branch: `previousSwitch?.allowedSpeed()` when a switch precedes a semaphore + * + * ## Approach + * + * 1. Load XML and create a SimulationContext + * 2. Build a full path from topology results + * 3. Configure switches in the path (same as PathReservationService would) + * 4. Call path.setUpPath(startSep, trainId) which triggers setUpSemaphores() + * + * ## Topologies + * + * ### vyhybna.xml (null branch) + * InOut_B(30,8) - track - zB(27,8) - track - vB(26,8) - track - doB1(25,8) + * ... middle tracks ... + * doA1(16,8) - track - vA(15,8) - track - zA(14,8) - track - InOut_A(11,8) + * + * ### switch-between-semaphores.xml (non-null branch) + * InOut_A(5,10) - track - semA(8,10) - track - sw1(12,10) - track - semB(15,10) - track - InOut_B(18,10) + * \- siding - InOut_C(18,11) + * Both semaphores have orientation=false (direction=F, facing toward higher X). + * Backward iteration from InOut_B encounters previousTrack on the F-segment side, + * so isSeparatorInDirection returns true for both semaphores: + * semB (in-direction, prevSwitch=null -> null branch), sw1 (prevSwitch=sw1), + * semA (in-direction, prevSwitch=sw1 -> non-null branch at line 282). + * + * @since Issue #357 + */ +@DisplayName("AbstractPath.setUpSemaphores Coverage (Issue #357)") +@Tag("integration-test") +@Timeout(30, unit = SECONDS) +class AbstractPathSetUpSemaphoresTest : KoinTestBase() { + private val editingContextFactory: EditingContextFactory by inject() + private val simulationContextFactory: SimulationContextFactory by inject() + + /** + * Minimal TrackOccupant for switch configuration (same pattern as + * DefaultPathReservationService.MinimalTrackOccupant which is private). + */ + private class TestTrackOccupant( + override val name: String + ) : TrackOccupant { + override fun distanceToSemaphore(): Double = 0.0 + override fun nextSemaphore(): OrientedPathSeparator? = null + } + + @Test + @DisplayName("setUpSemaphores uses ABSOLUTE_MAX_SPEED when no preceding switch exists") + fun setUpSemaphoresUsesAbsoluteMaxSpeedWhenNoPrecedingSwitch() { + val xmlStream = TestFixtures.loadShuntingXml() + ?: throw IllegalStateException("vyhybna.xml not found in resources") + + val editingContext = editingContextFactory.createContext(xmlStream) as EditingContext + val simulationContext = + simulationContextFactory.createContext(editingContext) as DefaultSimulationContext + + try { + // Step 1: Get topology navigator + val navigator: TopologyNavigator = simulationContext.scope.get() + + // Step 2: Get InOut elements + val inOutsList = simulationContext.getInOuts().toList() + val inOut1 = simulationContext.toDynamic(inOutsList[0]) + val inOut2 = simulationContext.toDynamic(inOutsList[1]) + + // Step 3: Get topology path (track sections) for InOut1 -> InOut2 + val candidatePaths = navigator.findAllTopologicalPaths(inOut1, inOut2) + assertThat(candidatePaths.size).isGreaterThanOrEqualTo(1) + val trackSections: List = candidatePaths[0] + + // Step 4: Build an ArrayPath from topology results + // Pattern matches PathInfoBuilder.buildFullPath() + val path = ArrayPath(simulationContext) + path.add(inOut1) + var currentSeparator: DynamicPathSeparator = inOut1 + for (trackSection in trackSections) { + path.add(trackSection) + val staticResult = trackSection.getSecondEnd(currentSeparator) + currentSeparator = simulationContext.toDynamic(staticResult) + path.add(currentSeparator) + } + assertThat(path.size).isGreaterThanOrEqualTo(3) + + // Step 5: Configure switches in the path before calling setUpPath + // AbstractPath.separatorSetting checks getFollowingSegment(from) === to + // for SET_UP_PATH, so switches must be configured first. + val pathElements = path.toList() + val trainOccupant = TestTrackOccupant("test-train") + for ((index, element) in pathElements.withIndex()) { + if (element is DynamicRailSwitch) { + // Find previous and next tracks + var previous: Track? = null + for (i in (index - 1) downTo 0) { + if (pathElements[i] is Track) { + previous = pathElements[i] as Track + break + } + } + var next: Track? = null + for (i in (index + 1) until pathElements.size) { + if (pathElements[i] is Track) { + next = pathElements[i] as Track + break + } + } + if (next != null) { + val from = simulationContext.getSegment(element, previous, next) + val to = simulationContext.getSegment(element, next, previous) + element.setUpPath(from, to, element.allowedSpeed(), trainOccupant) + } + } + } + + // Step 6: Call setUpPath which triggers setUpSemaphores() + // All blocks are FREE, switches are configured. + // setUpSemaphores() iterates backward from the other end and + // finds semaphore zA with no preceding switch, using + // ABSOLUTE_MAX_SPEED (line 282 of AbstractPath.kt). + path.setUpPath(inOut1, "test-train") + + // Step 7: Verify the path was successfully set up + // isSetUpPath returns true when all tracks are reserved + val isSetUp = path.isSetUpPath(inOut1) + assertThat(isSetUp).isTrue() + } finally { + simulationContext.close() + } + } + + @Test + @DisplayName("setUpSemaphores uses switch allowedSpeed when preceding switch exists") + fun setUpSemaphoresUsesSwitchSpeedWhenPrecedingSwitchExists() { + val xmlStream = TestFixtures.loadSwitchBetweenSemaphoresXml() + + val editingContext = editingContextFactory.createContext(xmlStream) as EditingContext + val simulationContext = + simulationContextFactory.createContext(editingContext) as DefaultSimulationContext + + try { + // Step 1: Get topology navigator + val navigator: TopologyNavigator = simulationContext.scope.get() + + // Step 2: Get InOut elements - find A and B by name + val inOutsList = simulationContext.getInOuts().toList() + val inOutA = inOutsList.first { it.name == "A" } + val inOutB = inOutsList.first { it.name == "B" } + + // Step 3: Get topology path (track sections) for A -> B + val candidatePaths = navigator.findAllTopologicalPaths(inOutA, inOutB) + assertThat(candidatePaths.size).isGreaterThanOrEqualTo(1) + val trackSections: List = candidatePaths[0] + + // Step 4: Build an ArrayPath from topology results + val path = ArrayPath(simulationContext) + path.add(inOutA) + var currentSeparator: DynamicPathSeparator = inOutA + for (trackSection in trackSections) { + path.add(trackSection) + val staticResult = trackSection.getSecondEnd(currentSeparator) + currentSeparator = simulationContext.toDynamic(staticResult) + path.add(currentSeparator) + } + assertThat(path.size).isGreaterThanOrEqualTo(3) + + // Step 5: Configure switches in the path + val pathElements = path.toList() + val trainOccupant = TestTrackOccupant("test-train-switch") + for ((index, element) in pathElements.withIndex()) { + if (element is DynamicRailSwitch) { + var previous: Track? = null + for (i in (index - 1) downTo 0) { + if (pathElements[i] is Track) { + previous = pathElements[i] as Track + break + } + } + var next: Track? = null + for (i in (index + 1) until pathElements.size) { + if (pathElements[i] is Track) { + next = pathElements[i] as Track + break + } + } + if (next != null) { + val from = simulationContext.getSegment(element, previous, next) + val to = simulationContext.getSegment(element, next, previous) + element.setUpPath(from, to, element.allowedSpeed(), trainOccupant) + } + } + } + + // Step 6: Call setUpPath which triggers setUpSemaphores() + // setUpSemaphores(inOutA) iterates backward from getSecondEnd(inOutA) = inOutB. + // Both semaphores have orientation=false -> direction()=F, matching the + // F-segment where previousTrack connects during backward iteration. + // Backward iteration: InOut_B, track, semB (direction=F, prevSwitch=null), + // track, sw1 (switch -> prevSwitch=sw1), track, + // semA (direction=F, prevSwitch=sw1 -> non-null branch at line 282) + path.setUpPath(inOutA, "test-train-switch") + + // Step 7: Verify the path was successfully set up + val isSetUp = path.isSetUpPath(inOutA) + assertThat(isSetUp).isTrue() + } finally { + simulationContext.close() + } + } +} diff --git a/core/src/jvmTest/kotlin/cz/vutbr/fit/interlockSim/testutil/TestFixtures.kt b/core/src/jvmTest/kotlin/cz/vutbr/fit/interlockSim/testutil/TestFixtures.kt index 2f1d9c989..9759cb641 100644 --- a/core/src/jvmTest/kotlin/cz/vutbr/fit/interlockSim/testutil/TestFixtures.kt +++ b/core/src/jvmTest/kotlin/cz/vutbr/fit/interlockSim/testutil/TestFixtures.kt @@ -55,6 +55,8 @@ object TestFixtures { fun loadInvalidNameTooLongXml(): InputStream = loadTestFixture("invalid-name-too-long.xml") + fun loadSwitchBetweenSemaphoresXml(): InputStream = loadTestFixture("switch-between-semaphores.xml") + fun loadInvalidInOutXml(fixtureName: String): InputStream = loadTestFixture(fixtureName) private fun loadMainResource(resourceName: String): InputStream = diff --git a/core/src/jvmTest/resources/cz/vutbr/fit/interlockSim/xml/fixtures/switch-between-semaphores.xml b/core/src/jvmTest/resources/cz/vutbr/fit/interlockSim/xml/fixtures/switch-between-semaphores.xml new file mode 100644 index 000000000..ff23e886d --- /dev/null +++ b/core/src/jvmTest/resources/cz/vutbr/fit/interlockSim/xml/fixtures/switch-between-semaphores.xml @@ -0,0 +1,15 @@ + + + + + + + + + + + + + + + diff --git a/desktop-ui/src/main/kotlin/cz/vutbr/fit/interlockSim/gui/MenuBar.kt b/desktop-ui/src/main/kotlin/cz/vutbr/fit/interlockSim/gui/MenuBar.kt index 7c82488bc..70da670fd 100644 --- a/desktop-ui/src/main/kotlin/cz/vutbr/fit/interlockSim/gui/MenuBar.kt +++ b/desktop-ui/src/main/kotlin/cz/vutbr/fit/interlockSim/gui/MenuBar.kt @@ -10,7 +10,9 @@ */ package cz.vutbr.fit.interlockSim.gui +import cz.vutbr.fit.interlockSim.context.EditingContext import cz.vutbr.fit.interlockSim.context.EditingContextFactory +import cz.vutbr.fit.interlockSim.xml.XMLContextFactory import org.koin.mp.KoinPlatform.getKoin import java.awt.event.ActionEvent import java.io.File @@ -27,6 +29,16 @@ class MenuBar : JMenuBar() { private val saveAction = SaveAction() private val saveAsAction = SaveAsAction() + companion object { + /** + * Pure validation: returns true if [context] has enough InOut elements to be saved. + * Does not show any dialog — callers handle error presentation. + * Issue #80: GUI validation to prevent saving contexts with insufficient InOut elements + */ + fun validateForSave(context: EditingContext): Boolean = + context.getInOuts().size >= XMLContextFactory.MIN_INOUT_ELEMENTS + } + /** * Opens a railway network file from disk into the EDITOR. * @@ -57,8 +69,8 @@ class MenuBar : JMenuBar() { try { // Use lenient parsing to separate unparseable XML from validation errors - val editingContextFactory = getKoin().get() - val parseResult = editingContextFactory.createContextLenient(selectedFile) + val xmlContextFactory = getKoin().get() + val parseResult = xmlContextFactory.createContextLenient(selectedFile) when { // Case 1: Successfully parsed with no errors @@ -88,16 +100,16 @@ class MenuBar : JMenuBar() { frame.modificationTracker.setCurrentFile(selectedFile) frame.modificationTracker.markClean() - // Check if context has insufficient InOuts and show warning (Issue #XXX, PR #358) + // Check if context has insufficient InOuts and show warning (Issue #80) val inOutsCount = context.getInOuts().size - val minRequired = cz.vutbr.fit.interlockSim.xml.XMLContextFactory.MIN_INOUT_ELEMENTS + val minRequired = XMLContextFactory.MIN_INOUT_ELEMENTS if (inOutsCount < minRequired) { JOptionPane.showMessageDialog( this@MenuBar, "WARNING: This railway network has insufficient InOut elements " + "($inOutsCount found, $minRequired required).\n\n" + "The editor will prevent saving this context until you add at least " + - "$minRequired InOut element(s).\n\n" + + "$minRequired InOut element (entry/exit point).\n\n" + "InOut elements define entry/exit points for trains.", "Validation Warning", JOptionPane.WARNING_MESSAGE @@ -164,21 +176,14 @@ class MenuBar : JMenuBar() { * Performs the actual save operation to the specified file. * Updates modification tracker on success. * - * DEFERRED: Save-time validation (Issue #258) - * Decision: Defer to follow-up issue for comprehensive validation framework + * **Validation (Issue #80):** + * - Pre-save validation checks InOut element count via [validateForSave] + * - Shows user-friendly error dialog if validation fails + * - Prevents saving invalid contexts that cannot be reloaded * - * Rationale: - * - Current implementation: Editor allows opening/editing any file (including broken ones) - * - Save validation would be inconsistent without load validation - * - Comprehensive solution requires: - * 1. Define validation rules (structural, safety, configuration) - * 2. Implement validators for each category - * 3. Add validation on both load AND save - * 4. Support warnings (allow save) vs errors (block save) - * 5. Provide user choice dialog for warnings - * - * Current behavior: Saves all files without validation - * Future enhancement: Track in separate issue for comprehensive validation system + * **Deferred validation (Issue #258):** + * - Other validation rules deferred for comprehensive validation framework + * - Future: Track constraints, path connectivity, etc. * * @return true if save succeeded, false otherwise */ @@ -187,6 +192,22 @@ class MenuBar : JMenuBar() { val frame = getKoin().get() val editingContext = frame.railwayNetGridCanvas.getEditingContext() + // Validate InOut count before saving (Issue #80) + val inOutsCount = editingContext.getInOuts().size + val minRequired = XMLContextFactory.MIN_INOUT_ELEMENTS + if (!validateForSave(editingContext)) { + JOptionPane.showMessageDialog( + this, + "Railway network must have at least $minRequired InOut element (entry/exit point).\n\n" + + "Current count: $inOutsCount\n\n" + + "An InOut element defines where trains can enter and/or exit the railway network.\n" + + "Please add the required InOut element before saving.", + "Cannot Save - Insufficient InOut Elements", + JOptionPane.ERROR_MESSAGE + ) + return false + } + val success = editingContextFactory.saveContext(editingContext, file) if (success) { @@ -197,28 +218,14 @@ class MenuBar : JMenuBar() { // Show non-intrusive success message in status bar frame.statusBar.showTemporaryMessage("✓ Saved: ${file.name}", 5000) } else { - // Check if failure was due to validation (Issue #XXX, PR #358) - val inOutsCount = editingContext.getInOuts().size - val minRequired = cz.vutbr.fit.interlockSim.xml.XMLContextFactory.MIN_INOUT_ELEMENTS - if (inOutsCount < minRequired) { - JOptionPane.showMessageDialog( - this, - "Cannot save: Railway network must have at least $minRequired InOut element(s).\n\n" + - "Current InOut count: $inOutsCount\n\n" + - "Add InOut elements (entry/exit points) before saving.", - "Save Failed - Validation Error", - JOptionPane.ERROR_MESSAGE - ) - } else { - // General IO error - JOptionPane.showMessageDialog( - this, - "Failed to save railway network to file: ${file.absolutePath}\n\n" + - "Check file permissions and disk space.", - "Save Failed - IO Error", - JOptionPane.ERROR_MESSAGE - ) - } + // IO error — InOut validation already passed above + JOptionPane.showMessageDialog( + this, + "Failed to save railway network to file: ${file.absolutePath}\n\n" + + "Check file permissions and disk space.", + "Save Failed - IO Error", + JOptionPane.ERROR_MESSAGE + ) } return success diff --git a/desktop-ui/src/test/kotlin/cz/vutbr/fit/interlockSim/gui/InOutSaveValidationTest.kt b/desktop-ui/src/test/kotlin/cz/vutbr/fit/interlockSim/gui/InOutSaveValidationTest.kt new file mode 100644 index 000000000..26a5953c4 --- /dev/null +++ b/desktop-ui/src/test/kotlin/cz/vutbr/fit/interlockSim/gui/InOutSaveValidationTest.kt @@ -0,0 +1,110 @@ +/* + Brno University of Technology + Faculty of Information Technology + + BSc Thesis 2006/2007 + Railway Interlocking Simulator + + Unit tests for InOut validation during save operation + Issue #80: GUI validation to prevent saving contexts with insufficient InOut elements +*/ + +package cz.vutbr.fit.interlockSim.gui + +import assertk.assertThat +import assertk.assertions.isEqualTo +import assertk.assertions.isFalse +import assertk.assertions.isTrue +import cz.vutbr.fit.interlockSim.testutil.KoinTestBase +import cz.vutbr.fit.interlockSim.testutil.TestContextBuilder +import cz.vutbr.fit.interlockSim.xml.XMLContextFactory +import org.junit.jupiter.api.DisplayName +import org.junit.jupiter.api.Test + +/** + * Tests for InOut validation during save operation. + * + * **Requirements (Issue #80):** + * - Save operation must validate InOut count before saving + * - Minimum [cz.vutbr.fit.interlockSim.xml.XMLContextFactory.MIN_INOUT_ELEMENTS] InOut required + * - Save operation blocked until requirement is met + * + * **Test Coverage:** + * - Save with 0 InOuts - should fail validation + * - Save with 1 InOut - should pass (min is 1 per PR #356 bidirectional operation) + * - Save with 2 InOuts - should pass + * - Save with 3+ InOuts - should pass + * + * Tests call [MenuBar.validateForSave] directly — no dialogs, no EDT, no reflection. + */ +@DisplayName("InOut Save Validation") +class InOutSaveValidationTest : KoinTestBase() { + + @Test + @DisplayName("context with 0 InOuts cannot be saved") + fun contextWith0InOutsCannotBeSaved() { + val context = TestContextBuilder().buildEditingContext() + assertThat(MenuBar.validateForSave(context)).isFalse() + } + + @Test + @DisplayName("context with 1 InOut can be saved") + fun contextWith1InOutCanBeSaved() { + val context = TestContextBuilder() + .withInOut("OnlyEntry", 1, 1, true) + .buildEditingContext() + assertThat(MenuBar.validateForSave(context)).isTrue() + } + + @Test + @DisplayName("context with 2 InOuts can be saved") + fun contextWith2InOutsCanBeSaved() { + val context = TestContextBuilder() + .withInOut("Entry", 1, 1, true) + .withInOut("Exit", 10, 10, false) + .buildEditingContext() + assertThat(MenuBar.validateForSave(context)).isTrue() + } + + @Test + @DisplayName("context with 3 InOuts can be saved") + fun contextWith3InOutsCanBeSaved() { + val context = TestContextBuilder() + .withInOut("Entry1", 1, 1, true) + .withInOut("Exit1", 10, 10, false) + .withInOut("Entry2", 5, 5, true) + .buildEditingContext() + assertThat(MenuBar.validateForSave(context)).isTrue() + } + + @Test + @DisplayName("MIN_INOUT_ELEMENTS constant value is 1") + fun minInOutElementsConstantIsOne() { + assertThat(XMLContextFactory.MIN_INOUT_ELEMENTS).isEqualTo(1) + } + + @Test + @DisplayName("validateForSave is idempotent — repeated calls return same result") + fun validateForSave_isIdempotent() { + val context = TestContextBuilder() + .withInOut("OnlyEntry", 1, 1, true) + .buildEditingContext() + val first = MenuBar.validateForSave(context) + val second = MenuBar.validateForSave(context) + val third = MenuBar.validateForSave(context) + assertThat(first).isTrue() + assertThat(second).isTrue() + assertThat(third).isTrue() + } + + @Test + @DisplayName("context with 10 InOuts can be saved") + fun contextWith10InOutsCanBeSaved() { + var builder = TestContextBuilder() + for (i in 1..10) { + builder = builder.withInOut("InOut$i", i, i, true) + } + val context = builder.buildEditingContext() + assertThat(MenuBar.validateForSave(context)).isTrue() + } +} diff --git a/desktop-ui/src/test/kotlin/cz/vutbr/fit/interlockSim/xml/XMLContextFactoryOutputStreamTest.kt b/desktop-ui/src/test/kotlin/cz/vutbr/fit/interlockSim/xml/XMLContextFactoryOutputStreamTest.kt index 4cc2fca5a..147be5446 100644 --- a/desktop-ui/src/test/kotlin/cz/vutbr/fit/interlockSim/xml/XMLContextFactoryOutputStreamTest.kt +++ b/desktop-ui/src/test/kotlin/cz/vutbr/fit/interlockSim/xml/XMLContextFactoryOutputStreamTest.kt @@ -538,7 +538,7 @@ class XMLContextFactoryOutputStreamTest : KoinTestBase() { } @Nested - @DisplayName("Pre-Save Validation (Issue #XXX, PR #358)") + @DisplayName("Pre-Save Validation (Issue #80, PR #357)") inner class PreSaveValidationTests { @Test @DisplayName("saveContext to File rejects context with 0 InOuts") diff --git a/docs/issues/README.md b/docs/issues/README.md new file mode 100644 index 000000000..e15469ee6 --- /dev/null +++ b/docs/issues/README.md @@ -0,0 +1,26 @@ +# Issue Documentation + +This directory contains detailed documentation for resolved issues in the Railway Interlocking Simulator project. + +## Issue #80: GUI InOut Validation + +**Status:** ✅ Implemented (2026-02-06) + +**File:** [`issue_80.md`](issue_80.md) + +**Summary:** +Implemented GUI validation to prevent saving railway networks with 0 InOut elements. Pre-save check in `MenuBar.performSave()` calls the pure `MenuBar.validateForSave(context)` method and shows a user-friendly error dialog if validation fails. + +**Related:** +- Issue #258: Future comprehensive validation framework + +--- + +## Other Issues + +- [`issue_291_investigation_report.md`](issue_291_investigation_report.md) — Train passivation fix +- [`issue_311_round_robin_load_balancing.md`](issue_311_round_robin_load_balancing.md) — Round-robin load balancing + +--- + +**Last Updated:** 2026-03-13 diff --git a/docs/issues/issue_80.md b/docs/issues/issue_80.md new file mode 100644 index 000000000..451d57dc1 --- /dev/null +++ b/docs/issues/issue_80.md @@ -0,0 +1,71 @@ +# Issue #80: GUI InOut Validation + +**Status:** Implemented (2026-02-06) +**Branch:** `copilot/add-gui-validation-in-editor` + +## Problem + +Users could save railway networks with 0 InOut elements through the GUI editor. The file was written successfully, but could not be reloaded (XML validation in XMLContextFactory requires >= 1 InOut since PR #356 bidirectional operation). Result: confusing "saved but can't be loaded" experience. + +## Implementation + +### Validation method — `MenuBar.validateForSave(context)` + +A pure companion-object function added to `MenuBar`: + +```kotlin +fun validateForSave(context: EditingContext): Boolean = + context.getInOuts().size >= XMLContextFactory.MIN_INOUT_ELEMENTS +``` + +### Pre-save check — `MenuBar.performSave()` + +Called before every save attempt. On failure, shows an error dialog and returns `false`: + +``` +Cannot Save - Insufficient InOut Elements +Railway network must have at least 1 InOut entry/exit point. +Current count: 0 +InOut entry/exit points define where trains enter and exit the railway network. +Please add at least one InOut entry/exit point before saving. +``` + +All three save paths go through `performSave()`: +1. File > Save → `SaveAction` +2. File > Save As... → `SaveAsAction` +3. Window close → `triggerSave()` + +### Constant alignment + +`MenuBar` uses `XMLContextFactory.MIN_INOUT_ELEMENTS` (= 1) — no separate constant. This avoids drift between the GUI and XML validation layers. + +## Validation flow + +``` +User: Save + -> MenuBar.performSave(file) + -> validateForSave(context) + -> false: show error dialog, return false (save blocked) + -> true: editingContextFactory.saveContext(...) + -> success: update tracker, show status bar message + -> failure: show IO error dialog +``` + +## Test coverage + +`InOutSaveValidationTest` (4 unit tests, no EDT, no reflection): + +| InOut count | Expected | Test | +|-------------|----------|------------------------------| +| 0 | false | `contextWith0InOutsCannotBeSaved` | +| 1 | true | `contextWith1InOutCanBeSaved` | +| 2 | true | `contextWith2InOutsCanBeSaved` | +| 3+ | true | `contextWith3InOutsCanBeSaved` | + +Tests extend `KoinTestBase` and call `MenuBar.validateForSave(context)` directly. + +## Related + +- PR #356 — bidirectional train operation (allows single-InOut networks) +- XMLContextFactory — enforces same constraint at XML load time +- Issue #258 — future comprehensive validation framework