Baselines dialog rewritten from Swing to JavaFX

[dbarashev]

No description provided.

[claude]

Claude finished @dbarashev's task in 1m 34s —— View job

---

PR Review: Baselines dialog rewritten from Swing to JavaFX

• Read the diff and the surrounding code (SharedUiComponents.kt, Dialog.kt)
• Compare behaviour with the old Swing EditableList version
• Post the review

Overall this is a clean port. It reuses the ItemListDialogPane / ItemEditorPaneImpl pattern from the column and filter dialogs, and it switches Swing chart updates onto the EDT with invokeLater. The single-shown-baseline rule in wireItem handles event ordering correctly. When showing B hides A, A's "set null" runnable is queued before B's "set B" runnable, so B ends up in the chart. The only cost is two reset() calls. setupButton(CancelAction) is fine here because dialog() uses DialogControllerFx, which implements it. The other two controllers leave it as TODO().

A few things are worth a look, most important first:

1. The editor still points at a deleted item and can bring its baseline back into the chart (probable bug)

ItemListDialogPane only updates selectedItem when newValue != null (SharedUiComponents.kt:297). If you delete the last baseline, and possibly any selected one, depending on how ListView moves the selection, editItem still holds the removed BaselineItem. Its wireItem listener is still attached. If you then flip the "visible" toggle in the editor, saveData runs item.isEnabledProperty().set(true), and the listener calls setBaseline(removedBaseline). That baseline is no longer in the list, and Apply won't save it. Two possible fixes:

• in saveData, return early when !listItems.contains(item), or
• in the removal branch of the ListChangeListener, clear selectedItem when it holds a removed item. You'd also need loadData(null) to reset the name field and the toggle.

Fix this →

2. Project and task model is read on the FX thread

The whole contentBuilder runs inside Platform.runLater. That means these calls now happen on the FX thread instead of the EDT:

• myProject.getBaselines()
• myUiFacade.getGanttChart().getBaseline()
• GanttPreviousState.createTasks(myProject.getTaskManager()), in the Add factory

createTasks walks the task tree to take the snapshot, so it could race with task edits made on the EDT. The risk is low while the dialog is open, but it breaks the "Swing model only on the EDT" rule the rest of the PR follows. You could take the initial snapshot of baselines and the current baseline on the EDT before calling dialog(...). For Add, you could build the snapshot with invokeAndWait, or defer createTasks to Apply.

3. Cancel doesn't undo renames or chart changes, and a rename that is cancelled never marks the project modified (low, pre-existing)

BaselineItem.setTitle writes directly to GanttPreviousState.setName. If you rename and then press Cancel, the new name stays in the model, but setModified() is never called, so it may or may not be saved later. The chart's shown baseline also stays however it was toggled. The old EditableList.updateValue behaved the same way, so this isn't a regression. Now that you have a BaselineItem wrapper, it would be cheap to hold the pending name in the item and apply it to GanttPreviousState only in the Apply handler.

4. Dialog sizing with the async SwingNode (minor)

The color options are put into the SwingNode via SwingUtilities.invokeLater, while dlg.resize() runs in onShown. If the Swing content arrives after onShown, the dialog may be sized without the color panel, which then gets clipped or leaves blank space. You could call Platform.runLater(dlg::resize) after setContent, or build the Swing component before opening the dialog.

5. Smaller notes

• Leftover i18n keys: baseline.dialog.hide and baseline.dialog.undefinedValueLabel are no longer used by this action. The submodule bump should cover the new keys under the baseline.dialog root: title, add, delete, apply, addItem, visibility.label, and the name property label. I couldn't check this because the localization submodule isn't checked out in CI.
• Empty names: ObservableString("name", …, getVoidValidator(), …) accepts empty names, which then show as a blank row (" "). A non-empty validator would disable Apply through the existing errorPane binding.
• Visibility changes in SharedUiComponents.kt: Dropping internal is needed for Java callers, since internal members get mangled names. As a result these classes and their members are now public API. ItemListDialogModel.selection staying internal looks intentional. Typing contentNode as Pane is fine.
• Listener buildup (existing shared code): The editItem watcher in ItemEditorPaneImpl adds a new isEnabledProperty listener every time the selection changes and never removes the old ones. It isn't introduced here, but this dialog now depends on that code path.

