Skip to content

Commit 39cfec0

Browse files
bedaHovorkaclaude
bedaHovorka
and
claude
committed
Address PR #357 review feedback
- Remove MenuBar private MIN_INOUT_ELEMENTS = 2; extract public validateForSave(context) companion method that reads from XMLContextFactory.MIN_INOUT_ELEMENTS (= 1, per PR #356) - Rewrite InOutSaveValidationTest: KoinTestBase, direct validateForSave() calls, no EDT/reflection/integration-tag; rename 1InOutCannotBeSaved -> 1InOutCanBeSaved; remove unused contextFactory/testFile/dead test - Remove conflicting COMMON_BRANCH_SPEED/COMMON_MAIN_SPEED imports from RailSwitch.kt; unqualified names now resolve to file-level deprecated constants as intended - Add EditingContext + XMLContextFactory imports to MenuBar; replace FQNs with short names throughout - Remove dead inOutsCount < minRequired branch from performSave failure path (unreachable after validateForSave guard) - Compact docs: merge 4 issue-80-*.md files into docs/issues/issue-80.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent d710b35 commit 39cfec0

9 files changed

Lines changed: 121 additions & 1128 deletions

File tree

‎core/src/commonMain/kotlin/cz/vutbr/fit/interlockSim/objects/cells/RailSwitch.kt‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@
99
*/
1010
package cz.vutbr.fit.interlockSim.objects.cells
1111

12-
import cz.vutbr.fit.interlockSim.domain.COMMON_BRANCH_SPEED
13-
import cz.vutbr.fit.interlockSim.domain.COMMON_MAIN_SPEED
1412
import cz.vutbr.fit.interlockSim.exceptions.requireSimulation
1513
import cz.vutbr.fit.interlockSim.objects.cells.RailSwitch.Type
1614
import cz.vutbr.fit.interlockSim.objects.core.Cell

‎desktop-ui/src/main/kotlin/cz/vutbr/fit/interlockSim/gui/MenuBar.kt‎

Lines changed: 20 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
1010
*/
1111
package cz.vutbr.fit.interlockSim.gui
1212

13+
import cz.vutbr.fit.interlockSim.context.EditingContext
1314
import cz.vutbr.fit.interlockSim.context.EditingContextFactory
15+
import cz.vutbr.fit.interlockSim.xml.XMLContextFactory
1416
import org.koin.mp.KoinPlatform.getKoin
1517
import java.awt.event.ActionEvent
1618
import java.io.File
@@ -29,10 +31,12 @@ class MenuBar : JMenuBar() {
2931

3032
companion object {
3133
/**
32-
* Minimum number of InOut elements required for a valid railway network.
34+
* Pure validation: returns true if [context] has enough InOut elements to be saved.
35+
* Does not show any dialog — callers handle error presentation.
3336
* Issue #80: GUI validation to prevent saving contexts with insufficient InOut elements
3437
*/
35-
private const val MIN_INOUT_ELEMENTS = 2
38+
fun validateForSave(context: EditingContext): Boolean =
39+
context.getInOuts().size >= XMLContextFactory.MIN_INOUT_ELEMENTS
3640
}
3741

3842
/**
@@ -65,7 +69,7 @@ class MenuBar : JMenuBar() {
6569

6670
try {
6771
// Use lenient parsing to separate unparseable XML from validation errors
68-
val editingContextFactory = getKoin().get<cz.vutbr.fit.interlockSim.xml.XMLContextFactory>()
72+
val editingContextFactory = getKoin().get<XMLContextFactory>()
6973
val parseResult = editingContextFactory.createContextLenient(selectedFile)
7074

7175
when {
@@ -98,7 +102,7 @@ class MenuBar : JMenuBar() {
98102

99103
// Check if context has insufficient InOuts and show warning (Issue #XXX, PR #358)
100104
val inOutsCount = context.getInOuts().size
101-
val minRequired = cz.vutbr.fit.interlockSim.xml.XMLContextFactory.MIN_INOUT_ELEMENTS
105+
val minRequired = XMLContextFactory.MIN_INOUT_ELEMENTS
102106
if (inOutsCount < minRequired) {
103107
JOptionPane.showMessageDialog(
104108
this@MenuBar,
@@ -173,7 +177,7 @@ class MenuBar : JMenuBar() {
173177
* Updates modification tracker on success.
174178
*
175179
* **Validation (Issue #80):**
176-
* - Pre-save validation checks InOut element count (minimum 2 required)
180+
* - Pre-save validation checks InOut element count via [validateForSave]
177181
* - Shows user-friendly error dialog if validation fails
178182
* - Prevents saving invalid contexts that cannot be reloaded
179183
*
@@ -190,10 +194,11 @@ class MenuBar : JMenuBar() {
190194

191195
// Validate InOut count before saving (Issue #80)
192196
val inOutCount = editingContext.getInOuts().size
193-
if (inOutCount < MIN_INOUT_ELEMENTS) {
197+
val minRequired = XMLContextFactory.MIN_INOUT_ELEMENTS
198+
if (!validateForSave(editingContext)) {
194199
JOptionPane.showMessageDialog(
195200
this,
196-
"Railway network must have at least $MIN_INOUT_ELEMENTS InOut elements (entry and exit points).\n\n" +
201+
"Railway network must have at least $minRequired InOut elements (entry and exit points).\n\n" +
197202
"Current count: $inOutCount\n\n" +
198203
"InOut elements define where trains enter and exit the railway network.\n" +
199204
"Please add more InOut elements before saving.",
@@ -213,28 +218,14 @@ class MenuBar : JMenuBar() {
213218
// Show non-intrusive success message in status bar
214219
frame.statusBar.showTemporaryMessage("✓ Saved: ${file.name}", 5000)
215220
} else {
216-
// Check if failure was due to validation (Issue #XXX, PR #358)
217-
val inOutsCount = editingContext.getInOuts().size
218-
val minRequired = cz.vutbr.fit.interlockSim.xml.XMLContextFactory.MIN_INOUT_ELEMENTS
219-
if (inOutsCount < minRequired) {
220-
JOptionPane.showMessageDialog(
221-
this,
222-
"Cannot save: Railway network must have at least $minRequired InOut element(s).\n\n" +
223-
"Current InOut count: $inOutsCount\n\n" +
224-
"Add InOut elements (entry/exit points) before saving.",
225-
"Save Failed - Validation Error",
226-
JOptionPane.ERROR_MESSAGE
227-
)
228-
} else {
229-
// General IO error
230-
JOptionPane.showMessageDialog(
231-
this,
232-
"Failed to save railway network to file: ${file.absolutePath}\n\n" +
233-
"Check file permissions and disk space.",
234-
"Save Failed - IO Error",
235-
JOptionPane.ERROR_MESSAGE
236-
)
237-
}
221+
// IO error — InOut validation already passed above
222+
JOptionPane.showMessageDialog(
223+
this,
224+
"Failed to save railway network to file: ${file.absolutePath}\n\n" +
225+
"Check file permissions and disk space.",
226+
"Save Failed - IO Error",
227+
JOptionPane.ERROR_MESSAGE
228+
)
238229
}
239230

240231
return success

‎desktop-ui/src/test/kotlin/cz/vutbr/fit/interlockSim/gui/InOutSaveValidationTest.kt‎

Lines changed: 26 additions & 163 deletions
Original file line numberDiff line numberDiff line change
@@ -12,203 +12,66 @@
1212
package cz.vutbr.fit.interlockSim.gui
1313

1414
import assertk.assertThat
15-
import assertk.assertions.isEqualTo
1615
import assertk.assertions.isFalse
1716
import assertk.assertions.isTrue
18-
import cz.vutbr.fit.interlockSim.context.EditingContextFactory
17+
import cz.vutbr.fit.interlockSim.testutil.KoinTestBase
1918
import cz.vutbr.fit.interlockSim.testutil.TestContextBuilder
20-
import org.junit.jupiter.api.BeforeEach
2119
import org.junit.jupiter.api.DisplayName
2220
import org.junit.jupiter.api.Test
23-
import org.junit.jupiter.api.Timeout
24-
import org.junit.jupiter.api.io.TempDir
25-
import org.koin.java.KoinJavaComponent.getKoin
26-
import java.io.File
27-
import java.nio.file.Path
28-
import java.util.concurrent.TimeUnit
2921

3022
/**
3123
* Tests for InOut validation during save operation.
3224
*
3325
* **Requirements (Issue #80):**
3426
* - Save operation must validate InOut count before saving
35-
* - Minimum 2 InOut elements required
36-
* - User-friendly error dialog shown if validation fails
27+
* - Minimum [cz.vutbr.fit.interlockSim.xml.XMLContextFactory.MIN_INOUT_ELEMENTS] InOut required
3728
* - Save operation blocked until requirement is met
3829
*
3930
* **Test Coverage:**
4031
* - Save with 0 InOuts - should fail validation
41-
* - Save with 1 InOut - should fail validation
42-
* - Save with 2 InOuts - should succeed
43-
* - Save with 3+ InOuts - should succeed
32+
* - Save with 1 InOut - should pass (min is 1 per PR #356 bidirectional operation)
33+
* - Save with 2 InOuts - should pass
34+
* - Save with 3+ InOuts - should pass
4435
*
45-
* Note: These tests verify the validation logic. GUI dialog appearance
46-
* is not tested as it requires UI automation.
36+
* Tests call [MenuBar.validateForSave] directly — no dialogs, no EDT, no reflection.
4737
*/
4838
@DisplayName("InOut Save Validation")
49-
class InOutSaveValidationTest : AbstractFrameTestBase() {
50-
@TempDir
51-
lateinit var tempDir: Path
52-
53-
private lateinit var frame: Frame
54-
private lateinit var contextFactory: EditingContextFactory
55-
56-
@BeforeEach
57-
override fun setUp() {
58-
super.setUp()
59-
60-
runOnEDT {
61-
frame = Frame()
62-
frames.add(frame)
63-
contextFactory = getKoin().get()
64-
}
65-
}
39+
class InOutSaveValidationTest : KoinTestBase() {
6640

6741
@Test
68-
@Timeout(value = 10, unit = TimeUnit.SECONDS)
6942
@DisplayName("context with 0 InOuts cannot be saved")
7043
fun contextWith0InOutsCannotBeSaved() {
71-
val testFile = tempDir.resolve("test-0-inouts.xml").toFile()
72-
73-
runOnEDT {
74-
// Create empty context (0 InOuts)
75-
val builder = TestContextBuilder()
76-
val editingContext = builder.buildEditingContext()
77-
78-
// Set context in frame
79-
frame.setContext(editingContext)
80-
81-
// Attempt to save - should fail validation
82-
val menuBar = frame.jMenuBar as MenuBar
83-
84-
// Use reflection to access performSave method
85-
val performSaveMethod = MenuBar::class.java.getDeclaredMethod("performSave", File::class.java)
86-
performSaveMethod.isAccessible = true
87-
val result = performSaveMethod.invoke(menuBar, testFile) as Boolean
88-
89-
// Verify save was blocked
90-
assertThat(result).isFalse()
91-
assertThat(testFile.exists()).isFalse()
92-
93-
editingContext.close()
94-
}
44+
val context = TestContextBuilder().buildEditingContext()
45+
assertThat(MenuBar.validateForSave(context)).isFalse()
9546
}
9647

9748
@Test
98-
@Timeout(value = 10, unit = TimeUnit.SECONDS)
99-
@DisplayName("context with 1 InOut cannot be saved")
100-
fun contextWith1InOutCannotBeSaved() {
101-
val testFile = tempDir.resolve("test-1-inout.xml").toFile()
102-
103-
runOnEDT {
104-
// Create context with 1 InOut
105-
val builder = TestContextBuilder()
106-
.withInOut("OnlyEntry", 1, 1, true)
107-
val editingContext = builder.buildEditingContext()
108-
109-
// Set context in frame
110-
frame.setContext(editingContext)
111-
112-
// Attempt to save - should fail validation
113-
val menuBar = frame.jMenuBar as MenuBar
114-
115-
val performSaveMethod = MenuBar::class.java.getDeclaredMethod("performSave", File::class.java)
116-
performSaveMethod.isAccessible = true
117-
val result = performSaveMethod.invoke(menuBar, testFile) as Boolean
118-
119-
// Verify save was blocked
120-
assertThat(result).isFalse()
121-
assertThat(testFile.exists()).isFalse()
122-
123-
editingContext.close()
124-
}
49+
@DisplayName("context with 1 InOut can be saved")
50+
fun contextWith1InOutCanBeSaved() {
51+
val context = TestContextBuilder()
52+
.withInOut("OnlyEntry", 1, 1, true)
53+
.buildEditingContext()
54+
assertThat(MenuBar.validateForSave(context)).isTrue()
12555
}
12656

12757
@Test
128-
@Timeout(value = 10, unit = TimeUnit.SECONDS)
12958
@DisplayName("context with 2 InOuts can be saved")
13059
fun contextWith2InOutsCanBeSaved() {
131-
val testFile = tempDir.resolve("test-2-inouts.xml").toFile()
132-
133-
runOnEDT {
134-
// Create context with 2 InOuts
135-
val builder = TestContextBuilder()
136-
.withInOut("Entry", 1, 1, true)
137-
.withInOut("Exit", 10, 10, false)
138-
.withConnection(1, 1, 10, 10, 100.0, 80.0)
139-
val editingContext = builder.buildEditingContext()
140-
141-
// Set context in frame
142-
frame.setContext(editingContext)
143-
144-
// Attempt to save - should succeed
145-
val menuBar = frame.jMenuBar as MenuBar
146-
147-
val performSaveMethod = MenuBar::class.java.getDeclaredMethod("performSave", File::class.java)
148-
performSaveMethod.isAccessible = true
149-
val result = performSaveMethod.invoke(menuBar, testFile) as Boolean
150-
151-
// Verify save succeeded
152-
assertThat(result).isTrue()
153-
assertThat(testFile.exists()).isTrue()
154-
155-
editingContext.close()
156-
}
60+
val context = TestContextBuilder()
61+
.withInOut("Entry", 1, 1, true)
62+
.withInOut("Exit", 10, 10, false)
63+
.buildEditingContext()
64+
assertThat(MenuBar.validateForSave(context)).isTrue()
15765
}
15866

15967
@Test
160-
@Timeout(value = 10, unit = TimeUnit.SECONDS)
16168
@DisplayName("context with 3 InOuts can be saved")
16269
fun contextWith3InOutsCanBeSaved() {
163-
val testFile = tempDir.resolve("test-3-inouts.xml").toFile()
164-
165-
runOnEDT {
166-
// Create context with 3 InOuts
167-
val builder = TestContextBuilder()
168-
.withInOut("Entry1", 1, 1, true)
169-
.withInOut("Exit1", 10, 10, false)
170-
.withInOut("Entry2", 5, 5, true)
171-
.withConnection(1, 1, 10, 10, 100.0, 80.0)
172-
.withConnection(5, 5, 10, 10, 50.0, 80.0)
173-
val editingContext = builder.buildEditingContext()
174-
175-
// Set context in frame
176-
frame.setContext(editingContext)
177-
178-
// Attempt to save - should succeed
179-
val menuBar = frame.jMenuBar as MenuBar
180-
181-
val performSaveMethod = MenuBar::class.java.getDeclaredMethod("performSave", File::class.java)
182-
performSaveMethod.isAccessible = true
183-
val result = performSaveMethod.invoke(menuBar, testFile) as Boolean
184-
185-
// Verify save succeeded
186-
assertThat(result).isTrue()
187-
assertThat(testFile.exists()).isTrue()
188-
189-
editingContext.close()
190-
}
191-
}
192-
193-
@Test
194-
@Timeout(value = 10, unit = TimeUnit.SECONDS)
195-
@DisplayName("validation message includes current InOut count")
196-
fun validationMessageIncludesCurrentInOutCount() {
197-
val testFile = tempDir.resolve("test-validation-message.xml").toFile()
198-
199-
runOnEDT {
200-
// Create context with 1 InOut
201-
val builder = TestContextBuilder()
202-
.withInOut("OnlyEntry", 1, 1, true)
203-
val editingContext = builder.buildEditingContext()
204-
205-
// Set context in frame
206-
frame.setContext(editingContext)
207-
208-
// Verify InOut count in context
209-
assertThat(editingContext.getInOuts().size).isEqualTo(1)
210-
211-
editingContext.close()
212-
}
70+
val context = TestContextBuilder()
71+
.withInOut("Entry1", 1, 1, true)
72+
.withInOut("Exit1", 10, 10, false)
73+
.withInOut("Entry2", 5, 5, true)
74+
.buildEditingContext()
75+
assertThat(MenuBar.validateForSave(context)).isTrue()
21376
}
21477
}

0 commit comments

Comments
 (0)