No build or tests were run; this review is based on reading the code only.

[github-actions]

Test Results

398 tests  +398   396 ✅ +396   19s ⏱️ +19s
105 suites +105     2 💤 +  2 
105 files   +105     0 ❌ ±  0 

Results for commit b014826. ± Comparison against base commit 4e1e554.

♻️ This comment has been updated with latest results.

[claude]

Claude finished @dbarashev's task in 1m 32s —— View job

---

PR Review: Baselines dialog rewritten from Swing to JavaFX

• Read the diff for the new commit (5b45981 Color choosers ported to JavaFX)
• Check the surrounding code (PropertyPaneBuilderImpl.createColorOptionEditor, ObservableColor, GPAbstractOption, ChartOptionGroup, the old OptionsPageBuilder.createColorComponent)
• Post the review

This review covers only the new commit. The first commit was reviewed in the earlier comment, and those points still apply. I only read the code; I didn't build it or run any tests.

The new commit fixes point 4 from the earlier review. The SwingNode is gone, so the dialog no longer resizes before the async Swing content arrives. Removing setSwingBackground and resize() from onShown makes sense. Adding color() to the PropertyPaneBuilder interface also helps other screens: any DefaultColorOption shown through the FX property panes (export options, the import wizard) now gets a real editor instead of the --- MISSING: id --- placeholder.

The two-way sync in DefaultColorOption.visitPropertyPaneBuilder doesn't loop, but it only works because of how equality is checked:

• A change in the picker calls ObservableColor.set(..., picker), which calls resetValue(..., picker), which fires the option listener.
• The listener compares java.awt.Color values, and those are equal, so it stops there.
• This matters because Style.Color has no equals(). ObservableImpl.set compares by reference, so the observable always fires. Comparing the unwrapped get() values is what breaks the cycle. It would help to add a comment saying so.

1. Every opening of the dialog leaks a listener on the long-lived chart options (low–medium)

DefaultColorOption.kt:53: addChangeValueListener { ... observableColor.set(...) } returns a Runnable that removes the listener, and nobody calls it. The baseline color options belong to GanttGraphicArea and live as long as the app. Each time the dialog opens, three more listeners are added. Through the watcher in createColorOptionEditor, each one keeps an ObservableColor and a ColorPicker (and its scene graph) alive.

What goes wrong: open and close the dialog N times. After that, every change to these options, such as loading a project (loadPersistentValue) or changing a color, updates N stale pickers. These updates run on whatever thread changed the option, often the EDT.

The old Swing createColorComponent leaked a label listener the same way, so the pattern isn't new. But the leaked objects are now heavier (FX nodes), and they get touched from off the FX thread. Possible fixes:

• visitPropertyPaneBuilder could hand the unsubscribe Runnable to the builder or pane, which runs it when the node is disposed or the dialog closes, or
• the dialog could remove the listeners in its close handler.

Fix this →

2. Threading: option listeners now run on the FX thread; FX nodes can be updated from the EDT (low)

• Picking a color calls resetValue on the FX thread. The existing listeners in GanttGraphicArea.createBaselineColorOptions then write to UIConfiguration, which the chart reads while painting on the EDT. These are plain field writes, so the practical risk is small. Still, it goes against the "Swing model on the EDT" rule the first commit follows for setBaseline. You could wrap the resetValue call in SwingUtilities.invokeLater. A Platform.runLater wrapper for the reverse direction would need to live in core, which may not be wanted.
• The reverse direction has the opposite problem: the option → observable listener calls picker.value = ... on whichever thread changed the option. While the dialog is open, that can be the EDT.

3. The chart still doesn't repaint after a color change (pre-existing, worth confirming)

ChartOptionGroup only calls optionsChanged() (which triggers a repaint) from commit(). Neither the old dialog nor this one calls lock() or commit() on getBaselineColorOptions(), so a new color only appears after some other repaint. The behaviour is the same as before, but the FX ColorPicker now applies every pick immediately, so users may expect to see the change right away. You could call colorOptions.commit() in the Apply handler (on the EDT), or when a color changes. If you also want Cancel to revert, call lock() when the dialog opens and rollback() on Cancel.

4. Smaller notes

• Possible NPE if an option becomes null: if the option value is set to null, the watcher sets picker.value = null. The picker's subscribe handler then calls ColorOption.Util.getColor(null), which probably throws on a non-null Kotlin parameter. The defaults are non-null, so this is unlikely, but a newValue ?: return guard in createColorOptionEditor costs nothing. Also, when the option is null, the picker shows WHITE while the observable holds null.
• Every pick counts as a recent color: GPColorChooser.addRecentColor now runs on every value change in the picker. The old code only added a color when the user pressed OK in the chooser. This is probably fine, but it could fill the recent-colors list faster.
• i18n keys: the new ourOptionLocalizer (root option) turns into option.<id>.label in the row builder, which matches OptionsPageBuilder.I18N.getCanonicalOptionLabelKey, so the existing translations should resolve. The group title uses optionGroup.ganttChartStateDiffColors.label through the root localizer, which is also fine.
• Imports: SwingUtilities is still used by the chart-update invokeLater calls, so no import has become unused.

[claude]

Claude finished @dbarashev's task in 1m 7s —— View job

---

PR Review: Baselines dialog rewritten from Swing to JavaFX

• Read the diff for the new commit (f683fd2 Dialog code ported to Kotlin)
• Check how the SharedUiComponents null-selection change affects the other dialogs (ColumnManager, FilterDialog, SettingsDialog)
• Post the review

This review covers only the new commit. I only read the code; I didn't build it or run any tests.

The port to BaselineDialog.kt mostly carries the logic over line for line, and the result is shorter and easier to read. It also fixes two points from the first review:

• Point 1 (the editor pointing at a deleted item): ItemListDialogPane now passes a null selection through to the editor. onEdit() only saves when editItem.value is non-null, so a deleted baseline can no longer be shown again through the toggle. loadData(null) also disables the controls.
• Point 2 (threading), partly: BaselineDialog is now built in actionPerformed, on the EDT. So myProject.baselines and getGanttChart().getBaseline() are read on the EDT. The Add factory (GanttPreviousState.createTasks(...)) still runs on the FX thread.

1. The Cancel button is gone (regression, or an intentional choice?)

The Java version called dlg.setupButton(new CancelAction(), btn -> Unit.INSTANCE) after dialogPane.build(dlg). BaselineDialog.show() (BaselineDialog.kt:125-128) has no equivalent, and ItemListDialogPane.build only adds Add, Delete and Apply. The dialog can still be closed with Esc or the window close button. But the only visible button is Apply, and Apply also overwrites myProject.baselines and marks the project modified. The column and filter dialogs don't have a Cancel button either, so this might be on purpose for consistency. If it isn't, add this after dialogPane.build(dlg):

dlg.setupButton(CancelAction()) {}

Fix this →

2. The SharedUiComponents change affects every list/editor dialog (low)

Removing the if (newValue != null) guard at SharedUiComponents.kt:297 changes behaviour for all users of ItemListDialogPane:

• FilterDialog: already has a null branch in loadData, which clears the fields and disables Delete. Fine.
• SettingsDialog: OptionPageUi uses event.newValue?.let, so it ignores null. Fine.
• ColumnManager: CustomPropertyEditor.loadData(null) does nothing. When the selection is cleared, for example after deleting a custom column, the editor keeps showing the deleted column's values. It also stays editable, and Delete stays enabled. Edits are dropped because editItem.value is null, and Delete does nothing with an empty selection, so no data is corrupted. But the UI now shows stale values in an editable pane. Before this change the pane stayed attached to the deleted item, which was arguably worse, so this is a small net improvement. An else branch like the one in FilterDialog would make it consistent.

3. The disabled editor still shows the deleted baseline's name (nit)

BaselineItemEditor.loadData(null) disables the controls but leaves myNameOption and visibilityToggle showing the removed item's values. Resetting them inside isEditIgnored is safe, because loadData is already called with isEditIgnored = true:

} else {
  myNameOption.value = ""
  visibilityToggle.isSelected = false
}

4. Smaller notes

• Nullable listener parameter: ListChangeListener { change: ListChangeListener.Change<out BaselineItem>? -> while (change!!.next()) is left over from the Java-to-Kotlin conversion. ListChangeListener { change -> while (change.next()) ... } avoids the !!.
• Needless Runnable wrappers: the explicit Runnable { ... } in SwingUtilities.invokeLater(Runnable { ... }) and the trailing Unit in the lambdas aren't needed in Kotlin.
• Double blank lines: there are double blank lines at lines 62, 104 and 124.
• DefaultColorOption leak: the new TODO(dbarashev): fix this listener leak records point 1 of the previous review. That's fine as a follow-up.
• Still open from the earlier reviews: Cancel doesn't undo renames or chart toggles. createTasks runs on the FX thread. Colour changes don't trigger a chart repaint (ChartOptionGroup.commit()). Empty baseline names are still accepted (voidValidator).

[claude]

Claude finished @dbarashev's task in 28s —— View job

---

PR Review: Baselines dialog rewritten from Swing to JavaFX

• Read the diff for the new commit (b014826 Code review round)
• Check it against the points from the earlier reviews
• Post the review

This review covers only the new commit. I only read the code; I didn't build it or run any tests.

The commit looks good and I found no new problems. It resolves these points from the previous reviews:

• Editor showing the deleted baseline's name: BaselineItemEditor.loadData(null) now resets the name to "" and turns the visibility toggle off. This is safe because the editItem watcher sets isEditIgnored = true around loadData (SharedUiComponents.kt:174-181), so onEdit() returns early and doesn't write back to any item.
• Colour changes not repainting the chart: each baseline colour option listener in GanttGraphicArea.createBaselineColorOptions now calls SwingUtilities.invokeLater(this::repaint). This is simpler than going through ChartOptionGroup.commit(), and it also gives the "write on the FX thread, repaint on the EDT" split. The method had to stop being static to use this. It's called from the constructor (GanttGraphicArea.java:92), but the lambdas only run later, so the early this reference is harmless. Component.repaint() is thread-safe anyway, so the invokeLater is just extra safety. Loading a project will trigger a few extra repaints through loadPersistentValue, which doesn't matter.
• Kotlin cleanups: the ListChangeListener lambda no longer uses !!, the explicit Runnable {} wrappers and trailing Unit are gone, and the double blank lines are removed.

Still open from the earlier reviews (none of them block the merge)

1. No Cancel button: the dialog still shows only Add, Delete and Apply, and you can close it with Esc or the window close button. The column and filter dialogs work the same way, so if this is intentional, ignore this. If you want Cancel back, add dlg.setupButton(CancelAction()) {} after dialogPane.build(dlg) in BaselineDialog.show().
2. Cancel doesn't undo changes: closing without Apply still keeps renames, because BaselineItem.title writes straight to GanttPreviousState. It also keeps chart show/hide toggles and colour picks. The old Swing dialog behaved the same, so this isn't a regression.
3. createTasks threading: GanttPreviousState.createTasks(...) in the Add factory still reads the task tree on the FX thread.
4. Empty names: the name uses voidValidator, so empty baseline names are still accepted.
5. DefaultColorOption listener leak: each opening of the dialog still adds listeners to the colour options that are never removed. It's marked with a TODO as a follow-up. With this commit the leaked pickers now sit alongside a repaint listener. That's harmless, but it's one more reason to fix the leak eventually.

I think the PR is ready to merge once you've decided on the Cancel button (point 1).