From f62c36a4e369ea2098bcc6cdbdb328a9bd26cef4 Mon Sep 17 00:00:00 2001 From: Pelotrio <45769595+Pelotrio@users.noreply.github.com> Date: Wed, 30 Sep 2026 11:21:09 +0200 Subject: [PATCH 01/12] Companion's systems, first slice: signals and one page loader docs/SYSTEMS.md describes the few systems every feature of Companion uses, and the rules that keep features on them. This is its first PR: the mechanisms with their first users. - Signal: an owner's change, carrying nothing. The catalog, the change record, the packs, the resource edits and the key assignments fire one instead of keeping their own listener lists. - PageLoader: a page names itself and the signals it follows. It reads when first shown, and after a signal once it is shown; a follow can say whether a change concerns the page; a page that writes holds its reads. - The Key bindings page, which only shows, and the resource editor, which edits, move completely onto it. They no longer read in their constructors or on navigation, and hidden resource tabs no longer read on every reload. The editor's read and write counters and follow flags become the loader's hold. - Key assignments are read again right after Companion's own write, so the page shows it without reading again. - Tables.keepingSelection keeps a table's selection and scroll when it is read again. - SystemsRulesTest lists every file that makes its own thread, listener list, watcher or shared-pool call today; a new one fails the build. Co-Authored-By: Claude Opus 5.5 (1M context) --- AGENTS.md | 2 + .../CompanionApplication.java | 4 +- .../catalog/KeyAssignments.java | 21 +- .../catalog/KeyBindingControl.java | 25 +- .../catalog/KeyBindingLabels.java | 2 +- .../catalog/PackCatalogService.java | 14 +- .../model/DefinitionView.java | 3 +- .../model/InspectionView.java | 3 +- .../model/KeyBindingsView.java | 5 - .../totalDebugCompanion/model/ModView.java | 3 +- .../navigation/NavigationService.java | 5 +- .../totalDebugCompanion/pack/GamePacks.java | 31 +- .../pack/ResourceEdits.java | 35 +-- .../project/ProjectScope.java | 2 +- .../storage/ChangeRecord.java | 17 +- .../ui/components/PageLoader.java | 127 ++++++-- .../ui/components/Tables.java | 26 ++ .../ui/components/catalog/ChangesPanel.java | 2 +- .../ui/components/catalog/ContentPanel.java | 2 +- .../components/catalog/DefinitionDetails.java | 2 +- .../components/catalog/KeyBindingsPanel.java | 46 +-- .../ui/components/catalog/LogsPanel.java | 2 +- .../ui/components/catalog/ModPanel.java | 2 +- .../catalog/PackConfigurationPanel.java | 2 +- .../catalog/PackResourcesPanel.java | 6 +- .../ui/components/catalog/WorldPanel.java | 4 +- .../editors/PackResourceEditor.java | 271 ++++++++---------- .../ui/components/treeView/FileTreeView.java | 3 +- .../totalDebugCompanion/util/Signal.java | 26 ++ .../totalDebugCompanion/SystemsRulesTest.java | 219 ++++++++++++++ .../catalog/ConfigChangesTest.java | 18 +- .../catalog/KeyAssignmentsTest.java | 4 +- .../catalog/KeyBindingControlTest.java | 23 +- .../catalog/PackCatalogServiceTest.java | 4 +- .../navigation/PageReadsTest.java | 90 ++++++ .../pack/GamePacksTest.java | 4 +- .../pack/PackSelectionsTest.java | 2 +- .../pack/ResourceEditsTest.java | 4 +- .../storage/ChangeRecordTest.java | 4 +- .../testui/UiTestScope.java | 15 + .../ui/components/PageLoaderTest.java | 98 +++++++ .../components/catalog/CatalogPanelsTest.java | 38 ++- .../components/catalog/ChangesPanelTest.java | 18 +- .../editors/ResourceTextEditorTest.java | 11 + .../components/editors/TextureEditorTest.java | 7 + docs/SYSTEMS.md | 219 ++++++++++++++ docs/UI_GUIDE.md | 2 +- 47 files changed, 1134 insertions(+), 339 deletions(-) create mode 100644 companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/util/Signal.java create mode 100644 companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/SystemsRulesTest.java create mode 100644 companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/navigation/PageReadsTest.java create mode 100644 docs/SYSTEMS.md diff --git a/AGENTS.md b/AGENTS.md index 2716bc61c..1abbbce3d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -16,6 +16,8 @@ UI text must serve an action: labels, actual state, errors, or necessary instruc Do not use the middle dot (`ยท`) as a separator anywhere: UI text, tooltips, tests or documentation. Separate metadata the way the surrounding view already does: muted secondary text or a secondary column in lists (`PrimarySecondaryText`), spacing between parts in the subject header, or commas inside a value. +Companion's features use the systems in [docs/SYSTEMS.md](docs/SYSTEMS.md) to learn of changes, watch files, load pages, run work off the Swing thread, write and receive game messages. Do not add a listener list, file watcher, thread or executor, page loading path or message route beside them; a need they do not meet changes the system, with a decision recorded in that document. Fix a problem of timing, staleness or reading twice in the system that owns it, not with a flag in a feature. `SystemsRulesTest` checks the parts it can. + Keep the evaluator and compiled Code mode within their existing responsibilities. Changes to that architecture require an explicit design decision. Match checks to the changed behavior and affected consumers. Documentation-only changes need link and diff checks. Preserve unrelated working-tree changes and coordinate file ownership when another task is editing the same repository. diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/CompanionApplication.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/CompanionApplication.java index fdcde3ca0..56450a6e6 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/CompanionApplication.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/CompanionApplication.java @@ -809,10 +809,10 @@ private void activateProfile(CompanionProfile requested) throws IOException { /** Shows the project's saved pack catalog and item icons, which stay browsable without a game connection. */ private void restoreCatalog(ProjectScope scope) { - scope.catalog().addListener(() -> { + scope.catalog().changed().subscribe(() -> { if (currentScope() == scope) onUi(CompanionUi::catalogChanged); }); - scope.changes().addListener(() -> { + scope.changes().changed().subscribe(() -> { if (currentScope() == scope) onUi(CompanionUi::changesRecorded); }); itemIcons.setItemLookup(itemId -> scope.catalog().index().flatMap(index -> index.itemIcon(itemId))); diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java index 956158d2a..c6a92d72f 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java @@ -1,5 +1,6 @@ package com.github.minecraft_ta.totalDebugCompanion.catalog; +import com.github.minecraft_ta.totalDebugCompanion.util.Signal; import java.io.IOException; import java.nio.file.ClosedWatchServiceException; import java.nio.file.FileSystems; @@ -22,14 +23,15 @@ * screen, Companion or an editor, the listeners hear of it once the assignments differ from those read before. Another * option written, such as the volume, tells nobody. The file's folder is watched, so a file replaced by a rename, as the * game and Companion write it, is seen; a deleted file assigns nothing. A file that cannot be read keeps the assignments - * read before, and is read again after 1, 5 and 30 seconds and at its next change. + * read before, and is read again after 1, 5 and 30 seconds and at its next change. Companion's own writes are read at once + * ({@link #readNow()}), so a page shows them without waiting for the watch. */ public final class KeyAssignments implements AutoCloseable { private static final long SETTLE_MILLIS = 300; private static final List RETRY_MILLIS = List.of(1_000L, 5_000L, 30_000L); private final Path options; - private final List listeners = new CopyOnWriteArrayList<>(); + private final Signal changed = new Signal(); private final ScheduledExecutorService timer = Executors.newSingleThreadScheduledExecutor(task -> Thread.ofPlatform() .daemon() .name("Companion options.txt") @@ -74,10 +76,15 @@ private static WatchService watcher(Path folder) { } } - /** Runs {@code listener} on a Companion thread after the assignments changed; returns its removal. */ - public Runnable addListener(Runnable listener) { - this.listeners.add(Objects.requireNonNull(listener, "listener")); - return () -> this.listeners.remove(listener); + /** Fires on a Companion thread after the assignments changed. */ + public Signal changed() { + return this.changed; + } + + /** Reads the file now rather than once a watch saw it settle, as after Companion wrote it. */ + public synchronized void readNow() { + long generation = ++this.generation; + schedule(() -> read(0, generation), 0); } private void watch() { @@ -135,7 +142,7 @@ private void read(int attempt, long generation) { this.read = now; this.unreadable = false; } - if (changed) this.listeners.forEach(Runnable::run); + if (changed) this.changed.fire(); } /** Stops watching. It never fails, so a project's shutdown goes on to its queued writes after it. */ diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControl.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControl.java index b7cdc5bb3..2b636a373 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControl.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControl.java @@ -1,5 +1,6 @@ package com.github.minecraft_ta.totalDebugCompanion.catalog; +import com.github.minecraft_ta.totalDebugCompanion.util.Signal; import com.github.minecraft_ta.totalDebugCompanion.change.ChangeCategory; import com.github.minecraft_ta.totalDebugCompanion.change.ChangePipeline; import com.github.minecraft_ta.totalDebugCompanion.game.Access; @@ -20,7 +21,6 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.function.Consumer; -import java.util.function.Function; /** * Puts key bindings on keys, as a category of the {@link ChangePipeline}: a binding is named as in {@code options.txt}, @@ -36,24 +36,21 @@ public record Change(String name, KeyBindings.Assignment shown, KeyBindings.Assi private final ChangePipeline pipeline; private final Path options; - private final Function assignmentsChanged; + private final KeyAssignments assignments; /** - * Changes the keys of the pipeline's game, in its {@code options.txt}. {@code assignmentsChanged} adds a listener for - * the keys that file assigns changing, whoever wrote it ({@link KeyAssignments}), and returns its removal. + * Changes the keys of the pipeline's game, in its {@code options.txt}, whose keys {@code assignments} follows, whoever + * writes the file. */ - public KeyBindingControl(ChangePipeline pipeline, Function assignmentsChanged) { + public KeyBindingControl(ChangePipeline pipeline, KeyAssignments assignments) { this.pipeline = Objects.requireNonNull(pipeline, "pipeline"); this.options = pipeline.location().workspace().resolve("options.txt"); - this.assignmentsChanged = Objects.requireNonNull(assignmentsChanged, "assignmentsChanged"); + this.assignments = Objects.requireNonNull(assignments, "assignments"); } - /** - * Runs {@code listener} when the keys {@code options.txt} assigns changed, such as a key rebound in the game's - * controls screen; returns its removal. - */ - public Runnable addAssignmentListener(Runnable listener) { - return this.assignmentsChanged.apply(listener); + /** Fires when the keys {@code options.txt} assigns changed, such as a key rebound in the game's controls screen. */ + public Signal assignmentsChanged() { + return this.assignments.changed(); } /** The change record the bindings' changes are entered in. */ @@ -73,7 +70,9 @@ public Path options() { public CompletableFuture set(List changes) { List> edits = changes.stream().map(change -> new ChangePipeline.Edit<>( new ChangeRecord.KeyBinding(change.name()), change.shown().encode(), change.assignment().encode())).toList(); - return this.pipeline.change(this, edits).handle((done, failure) -> failure == null ? "" : message(failure)); + // The game saved options.txt before it answered, or Companion wrote it: the owner of the keys reads what changed. + return this.pipeline.change(this, edits).whenComplete((done, failure) -> this.assignments.readNow()) + .handle((done, failure) -> failure == null ? "" : message(failure)); } private static String message(Throwable failure) { diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingLabels.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingLabels.java index 70f09d342..0e0e5600e 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingLabels.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingLabels.java @@ -23,7 +23,7 @@ public final class KeyBindingLabels implements ChangeLabels { /** A key rebound outside Companion, such as in the game, changes the key a row shows. */ @Override public Runnable follow(Runnable listener) { - return this.keys.addAssignmentListener(listener); + return this.keys.assignmentsChanged().subscribe(listener); } public KeyBindingLabels(KeyBindingControl keys) { diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/PackCatalogService.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/PackCatalogService.java index 5b3117603..fd59f259c 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/PackCatalogService.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/PackCatalogService.java @@ -1,5 +1,6 @@ package com.github.minecraft_ta.totalDebugCompanion.catalog; +import com.github.minecraft_ta.totalDebugCompanion.util.Signal; import com.github.minecraft_ta.totaldebug.storage.InstancePaths; import com.github.minecraft_ta.totaldebug.storage.PackCatalog; import com.github.minecraft_ta.totaldebug.storage.RuntimeInventory; @@ -44,7 +45,7 @@ public record Failed(String detail) implements State { } private final InstancePaths paths; - private final List listeners = new CopyOnWriteArrayList<>(); + private final Signal changed = new Signal(); private State state = new None(); private long generation; @@ -61,10 +62,9 @@ public Optional index() { return Optional.ofNullable(shown(state())); } - /** Listeners run on the Swing thread after the state changed. */ - public Runnable addListener(Runnable listener) { - this.listeners.add(Objects.requireNonNull(listener, "listener")); - return () -> this.listeners.remove(listener); + /** Fires on the Swing thread after the state changed. */ + public Signal changed() { + return this.changed; } /** @@ -108,7 +108,7 @@ public void capturing() { this.state = new Capturing(shown); } // Pages that show the catalog go on showing it; only one without a catalog says it is being captured. - if (!shownBefore) SwingUtilities.invokeLater(() -> this.listeners.forEach(Runnable::run)); + if (!shownBefore) SwingUtilities.invokeLater(this.changed::fire); } /** @@ -183,7 +183,7 @@ private void apply(long expectedGeneration, State state) { } this.state = state; } - SwingUtilities.invokeLater(() -> this.listeners.forEach(Runnable::run)); + SwingUtilities.invokeLater(this.changed::fire); } /** The catalog {@code state} shows: the one ready, or while Minecraft captures it again, the one before; or null. */ diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/DefinitionView.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/DefinitionView.java index cefebb772..4eb058339 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/DefinitionView.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/DefinitionView.java @@ -1,5 +1,6 @@ package com.github.minecraft_ta.totalDebugCompanion.model; +import com.github.minecraft_ta.totalDebugCompanion.storage.ChangeRecord; import com.github.minecraft_ta.totalDebugCompanion.navigation.NavigationTarget; import com.github.minecraft_ta.totalDebugCompanion.ui.EditorContext; import com.github.minecraft_ta.totalDebugCompanion.ui.components.catalog.DefinitionDetails; @@ -18,7 +19,7 @@ public DefinitionView(EditorContext context, SubjectRef.Definition subject) { this.subject = subject; this.panel = SubjectPanel.definition(subject, new DefinitionDetails.Services(context.project().catalog(), () -> context.project().sources(), context.itemIcons(), context.navigation()::navigate, - context.project().packs()::addResourcePackListener)); + context.project().packs().changed(ChangeRecord.PackSide.RESOURCES)::subscribe)); } public SubjectRef.Definition subject() { diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/InspectionView.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/InspectionView.java index 14b491668..ac6266f0b 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/InspectionView.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/InspectionView.java @@ -1,5 +1,6 @@ package com.github.minecraft_ta.totalDebugCompanion.model; +import com.github.minecraft_ta.totalDebugCompanion.storage.ChangeRecord; import com.github.minecraft_ta.totalDebugCompanion.navigation.NavigationTarget; import com.github.minecraft_ta.totalDebugCompanion.runtime.RuntimeBinding; import com.github.minecraft_ta.totalDebugCompanion.ui.EditorContext; @@ -22,7 +23,7 @@ public InspectionView(EditorContext context, InspectSubjectPayload subject, Runt this.subject = Objects.requireNonNull(subject, "subject"); this.panel = SubjectPanel.occurrence(subject, context.snippets(), () -> context.project().scriptFiles(), new DefinitionDetails.Services(context.project().catalog(), () -> context.project().sources(), - context.itemIcons(), context.navigation()::navigate, context.project().packs()::addResourcePackListener)); + context.itemIcons(), context.navigation()::navigate, context.project().packs().changed(ChangeRecord.PackSide.RESOURCES)::subscribe)); } public InspectSubjectPayload subject() { diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/KeyBindingsView.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/KeyBindingsView.java index f89f782f4..8801cd2b3 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/KeyBindingsView.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/KeyBindingsView.java @@ -17,11 +17,6 @@ public KeyBindingsView(EditorContext context) { context.navigation()::navigate); } - /** Reads the keys again. */ - public void refresh() { - this.panel.load(); - } - /** Shows a binding, such as {@code key.jump}; an empty name shows none. */ public void show(String binding) { if (!binding.isEmpty()) this.panel.select(binding); diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/ModView.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/ModView.java index d13a47dd2..9cdc97baf 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/ModView.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/model/ModView.java @@ -1,5 +1,6 @@ package com.github.minecraft_ta.totalDebugCompanion.model; +import com.github.minecraft_ta.totalDebugCompanion.storage.ChangeRecord; import com.github.minecraft_ta.totalDebugCompanion.Icons; import com.github.minecraft_ta.totalDebugCompanion.navigation.NavigationTarget; import com.github.minecraft_ta.totalDebugCompanion.ui.EditorContext; @@ -15,7 +16,7 @@ public final class ModView implements IEditorPanel { public ModView(EditorContext context, NavigationTarget.ModPage page) { this.panel = new ModPanel(page.modId(), context.project().catalog(), () -> context.project().sources(), context.itemIcons(), context.project().profile().workspaceDirectory(), context.project().configSettings(), - context.project().keyBindings(), context.navigation()::navigate, context.project().packs()::addResourcePackListener); + context.project().keyBindings(), context.navigation()::navigate, context.project().packs().changed(ChangeRecord.PackSide.RESOURCES)::subscribe); this.panel.show(page); } diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/navigation/NavigationService.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/navigation/NavigationService.java index 9d4d79b68..3e68730f4 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/navigation/NavigationService.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/navigation/NavigationService.java @@ -342,10 +342,7 @@ private CompletableFuture performNavigation(NavigationTarget target, Activ KeyBindingsView.class, view -> true, () -> new KeyBindingsView(editors.get()) - ).thenAccept(view -> { - view.refresh(); - view.show(keys.binding()); - }), activation); + ).thenAccept(view -> view.show(keys.binding())), activation); case NavigationTarget.Pack pack -> dispatchNavigation(() -> this.tabs.focusOrCreateIfAbsent( PackView.class, view -> view.file().equals(pack.file()), diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/GamePacks.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/GamePacks.java index f3badbd2a..b21e72b99 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/GamePacks.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/GamePacks.java @@ -1,5 +1,6 @@ package com.github.minecraft_ta.totalDebugCompanion.pack; +import com.github.minecraft_ta.totalDebugCompanion.util.Signal; import com.github.minecraft_ta.totalDebugCompanion.catalog.CurrentWorld; import com.github.minecraft_ta.totalDebugCompanion.catalog.ListedPack; import com.github.minecraft_ta.totalDebugCompanion.catalog.PackFolders; @@ -29,7 +30,7 @@ * it plays names, each in its order; without them, what {@code options.txt} and a world's {@code level.dat} enable. It * answers which pack's copy of a file the game uses, for the views and the edits that show whether a copy is used. * - *

Each side tells its own listeners, only when its packs may differ: the resource packs whenever the game client + *

Each side has its own signal, fired only when its packs may differ: the resource packs whenever the game client * names them, and the datapacks whenever the world's server does, since each names them after a load of its resources, * which may change the packs' files though not their order; the datapacks also when the game goes to another world; and * a side whose file Companion wrote. A disconnect changes both, since the files take over from the game. @@ -45,9 +46,9 @@ public final class GamePacks { private volatile String worldRefusal = ""; /** What the game played when its server named {@link #datapacks}, whose datapacks they are. */ private volatile PlayingPayload datapacksFor; - private final Map> listeners = new EnumMap<>(Map.of( - ChangeRecord.PackSide.RESOURCES, new CopyOnWriteArrayList<>(), - ChangeRecord.PackSide.DATA, new CopyOnWriteArrayList<>())); + private final Map changed = new EnumMap<>(Map.of( + ChangeRecord.PackSide.RESOURCES, new Signal(), + ChangeRecord.PackSide.DATA, new Signal())); /** The packs of the game {@code location} tells of. */ public GamePacks(GameLocation location) { @@ -172,13 +173,9 @@ private List enabled(String path) { return stack == null ? null : stack.enabled(); } - /** - * Runs {@code listener} whenever the packs of {@code side} may differ, on the thread that saw it; returns its removal. - */ - public Runnable addListener(ChangeRecord.PackSide side, Runnable listener) { - List listeners = this.listeners.get(Objects.requireNonNull(side, "side")); - listeners.add(listener); - return () -> listeners.remove(listener); + /** Fires whenever the packs of {@code side} may differ, on the thread that saw it. */ + public Signal changed(ChangeRecord.PackSide side) { + return this.changed.get(Objects.requireNonNull(side, "side")); } /** The side whose packs supply {@code path}: resource packs for {@code assets/}, datapacks for the rest. */ @@ -186,18 +183,8 @@ public static ChangeRecord.PackSide side(String path) { return path.startsWith("assets/") ? ChangeRecord.PackSide.RESOURCES : ChangeRecord.PackSide.DATA; } - /** Listens to the resource packs, for {@code PageLoader.follow}. */ - public Runnable addResourcePackListener(Runnable listener) { - return addListener(ChangeRecord.PackSide.RESOURCES, listener); - } - - /** Listens to the datapacks of the world the game plays, for {@code PageLoader.follow}. */ - public Runnable addDatapackListener(Runnable listener) { - return addListener(ChangeRecord.PackSide.DATA, listener); - } - private void tell(ChangeRecord.PackSide side) { - this.listeners.get(side).forEach(Runnable::run); + this.changed.get(side).fire(); } /** diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java index 279755b2a..0ec9ee097 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java @@ -1,5 +1,6 @@ package com.github.minecraft_ta.totalDebugCompanion.pack; +import com.github.minecraft_ta.totalDebugCompanion.util.Signal; import com.github.minecraft_ta.totalDebugCompanion.catalog.PackFolders; import com.github.minecraft_ta.totalDebugCompanion.change.ChangeCategory; import com.github.minecraft_ta.totalDebugCompanion.change.ChangePipeline; @@ -83,9 +84,9 @@ public record Saved(Effect effect, Path pack, List problems, String relo private final Executor writes; private final InstanceState state; /** Run after a save or revert has written its file and the game used it, or failed to. */ - private final List editListeners = new CopyOnWriteArrayList<>(); - /** Run when a working pack is chosen, which changes where resource tabs save. */ - private final List> workingPackListeners = new CopyOnWriteArrayList<>(); + private final Signal edited = new Signal(); + /** Fired by side when its working pack is chosen, which changes where resource tabs save. */ + private final Map workingPackChosen = Map.of("assets", new Signal(), "data", new Signal()); /** The hash of what Companion last wrote to each resource, by the write queue, so a program's save is told from it. */ private final Map lastWritten = new ConcurrentHashMap<>(); /** Opens resources in other programs and takes their saves; started when the first is opened. */ @@ -136,18 +137,14 @@ public void close() { this.external.close(); } - /** - * Runs {@code listener} after each save or revert has finished, the managed pack enabled and the game reloaded; - * returns its removal. - */ - public Runnable addEditListener(Runnable listener) { - this.editListeners.add(listener); - return () -> this.editListeners.remove(listener); + /** Fires after each save or revert has finished, the managed pack enabled and the game reloaded. */ + public Signal edited() { + return this.edited; } - /** Tells the edit listeners once {@code edit} has finished, whether it worked or not. */ + /** Fires {@link #edited()} once {@code edit} has finished, whether it worked or not. */ private CompletableFuture finished(CompletableFuture edit) { - return edit.whenComplete((ignored, failure) -> this.editListeners.forEach(Runnable::run)); + return edit.whenComplete((ignored, failure) -> this.edited.fire()); } /** @@ -185,13 +182,17 @@ public List packs(String path) throws IOException { public void setWorkingPack(String path, Path pack) { String name = pack.getFileName().toString(); this.state.setWorkingPack(side(path), name.equals(PACK_NAME) ? "" : name); - this.workingPackListeners.forEach(listener -> listener.accept(side(path))); + this.workingPackChosen.get(side(path)).fire(); + } + + /** Fires whenever a working pack of {@code path}'s side is chosen. */ + public Signal workingPackChosen(String path) { + return this.workingPackChosen.get(side(path)); } - /** Runs {@code listener} with the side, as {@link #side} names it, whenever a working pack is chosen; returns what removes it. */ - public Runnable addWorkingPackListener(Consumer listener) { - this.workingPackListeners.add(listener); - return () -> this.workingPackListeners.remove(listener); + /** The name of the working pack chosen for {@code path}'s side, or empty for the pack Companion manages. */ + public String workingPackName(String path) { + return this.state.workingPack(side(path)); } /** Whether {@code pack} is a pack Companion manages, which it creates, enables and places on top. */ diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/project/ProjectScope.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/project/ProjectScope.java index 53af5095c..9fe18a276 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/project/ProjectScope.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/project/ProjectScope.java @@ -108,7 +108,7 @@ public ProjectScope(Object lock, CompanionProfile profile, InstanceState state, this.pipeline = new ChangePipeline(this.location, changes, this.configChanges.writes()); this.configSettings = new ConfigSettings(this.configChanges, this.pipeline); this.keyAssignments = new KeyAssignments(profile.workspaceDirectory().resolve("options.txt")); - this.keyBindings = new KeyBindingControl(this.pipeline, this.keyAssignments::addListener); + this.keyBindings = new KeyBindingControl(this.pipeline, this.keyAssignments); this.packs = new GamePacks(this.location); this.resources = new ResourceEdits(this.pipeline, this.packs, new ResourceOriginals(paths().originals()), this.configChanges.writes(), state); diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/storage/ChangeRecord.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/storage/ChangeRecord.java index 7cd3da48b..7816a7079 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/storage/ChangeRecord.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/storage/ChangeRecord.java @@ -1,5 +1,6 @@ package com.github.minecraft_ta.totalDebugCompanion.storage; +import com.github.minecraft_ta.totalDebugCompanion.util.Signal; import com.github.minecraft_ta.totaldebug.storage.InstancePaths; import com.github.minecraft_ta.totaldebug.storage.JsonFiles; import com.google.gson.JsonArray; @@ -113,7 +114,7 @@ public record Change(Target target, String original, String current, Instant fir /** The game directory the record's files are stored relative to, or null for a record that is not saved. */ private final Path gameDirectory; private final Map changes = new LinkedHashMap<>(); - private final List listeners = new CopyOnWriteArrayList<>(); + private final Signal changed = new Signal(); private ChangeRecord(JsonStateWriter writer, Clock clock, Path gameDirectory) { this.writer = writer; @@ -232,7 +233,7 @@ public void changed(Target target, String previous, String written, BiPredicate< } scheduleSave(); } - this.listeners.forEach(Runnable::run); + this.changed.fire(); } /** @@ -250,7 +251,7 @@ public void observed(Target target, String literal, BiPredicate scheduleSave(); } } - if (dropped) this.listeners.forEach(Runnable::run); + if (dropped) this.changed.fire(); } /** The value a setting of {@code file} had before Companion first changed it, or null when it is unchanged. */ @@ -286,13 +287,9 @@ public synchronized int size() { return this.changes.size(); } - /** - * Runs {@code listener} after every change of the record, on the thread that changed it, but not after a write that - * leaves it as it was; returns its removal. - */ - public Runnable addListener(Runnable listener) { - this.listeners.add(listener); - return () -> this.listeners.remove(listener); + /** Fires after every change of the record, on the thread that changed it, but not after a write that leaves it as it was. */ + public Signal changed() { + return this.changed; } private void scheduleSave() { diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java index f3f2f5ad3..29582d9b5 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java @@ -1,5 +1,7 @@ package com.github.minecraft_ta.totalDebugCompanion.ui.components; +import com.github.minecraft_ta.totalDebugCompanion.util.Signal; + import javax.swing.JComponent; import javax.swing.SwingUtilities; import java.awt.event.HierarchyEvent; @@ -10,15 +12,23 @@ import java.util.concurrent.Callable; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; +import java.util.function.BooleanSupplier; import java.util.function.Consumer; import java.util.function.Function; /** - * Reads what a page shows off the Swing thread and shows it on that thread (docs/UI_GUIDE.md, Building and checking). - * One read runs at a time: asking again while one runs reads once more when it finishes, and the older read is not - * shown, since it may predate what the new request is about, such as a write. So a page shows only what its last request - * read, and the same files are never read twice at once. A page loads whenever a source it follows changes; with a page - * named, a change while the page is hidden loads once it is shown, so hidden pages do no work for what they cannot show. + * Reads what a page shows off the Swing thread and shows it on that thread (docs/SYSTEMS.md, section 3). One read runs + * at a time: asking again while one runs reads once more when it finishes, and the older read is not shown, since it may + * predate what the new request is about, such as a write. So a page shows only what its last request read, and the same + * files are never read twice at once. + * + *

A page names itself ({@link #page}) and the signals it follows ({@link #follows}); the loader reads when the page is + * first shown, and after a followed signal fires, at once while the page is shown, otherwise once it is shown again. A + * page shown again with nothing changed reads nothing. While the page holds its reads ({@link #hold}), as during a save, + * signals wait in the same way. The page never reads in its constructor or because a navigation showed it.

+ * + *

Pages not moved to {@link #page} yet use the older modes {@link #whenShown}, {@link #waitsWhileHidden} and + * {@link #readsWhenShown} with {@link #follow}, and read in their constructors; the last of them to move deletes those.

*/ public final class PageLoader { /** What to read: prepared on the Swing thread, where the page's state is captured, then run off it. */ @@ -40,8 +50,15 @@ public interface Read { private boolean disposed; /** The page whose visibility decides when a followed change loads, or null to load at once. */ private JComponent page; - /** Whether a followed source changed while the page was hidden. */ - private boolean stale; + /** + * What the page missed while it was hidden or held: for each followed change, whether it concerns the page, asked when + * the page would read. Empty when there is nothing to read. + */ + private final List missed = new ArrayList<>(); + /** Whether the page holds its reads, as while it saves. */ + private boolean held; + /** How many reads started. */ + private int reads; /** Removes the listeners on the components whose showing this loader watches. */ private final List unwatch = new ArrayList<>(); /** @@ -57,6 +74,34 @@ public PageLoader(Read read, Consumer show, Consumer fail) { this.fail = Objects.requireNonNull(fail, "fail"); } + /** + * Reads for {@code page}: when it is first shown, and after a followed signal fired, once it is shown. It may be a part + * of a larger page, which then reads when that part is chosen. + */ + public PageLoader page(JComponent page) { + this.page = Objects.requireNonNull(page, "page"); + this.missed.add(() -> true); + watch(page, false); + if (page.isShowing()) SwingUtilities.invokeLater(this::resume); + return this; + } + + /** Reads again whenever {@code signal} fires. */ + public PageLoader follows(Signal signal) { + return follows(signal, () -> true); + } + + /** + * Reads again when {@code signal} fires and {@code concerns} says the change is one the page shows, such as the entry + * of its own file among all the changes of the record; {@code concerns} is asked on the Swing thread when the page + * would read, so a change the page made itself meanwhile does not count. + */ + public PageLoader follows(Signal signal, BooleanSupplier concerns) { + Objects.requireNonNull(concerns, "concerns"); + this.unsubscribe.add(signal.subscribe(() -> SwingUtilities.invokeLater(() -> changed(concerns)))); + return this; + } + /** * Waits while {@code page} is hidden and reads every time it is shown, since what it shows may have changed while it * was hidden without a source telling, such as a file the game writes. @@ -83,15 +128,16 @@ public PageLoader readsWhenShown(JComponent component) { return watch(component, true); } - /** Reads when {@code component} is shown: {@code always}, or only after a change it missed. */ + /** Reads when {@code component} is shown: {@code always}, or only what the page missed. */ private PageLoader watch(JComponent component, boolean always) { HierarchyListener listener = event -> { if ((event.getChangeFlags() & HierarchyEvent.SHOWING_CHANGED) == 0 || !component.isShowing()) return; - if (!always && !this.stale || this.showReadQueued) return; + if (!always && this.missed.isEmpty() || this.showReadQueued) return; this.showReadQueued = true; SwingUtilities.invokeLater(() -> { this.showReadQueued = false; - load(); + if (always) load(); + else resume(); }); }; component.addHierarchyListener(listener); @@ -101,30 +147,67 @@ private PageLoader watch(JComponent component, boolean always) { /** * Loads whenever a source changes, or once the page is shown again. {@code subscribe} adds a listener to the source - * and returns what removes it, as {@code catalog::addListener} does; the listener may be called on any thread. + * and returns what removes it, as {@code catalog.changed()::subscribe} does; the listener may be called on any thread. */ public PageLoader follow(Function subscribe) { - this.unsubscribe.add(subscribe.apply(() -> SwingUtilities.invokeLater(this::changed))); + this.unsubscribe.add(subscribe.apply(() -> SwingUtilities.invokeLater(() -> changed(() -> true)))); return this; } - /** A followed source changed: loads now, or marks a hidden page to load when shown. */ - private void changed() { - if (this.page != null && !this.page.isShowing()) { - this.stale = true; + /** A followed source changed: loads now where it concerns the page, or notes it while the page is hidden or held. */ + private void changed(BooleanSupplier concerns) { + if (this.disposed) return; + if (waiting()) { + this.missed.add(concerns); return; } - load(); + if (concerns.getAsBoolean()) load(); + } + + private boolean waiting() { + return this.held || this.page != null && !this.page.isShowing(); + } + + /** Reads what the page missed, now that it is shown and not held. */ + private void resume() { + if (this.disposed || waiting() || this.missed.isEmpty()) return; + boolean concerned = false; + for (BooleanSupplier concerns : this.missed) concerned |= concerns.getAsBoolean(); + this.missed.clear(); + if (concerned) load(); + } + + /** + * Holds reads until {@link #release()}, as while the page writes what it shows: a read under way is not shown, since it + * may predate the write, and followed changes wait as while the page is hidden. Swing thread only. + */ + public void hold() { + this.held = true; + if (this.running || this.again) { + // What that read was for is read again once released. + this.missed.add(() -> true); + cancel(); + } + } + + /** Ends {@link #hold()}, reading what the page missed meanwhile. Swing thread only. */ + public void release() { + this.held = false; + resume(); } - /** Reads again, now or once the running read has finished. */ + /** Reads again, now or once the running read has finished; while held, once released. */ public void load() { if (!SwingUtilities.isEventDispatchThread()) { SwingUtilities.invokeLater(this::load); return; } if (this.disposed) return; - this.stale = false; + if (this.held) { + this.missed.add(() -> true); + return; + } + this.missed.clear(); if (this.running) { this.again = true; return; @@ -133,6 +216,7 @@ public void load() { if (task == null) return; this.running = true; this.cancelled = false; + this.reads++; CompletableFuture finished = new CompletableFuture<>(); this.current = finished; CompletableFuture.supplyAsync(() -> { @@ -167,6 +251,11 @@ public CompletableFuture current() { return this.current; } + /** How many reads started, which tests count to show that a page reads once where it should. Swing thread only. */ + public int reads() { + return this.reads; + } + /** Drops the running read and any request waiting for it, as when the page moved on to something else. */ public void cancel() { this.again = false; diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/Tables.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/Tables.java index 0c381a348..b34e74e14 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/Tables.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/Tables.java @@ -5,10 +5,15 @@ import javax.swing.JLabel; import javax.swing.JTable; +import javax.swing.JViewport; import javax.swing.SwingConstants; import javax.swing.table.TableCellRenderer; import java.awt.Component; import java.awt.Dimension; +import java.awt.Point; +import java.util.HashSet; +import java.util.Set; +import java.util.function.IntFunction; /** * The table setup every Companion table shares (docs/UI_GUIDE.md): no grid, filling the viewport, headers aligned @@ -31,4 +36,25 @@ public static void configure(JTable table) { return component; }); } + + /** + * Runs {@code update}, which puts other rows in {@code table}, keeping what was selected and where the table was + * scrolled: the rows selected before are selected again where they still are, known by {@code identity} of a row as + * the table shows it, such as a binding's name. A table read again, as after a change elsewhere, stays as the user left + * it (docs/SYSTEMS.md, section 3). + */ + public static void keepingSelection(JTable table, IntFunction identity, Runnable update) { + Set selected = new HashSet<>(); + for (int row : table.getSelectedRows()) { + if (row < table.getRowCount()) selected.add(identity.apply(row)); + } + Point scrolled = table.getParent() instanceof JViewport viewport ? viewport.getViewPosition() : null; + update.run(); + if (!selected.isEmpty()) { + for (int row = 0; row < table.getRowCount(); row++) { + if (selected.contains(identity.apply(row))) table.addRowSelectionInterval(row, row); + } + } + if (scrolled != null && table.getParent() instanceof JViewport viewport) viewport.setViewPosition(scrolled); + } } diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ChangesPanel.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ChangesPanel.java index 94b8f7647..6448fd1d4 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ChangesPanel.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ChangesPanel.java @@ -223,7 +223,7 @@ public ChangesPanel(PackCatalogService catalog, ChangeRecord record, List(this::prepareLoad, this::show, failure -> setStatus("Could not read the changes: " + failure.getMessage())) - .whenShown(this).follow(this.record::addListener).follow(this.catalog::addListener); + .whenShown(this).follow(this.record.changed()::subscribe).follow(this.catalog.changed()::subscribe); for (ChangeLabels labels : categories) this.loader.follow(labels::follow); load(); } diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ContentPanel.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ContentPanel.java index ed8b409eb..03bb097a2 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ContentPanel.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ContentPanel.java @@ -35,7 +35,7 @@ public ContentPanel(PackCatalogService catalog, ItemIconService icons, Consumer< this.browser = new ContentBrowser(this.icons, this::iconOf, Objects.requireNonNull(navigator, "navigator"), entry -> this.index == null ? entry.namespace() : this.index.ownerName(entry.namespace())); this.message.setVerticalAlignment(JLabel.TOP); - this.removeCatalogListener = ShownUpdates.follow(this, catalog::addListener, this::load); + this.removeCatalogListener = ShownUpdates.follow(this, catalog.changed()::subscribe, this::load); load(); } diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/DefinitionDetails.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/DefinitionDetails.java index 57dbb6e37..cc10b052c 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/DefinitionDetails.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/DefinitionDetails.java @@ -118,7 +118,7 @@ public DefinitionDetails(SubjectRef.Definition subject, Services services, JComp this.matched = List.of(); showExtras(); }).waitsWhileHidden(page).follow(services.resourcesRead()); - this.removeCatalogListener = ShownUpdates.follow(page, services.catalog()::addListener, this::reload); + this.removeCatalogListener = ShownUpdates.follow(page, services.catalog().changed()::subscribe, this::reload); read(); loadAppearance(); loadResources(); diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/KeyBindingsPanel.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/KeyBindingsPanel.java index dd93d6a6c..92c88acbe 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/KeyBindingsPanel.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/KeyBindingsPanel.java @@ -27,7 +27,6 @@ import javax.swing.JPanel; import javax.swing.JPopupMenu; import javax.swing.JTable; -import javax.swing.JViewport; import javax.swing.KeyStroke; import javax.swing.ListSelectionModel; import javax.swing.SwingUtilities; @@ -40,7 +39,6 @@ import java.awt.Component; import java.awt.KeyEventDispatcher; import java.awt.KeyboardFocusManager; -import java.awt.Point; import java.awt.event.ActionEvent; import java.awt.event.HierarchyEvent; import java.awt.event.KeyEvent; @@ -163,11 +161,10 @@ public void mousePressed(MouseEvent event) { this.loader = new PageLoader<>(this::prepareLoad, loaded -> show(loaded.index(), loaded.bindings(), ""), failure -> show(this.catalog.index().orElse(null), null, "Could not read options.txt: " + failure.getMessage())) - .whenShown(this).follow(catalog::addListener).follow(control::addAssignmentListener); + .page(this).follows(catalog.changed()).follows(control.assignmentsChanged()); addHierarchyListener(event -> { if ((event.getChangeFlags() & HierarchyEvent.SHOWING_CHANGED) != 0 && !isShowing()) stopCapture(); }); - load(); } private void configureTable() { @@ -225,7 +222,6 @@ public void actionPerformed(ActionEvent event) { } } - /** Reads the keys again. */ /** The table, for tests. */ JTable table() { return this.table; @@ -236,8 +232,9 @@ CompletableFuture loading() { return this.loader.current(); } - public void load() { - this.loader.load(); + /** How many times the page read {@code options.txt}, which tests count. */ + public int reads() { + return this.loader.reads(); } /** Reads {@code options.txt} against the captured catalog; without one there is nothing to read. */ @@ -283,9 +280,6 @@ private void selectPending() { } private void show(CatalogIndex index, KeyBindings bindings, String problem) { - // Read again, as after a key rebound in the game, the table keeps what was selected and where it was scrolled. - Set selected = selectedRows(); - Point scrolled = this.table.getParent() instanceof JViewport viewport ? viewport.getViewPosition() : null; this.index = index; this.bindings = bindings; this.problem = bindings == null ? "" : problem; @@ -302,32 +296,18 @@ private void show(CatalogIndex index, KeyBindings bindings, String problem) { for (KeyBindings.Binding binding : members) rows.add(new Row(category, binding, 0)); }); } - this.model.setRows(rows); this.unavailable = bindings == null ? problem : ""; - applyFilter(); - if (this.selectAfterLoad == null) { - select(selected); - if (scrolled != null && this.table.getParent() instanceof JViewport viewport) viewport.setViewPosition(scrolled); - } + Runnable update = () -> { + this.model.setRows(rows); + applyFilter(); + }; + // Read again, as after a key rebound in the game, the table stays as it was, unless a binding is to be shown. + if (this.selectAfterLoad == null) Tables.keepingSelection(this.table, row -> identity(this.model.shown.get(row)), update); + else update.run(); selectPending(); } - /** The selected rows by what they show: a binding by its name, a category by its name. */ - private Set selectedRows() { - Set selected = new HashSet<>(); - for (int viewRow : this.table.getSelectedRows()) { - if (viewRow < this.model.shown.size()) selected.add(identity(this.model.shown.get(viewRow))); - } - return selected; - } - - private void select(Set rows) { - if (rows.isEmpty()) return; - for (int viewRow = 0; viewRow < this.model.shown.size(); viewRow++) { - if (rows.contains(identity(this.model.shown.get(viewRow)))) this.table.addRowSelectionInterval(viewRow, viewRow); - } - } - + /** A row by what it shows: a binding by its name, a category by its name. */ private static String identity(Row row) { return row.binding() == null ? "category " + row.category() : "binding " + row.binding().spec().name(); } @@ -554,8 +534,8 @@ private void apply(Map changes) { this.control.set(requests).thenAccept(reason -> SwingUtilities.invokeLater(() -> { Map failed = new LinkedHashMap<>(); if (!reason.isEmpty()) requests.forEach(request -> failed.put(request.name(), reason)); + // The keys changed show once their owner read them; only what failed is told here. setStatus(notChanged(failed, names)); - load(); })); } diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/LogsPanel.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/LogsPanel.java index 7dc8500b2..50a2d1365 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/LogsPanel.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/LogsPanel.java @@ -174,7 +174,7 @@ public void actionPerformed(ActionEvent event) { this.loader = new PageLoader>(() -> this::listFiles, this::showFiles, failure -> showMessage("The logs could not be listed: " + failure.getMessage())).whenShown(this); // Rows name the mods behind frames and failures as the catalog knows them. - this.removeCatalogListener = ShownUpdates.follow(this, catalog::addListener, () -> { + this.removeCatalogListener = ShownUpdates.follow(this, catalog.changed()::subscribe, () -> { if (this.disposed) return; this.rowsOf = null; showRows(); diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ModPanel.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ModPanel.java index 90d619d95..0cd6acb80 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ModPanel.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ModPanel.java @@ -146,7 +146,7 @@ public ModPanel(String modId, PackCatalogService catalog, Supplier(this::prepareLoad, loaded -> show(loaded, ""), failure -> show(new Loaded(List.of(), Map.of(), Map.of(), List.of()), "Could not read the configuration files: " + failure.getMessage())) - .whenShown(this).follow(catalog::addListener) + .whenShown(this).follow(catalog.changed()::subscribe) // A server configuration is shown from the copy of the world the game has open, which changes with it. .follow(listener -> this.location.addListener(change -> { if (change != GameLocation.Change.PROCESS) listener.run(); diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/PackResourcesPanel.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/PackResourcesPanel.java index b18e11338..5c4867331 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/PackResourcesPanel.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/PackResourcesPanel.java @@ -66,8 +66,8 @@ public PackResourcesPanel(PackCatalogService catalog, ResourceEdits edits, PackS TabTitles.setUncounted(this.tabs, 0, ResourcesTab.FILES.title()); this.browser.setResources(List.of()); this.browser.setMessage("Resources could not be read: " + failure.getMessage()); - }).waitsWhileHidden(this).follow(catalog::addListener).follow(edits.packs()::addResourcePackListener) - .follow(edits.packs()::addDatapackListener).follow(edits::addEditListener); + }).waitsWhileHidden(this).follow(catalog.changed()::subscribe).follow(edits.packs().changed(ChangeRecord.PackSide.RESOURCES)::subscribe) + .follow(edits.packs().changed(ChangeRecord.PackSide.DATA)::subscribe).follow(edits.edited()::subscribe); this.packLoader = new PageLoader<>(() -> { PackStackPayload stack = this.edits.packs().resourcePacks(); return () -> PackResources.resourcePacks(stack, this.workspace); @@ -80,7 +80,7 @@ public PackResourcesPanel(PackCatalogService catalog, ResourceEdits edits, PackS }) // Its count on the tab follows while the page is shown; the folders are read again when the tab is chosen. .waitsWhileHidden(this).readsWhenShown(this.packs) - .follow(edits.packs()::addResourcePackListener).follow(catalog::addListener); + .follow(edits.packs().changed(ChangeRecord.PackSide.RESOURCES)::subscribe).follow(catalog.changed()::subscribe); load(); } diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/WorldPanel.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/WorldPanel.java index f8fe1a633..60216d66b 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/WorldPanel.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/WorldPanel.java @@ -150,7 +150,7 @@ public WorldPanel(PackCatalogService catalog, ItemIconService icons, WorldReadin add(this.cards, BorderLayout.CENTER); // Only the names of the mods behind datapacks come from the catalog. - this.removeCatalogListener = ShownUpdates.follow(this, catalog::addListener, () -> { + this.removeCatalogListener = ShownUpdates.follow(this, catalog.changed()::subscribe, () -> { if (!this.disposed && (this.saved != null || this.server != null)) this.datapacks.setPacks(this.datapackList, this.catalog.index().orElse(null)); }); // The game saves the world while it runs, so the page reads it whenever it is shown. A change of the datapacks, @@ -161,7 +161,7 @@ public WorldPanel(PackCatalogService catalog, ItemIconService icons, WorldReadin String refusal = edits.packs().worldRefusal(); return () -> read(edits.location().read(), stack, refusal); }, this::show, failure -> show(Loaded.problem("The world could not be read: " + failure.getMessage()))) - .readsWhenShown(this).follow(edits.packs()::addDatapackListener); + .readsWhenShown(this).follow(edits.packs().changed(ChangeRecord.PackSide.DATA)::subscribe); } private static Loaded read(GameState game, PackStackPayload stack, String refusal) { diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java index 3ea322e5a..5f43fd3a7 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java @@ -9,6 +9,7 @@ import com.github.minecraft_ta.totalDebugCompanion.storage.ResourceOriginals; import com.github.minecraft_ta.totalDebugCompanion.ui.Tooltip; import com.github.minecraft_ta.totalDebugCompanion.ui.UiMetrics; +import com.github.minecraft_ta.totalDebugCompanion.ui.components.PageLoader; import com.github.minecraft_ta.totalDebugCompanion.ui.theme.ThemeColors; import javax.swing.BorderFactory; @@ -32,6 +33,7 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.concurrent.Callable; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.function.Consumer; @@ -51,10 +53,11 @@ abstract class PackResourceEditor extends JPanel { /** * The pack the content is saved in, its entry in the change record when the copy was read, its copy there or null and - * the hash of that copy's bytes, empty for none, why the game does not use that copy or null, and the packs it could be - * saved into instead, empty for an opened pack. + * the hash of that copy's bytes, empty for none, why the game does not use that copy or null, the packs it could be + * saved into instead, empty for an opened pack, and the working pack chosen when it was read, or null for an opened pack. */ - private record Found(Path pack, ChangeRecord.Change recorded, V managed, String hash, String unused, List targets) { + private record Found(Path pack, ChangeRecord.Change recorded, V managed, String hash, String unused, List targets, + String working) { } private final String path; @@ -71,39 +74,29 @@ private record Found(Path pack, ChangeRecord.Change recorded, V managed, Stri private Supplier noticeColor = ThemeColors::secondaryText; /** The content of the file that was opened, which the pack supplies while the managed pack holds no copy. */ private final V openedContent; - /** Stops following the change record. */ - private Runnable stopListening = () -> { }; - /** Stops following the game's packs. */ - private Runnable stopFollowingPacks = () -> { }; - /** Stops following the choice of working pack. */ - private Runnable stopFollowingWorkingPack = () -> { }; + /** Reads the copies when the tab is shown and after what changes them; held while a save runs. */ + private PageLoader> loader; /** The content a save under way writes, or null. */ private V saving; /** The folder pack the opened file lies in, which stays the target, or null to save into the working pack. */ private final Path opened; /** The content the pack supplies: the managed pack's copy, or the opened file's; {@link #none()} for no copy. */ private V packContent; - /** This resource's entry in the change record when the copies were last read, or null. */ + /** + * This resource's entry in the change record when the copies were last read or saved, or null: a change of the record + * that leaves it alone does not concern the tab. + */ private ChangeRecord.Change seen; + /** The working pack chosen when the copies were last read, or null before; an opened pack's tab has none. */ + private String readWorking; /** The pack the shown copy was read from, or null until it is read. */ private Path pack; /** The pack the content is saved in, as the bar names it. */ private String packName = "the working pack"; - /** Counts writes, so the copies read when the tab opened never replace the content of a later write. */ - private int writes; - /** Counts reads of the copies; only the latest may show its result, since reads can finish out of order. */ - private int reads; private boolean managed; private boolean busy; - /** - * The working pack or the current world changed and its copy is being read; a save waits for it, so it goes into the - * pack the tab shows. - */ + /** The copies are being read; a save waits for them, so it goes into the pack the tab shows. */ private boolean following; - /** The working pack or the current world changed during a save, and is followed once the save completes. */ - private boolean followAfterSave; - /** Whether that change ends what the notice said, as a new working pack does; a pack stack change does not. */ - private boolean clearAfterSave; /** * What a read of the copies last put in the notice, such as why the game does not use the shown copy, or null. The * next read replaces it; a save's result stays. @@ -242,15 +235,15 @@ public Component getListCellRendererComponent(JList list, Object value, int i this.following = true; changed(); this.seen = recorded(); - readCopies(false); - // A revert on the Changes page changes what the game uses. - this.stopListening = this.edits.record().addListener(() -> SwingUtilities.invokeLater(this::recordChanged)); - // Another world opening changes the current world's datapack a data file is shown from and saved into. - this.stopFollowingPacks = this.edits.packs().addListener(GamePacks.side(this.path), () -> SwingUtilities.invokeLater(this::packsChanged)); - // A working pack chosen in another tab is where this one saves too. - this.stopFollowingWorkingPack = this.edits.addWorkingPackListener(side -> { - if (side.equals(ResourceEdits.side(this.path))) SwingUtilities.invokeLater(this::targetChanged); - }); + // A revert on the Changes page changes what the game uses; the tab's own saves show their own result. + this.loader = new PageLoader<>(this::prepareCopies, this::showCopies, this::copiesFailed) + .page(this).follows(this.edits.record().changed(), () -> !Objects.equals(recorded(), this.seen)); + if (this.opened == null) { + // Another world opening changes the current world's datapack a data file is shown from and saved into, and a + // working pack chosen in another tab is where this one saves too. + this.loader.follows(this.edits.packs().changed(GamePacks.side(this.path))) + .follows(this.edits.workingPackChosen(this.path)); + } } private String saveTooltip() { @@ -272,42 +265,6 @@ private void chooseTarget() { this.edits.setWorkingPack(this.path, chosen); } - /** Shows the copy of the working pack chosen now, whose notice replaces the last pack's. */ - private void targetChanged() { - follow(true); - } - - /** Reads the copies again when the tab follows the current world, which may have changed. */ - private void packsChanged() { - follow(false); - } - - /** Reads the working pack's copy again, once a save under way completes; {@code changed} as for {@link #readCopies}. */ - private void follow(boolean changed) { - if (this.disposed || this.opened != null) return; - if (this.busy) { - this.followAfterSave = true; - this.clearAfterSave |= changed; - return; - } - this.following = true; - // A new working pack's copy is read before anything is edited, as when the tab opened. A pack stack change, which - // comes after every reload and rarely changes the pack, leaves typing and drawing alone. - if (changed) setEditable(false); - changed(); - readCopies(changed); - } - - /** Reads the copies again when this resource's entry in the change record changed. */ - private void recordChanged() { - if (this.disposed) return; - ChangeRecord.Change now = recorded(); - if (Objects.equals(now, this.seen)) return; - this.seen = now; - // A write of this editor shows its own result when it completes. - if (!this.busy) readCopies(true); - } - /** This resource's entry in the change record for its managed pack, or null while it has none or the pack is not known. */ private ChangeRecord.Change recorded() { return this.pack == null ? null : this.edits.record().change(new ChangeRecord.Resource(this.path, this.pack)); @@ -322,74 +279,77 @@ String noticeText() { } /** - * Shows the managed pack's copy instead of the opened file's, and names a pack that overrides the managed one. - * {@code changed} tells that the file itself changed, such as by a revert, which ends what the notice said about it. + * Reads the managed pack's copy, to show it instead of the opened file's, and whether a pack overrides it. Without an + * opened pack, the working pack is looked up each time: the player can open another world or choose another pack. */ - private void readCopies(boolean changed) { - int started = this.writes; - int read = ++this.reads; - CompletableFuture.supplyAsync(() -> { - try { - // Without an opened pack, the working pack is looked up each time: the player can open another world or - // choose another pack. - Path pack = this.opened != null ? this.opened : this.edits.pack(this.path); - // The record is looked at before the file, so a change between the two is caught afterwards. - ChangeRecord.Change recorded = this.edits.record().change(new ChangeRecord.Resource(this.path, pack)); - Optional copy = this.edits.managed(pack, this.path); - return new Found<>(pack, recorded, copy.isPresent() ? decode(copy.get(), pack) : null, - ResourceOriginals.hash(copy.orElse(null)), - this.edits.packs().unusedBecause(this.path, pack).orElse(null), - this.opened != null ? List.of() : this.edits.packs(this.path)); - } catch (Exception exception) { - throw new CompletionException(exception); - } - }).whenComplete((found, failure) -> SwingUtilities.invokeLater(() -> { - if (this.disposed || this.writes != started || this.reads != read) return; - // A read that fails leaves the pack to save into unknown, so saving waits for the next one. - if (failure != null) { - this.readNotice = message(failure); - showNotice(this.readNotice, ThemeColors::error); - return; - } - this.following = false; - setEditable(true); - showPack(found.pack()); - showTargets(found.targets(), found.pack()); - this.seen = found.recorded(); - if (changed) showNotice("", ThemeColors::secondaryText); - // Without a managed copy, such as after a revert, the pack supplies the opened file's content again, unless - // the opened file was the managed copy: then nothing is left, and the shown content is unsaved. - this.managed = found.managed() != null; - this.packContent = this.managed ? found.managed() : this.opened != null ? none() : this.openedContent; - // A deleted file keeps its content on screen, as unsaved content a Save would write again. - if (!modified() && (this.managed || this.opened == null)) load(this.packContent); - else markSaved(this.packContent); - // Changes that match the copy read now, such as another tab's save of the same text, are unsaved no more. - boolean unsaved = modified(); - // Unsaved changes keep the copy they were made to as the one a save replaces, so replacing another asks. A tab - // moved to another pack, such as a newly chosen working pack, carries its changes over to that pack's copy. - boolean samePack = found.pack().equals(this.baselinePack); - boolean changedSince = unsaved && samePack && this.baseline != null && !this.baseline.equals(found.hash()); - if (!unsaved || this.baseline == null || !samePack) { - this.baseline = found.hash(); - this.baselinePack = found.pack(); - } - this.packHash = found.hash(); - showState(""); - // Why the game did not use the copy ends when it does now, such as after the player enabled the pack. - boolean replaceable = this.notice.getText().isEmpty() || this.notice.getText().equals(this.readNotice); - if (replaceable) showNotice("", ThemeColors::secondaryText); - this.readNotice = found.unused(); - // A save's reload failure or problems stay: they tell why, such as a pack the game turned off after a failure. - if (found.unused() != null && replaceable) showNotice(found.unused(), ThemeColors::warning); - if (changedSince && replaceable) { - this.readNotice = "The " + noun() + " changed in the pack since this tab read it; saving asks before replacing it"; - showNotice(this.readNotice, ThemeColors::warning); - } - changed(); - // A save or revert that came after the record was looked at is read again. - if (!Objects.equals(recorded(), this.seen)) readCopies(true); - })); + private Callable> prepareCopies() { + String working = this.opened != null ? null : this.edits.workingPackName(this.path); + // A new working pack's copy is read before anything is edited, as when the tab opened. A pack stack change, which + // comes after every reload and rarely changes the pack, leaves typing and drawing alone. + if (working != null && !working.equals(this.readWorking)) setEditable(false); + this.following = true; + changed(); + return () -> { + Path pack = this.opened != null ? this.opened : this.edits.pack(this.path); + // The record is looked at before the file, so a change between the two is caught afterwards. + ChangeRecord.Change recorded = this.edits.record().change(new ChangeRecord.Resource(this.path, pack)); + Optional copy = this.edits.managed(pack, this.path); + return new Found<>(pack, recorded, copy.isPresent() ? decode(copy.get(), pack) : null, + ResourceOriginals.hash(copy.orElse(null)), + this.edits.packs().unusedBecause(this.path, pack).orElse(null), + this.opened != null ? List.of() : this.edits.packs(this.path), working); + }; + } + + /** A read that fails leaves the pack to save into unknown, so saving waits for the next one. */ + private void copiesFailed(Throwable failure) { + this.readNotice = message(failure); + showNotice(this.readNotice, ThemeColors::error); + } + + /** + * Shows the copies read. Another pack, as a newly chosen working pack, or a change of the file itself, as a revert, + * ends what the notice said about the copy shown before. + */ + private void showCopies(Found found) { + boolean changed = this.pack != null && (!found.pack().equals(this.pack) || !Objects.equals(found.recorded(), this.seen)); + this.following = false; + this.readWorking = found.working(); + setEditable(true); + showPack(found.pack()); + showTargets(found.targets(), found.pack()); + this.seen = found.recorded(); + if (changed) showNotice("", ThemeColors::secondaryText); + // Without a managed copy, such as after a revert, the pack supplies the opened file's content again, unless + // the opened file was the managed copy: then nothing is left, and the shown content is unsaved. + this.managed = found.managed() != null; + this.packContent = this.managed ? found.managed() : this.opened != null ? none() : this.openedContent; + // A deleted file keeps its content on screen, as unsaved content a Save would write again. + if (!modified() && (this.managed || this.opened == null)) load(this.packContent); + else markSaved(this.packContent); + // Changes that match the copy read now, such as another tab's save of the same text, are unsaved no more. + boolean unsaved = modified(); + // Unsaved changes keep the copy they were made to as the one a save replaces, so replacing another asks. A tab + // moved to another pack, such as a newly chosen working pack, carries its changes over to that pack's copy. + boolean samePack = found.pack().equals(this.baselinePack); + boolean changedSince = unsaved && samePack && this.baseline != null && !this.baseline.equals(found.hash()); + if (!unsaved || this.baseline == null || !samePack) { + this.baseline = found.hash(); + this.baselinePack = found.pack(); + } + this.packHash = found.hash(); + showState(""); + // Why the game did not use the copy ends when it does now, such as after the player enabled the pack. + boolean replaceable = this.notice.getText().isEmpty() || this.notice.getText().equals(this.readNotice); + if (replaceable) showNotice("", ThemeColors::secondaryText); + this.readNotice = found.unused(); + // A save's reload failure or problems stay: they tell why, such as a pack the game turned off after a failure. + if (found.unused() != null && replaceable) showNotice(found.unused(), ThemeColors::warning); + if (changedSince && replaceable) { + this.readNotice = "The " + noun() + " changed in the pack since this tab read it; saving asks before replacing it"; + showNotice(this.readNotice, ThemeColors::warning); + } + changed(); } /** Lists the packs the content could be saved into, the current one selected; an opened pack's file shows none. */ @@ -453,6 +413,7 @@ protected final void saveThen(Consumer after) { Path pack = this.opened != null ? this.opened : this.pack; String read = this.packHash; this.busy = true; + this.loader.hold(); changed(); CompletableFuture.supplyAsync(() -> { try { @@ -467,27 +428,33 @@ protected final void saveThen(Consumer after) { if (this.disposed) return; if (failure != null) { showNotice("Not opened: " + message(failure), ThemeColors::error); + this.loader.release(); } else if (held.equals(read)) { + this.loader.release(); after.accept(pack); } else if (this.askToReplace.test(new ChangePipeline.Stale(this.path.substring(this.path.lastIndexOf('/') + 1) + " changed in " + this.packName + " since this tab read it"))) { this.baseline = null; - save(after); + // The save holds the reads on; where it does not start, they go on. + if (!save(after)) this.loader.release(); } else { readAfterSave(); + this.loader.release(); } })); } - private void save(Consumer after) { - if (this.busy || this.following || after == null && same(shown(), this.packContent)) return; + /** Saves as {@link #save()} does, then runs {@code after} with the pack; returns whether a save started. */ + private boolean save(Consumer after) { + if (this.busy || this.following || after == null && same(shown(), this.packContent)) return false; V edited = copy(shown()); Optional problem = check(edited); if (problem.isPresent()) { showNotice(problem.get(), ThemeColors::error); - return; + return false; } - this.writes++; + // The copies read before the write never replace what it writes; what changes meanwhile is read after it. + this.loader.hold(); this.busy = true; changed(); this.state.setText("Reloading in the game"); @@ -516,14 +483,16 @@ private void save(Consumer after) { // Asked before following a change that came during the save, which would hold the overwrite back. if (cause(failure) instanceof ChangePipeline.Stale changedSince && this.askToReplace.test(changedSince)) { this.baseline = null; - save(after); + if (!save(after)) this.loader.release(); return; } showNotice("Not saved: " + message(failure), ThemeColors::error); - followLater(); + // What the record got during the attempt is the save's to tell, not a change that ends its notice. + this.seen = recorded(); // A copy written since, which the save refused to replace, is read, so Discard goes back to it; // the changes on screen stay, still unsaved. readAfterSave(); + this.loader.release(); return; } this.baseline = written[0]; @@ -537,12 +506,13 @@ private void save(Consumer after) { else load(edited); showSaved(saved); changed(); - followLater(); // Another tab's save that came while this one ran was not read then. ChangeRecord.Change now = recorded(); if (now != null && !now.current().equals(written[0])) readAfterSave(); + this.loader.release(); if (after != null) after.accept(saved.pack()); })); + return true; } /** Shows what the game made of a save: whether it uses it now, and what its reload reported. */ @@ -556,18 +526,9 @@ protected final void showSaved(ResourceEdits.Saved saved) { } } - /** Reads the copies again after a save, unless following a working pack or world change does already. */ + /** Reads the copies again after a save, unless a read of them is under way already. */ private void readAfterSave() { - if (!this.following && !this.busy) readCopies(false); - } - - /** Follows a working pack or world change that came during the save just completed. */ - private void followLater() { - if (!this.followAfterSave) return; - boolean changed = this.clearAfterSave; - this.followAfterSave = false; - this.clearAfterSave = false; - follow(changed); + if (!this.following) this.loader.load(); } private void showResult(List problems, String reloadFailure) { @@ -643,8 +604,6 @@ protected final boolean disposed() { void dispose() { this.disposed = true; - this.stopListening.run(); - this.stopFollowingPacks.run(); - this.stopFollowingWorkingPack.run(); + this.loader.dispose(); } } diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/treeView/FileTreeView.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/treeView/FileTreeView.java index a812e484c..1757a2db8 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/treeView/FileTreeView.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/treeView/FileTreeView.java @@ -1,5 +1,6 @@ package com.github.minecraft_ta.totalDebugCompanion.ui.components.treeView; +import com.github.minecraft_ta.totalDebugCompanion.storage.ChangeRecord; import com.github.minecraft_ta.totalDebugCompanion.game.GameLocation; import com.github.minecraft_ta.totalDebugCompanion.project.ProjectScope; @@ -281,7 +282,7 @@ public List loadChildren() { }); }); // A server names its world's datapacks after the game joined it. - Runnable removeNamed = scope.packs().addDatapackListener(refresh); + Runnable removeNamed = scope.packs().changed(ChangeRecord.PackSide.DATA).subscribe(refresh); this.removeWorldListener = () -> { removeRead.run(); removePlayed.run(); diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/util/Signal.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/util/Signal.java new file mode 100644 index 000000000..9ccc5c7ab --- /dev/null +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/util/Signal.java @@ -0,0 +1,26 @@ +package com.github.minecraft_ta.totalDebugCompanion.util; + +import java.util.List; +import java.util.Objects; +import java.util.concurrent.CopyOnWriteArrayList; + +/** + * Tells that a part of an owner's state changed (docs/SYSTEMS.md, section 1). A signal carries nothing: a follower asks the + * owner for the value. The owner fires it after the value changed, never while holding its own lock. + */ +public final class Signal { + private final List listeners = new CopyOnWriteArrayList<>(); + + /** Runs {@code listener} after each change, on the thread that fired it; returns what removes it. */ + public Runnable subscribe(Runnable listener) { + // A wrapper of its own, so removing it never removes another subscription of the same listener. + Runnable subscription = Objects.requireNonNull(listener, "listener")::run; + this.listeners.add(subscription); + return () -> this.listeners.remove(subscription); + } + + /** Tells every listener, in the order they subscribed. */ + public void fire() { + this.listeners.forEach(Runnable::run); + } +} diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/SystemsRulesTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/SystemsRulesTest.java new file mode 100644 index 000000000..2e11c14e0 --- /dev/null +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/SystemsRulesTest.java @@ -0,0 +1,219 @@ +package com.github.minecraft_ta.totalDebugCompanion; + +import org.eclipse.jdt.core.JavaCore; +import org.eclipse.jdt.core.dom.AST; +import org.eclipse.jdt.core.dom.ASTParser; +import org.eclipse.jdt.core.dom.ASTVisitor; +import org.eclipse.jdt.core.dom.ClassInstanceCreation; +import org.eclipse.jdt.core.dom.CompilationUnit; +import org.eclipse.jdt.core.dom.FieldDeclaration; +import org.eclipse.jdt.core.dom.MethodInvocation; +import org.eclipse.jdt.core.dom.SimpleName; +import org.eclipse.jdt.core.dom.VariableDeclarationFragment; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import java.util.TreeMap; +import java.util.regex.Pattern; +import java.util.stream.Stream; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The rules of docs/SYSTEMS.md that the build checks, so a new listener list, thread or watcher is found here rather than + * in a review. Each file that breaks a rule today is listed with how often and why; a new one fails, and so does a listed + * one that no longer breaks it, so the lists only shrink. Adding to a list is a decision recorded in docs/SYSTEMS.md. + */ +class SystemsRulesTest { + private static final Path SOURCES = Path.of("src/main/java/com/github/minecraft_ta/totalDebugCompanion"); + private static final Pattern LISTENER_COLLECTION = Pattern.compile( + "(List|Set|Collection|Map)<.*\\b(Runnable|Consumer|BiConsumer|IntConsumer|[A-Z]\\w*Listener)\\b.*>"); + private static final Set THREAD_TYPES = Set.of("Thread", "ThreadPoolExecutor", "ScheduledThreadPoolExecutor", "ForkJoinPool"); + + /** What a file does against a rule, and how often. */ + private record Found(Map threads, Map sharedPool, Map listenerLists, + Map watchers) { + } + + private static Found found; + + /** A file allowed to break a rule this many times, and why. */ + private record Allowed(int times, String why) { + } + + // Counted per call: an executor made with a thread factory counts twice. Services that own a thread for a reason of their + // own (section 4), and those that move onto Workers. + private static final Map THREADS = Map.ofEntries( + Map.entry("CompanionApplication.java", new Allowed(4, "project switching and the MCP lifecycle")), + Map.entry("debugger/DebuggerSessionQueue.java", new Allowed(2, "the debugger")), + Map.entry("debugger/expression/DebuggerEvaluationRunner.java", new Allowed(1, "the debugger")), + Map.entry("ui/components/global/EditorTabs.java", new Allowed(2, "the editor's Java analysis")), + Map.entry("script/ScriptCompilationService.java", new Allowed(2, "script compilation")), + Map.entry("decompile/CompanionDecompilationService.java", new Allowed(3, "decompilation")), + Map.entry("search/SearchManager.java", new Allowed(2, "search")), + Map.entry("search/reference/ReferenceSearchService.java", new Allowed(2, "search")), + Map.entry("search/insight/CodeInsightService.java", new Allowed(2, "search")), + Map.entry("ui/views/SearchEverywherePopup.java", new Allowed(2, "search")), + Map.entry("runtime/RuntimeIndexService.java", new Allowed(2, "the runtime index")), + Map.entry("mcp/CodeModeJobService.java", new Allowed(2, "the MCP job service")), + Map.entry("session/ProjectSelectionServer.java", new Allowed(2, "accepts connections")), + Map.entry("inspection/ItemIconService.java", new Allowed(2, "the item icon renderer is confined to one thread")), + Map.entry("catalog/ConfigChanges.java", new Allowed(2, "the project's write queue, which the pipeline takes over in PR 3")), + Map.entry("storage/JsonStateWriter.java", new Allowed(2, "moves onto Workers in PR 3")), + Map.entry("ui/components/catalog/TextureThumbnails.java", new Allowed(2, "moves onto Workers in PR 3")), + Map.entry("ui/components/catalog/ModLogoIcons.java", new Allowed(2, "moves onto Workers in PR 3")), + Map.entry("ui/components/editors/ResourceViewPanel.java", new Allowed(2, "moves onto Workers in PR 3")), + Map.entry("catalog/KeyAssignments.java", new Allowed(3, "becomes a file reading in PR 4")), + Map.entry("pack/ExternalEdits.java", new Allowed(3, "becomes an adoption in PR 5")), + Map.entry("util/FileUtils.java", new Allowed(1, "its watcher becomes FileWatch in PR 4"))); + + // All move onto Workers' file work in PR 3. + private static final Map SHARED_POOL = Map.ofEntries( + Map.entry("CompanionApplication.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("inspection/InspectionSession.java", new Allowed(2, "moves onto Workers in PR 3")), + Map.entry("model/CodeView.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("navigation/NavigationService.java", new Allowed(2, "moves onto Workers in PR 3")), + Map.entry("pack/ExternalEdits.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("script/SnippetExpressionSupport.java", new Allowed(2, "moves onto Workers in PR 3")), + Map.entry("ui/components/PageLoader.java", new Allowed(1, "reads on Workers' file work from PR 3")), + Map.entry("ui/components/catalog/ModPanel.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("ui/components/editors/PackResourceEditor.java", new Allowed(2, "its saves move onto Workers in PR 3")), + Map.entry("ui/components/editors/ResourceTextEditor.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("ui/components/editors/ScriptPanel.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("ui/components/global/NotificationWidget.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("ui/components/global/ProjectSelector.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("ui/components/treeView/FileTreeView.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("ui/components/treeView/ScriptFileActions.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("ui/components/treeView/lazyFileTree/LazyFileJTree.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("ui/views/PrismInstancePicker.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("ui/views/debugger/BreakpointsWindow.java", new Allowed(2, "moves onto Workers in PR 3")), + Map.entry("ui/views/debugger/DebuggerInspector.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("util/FileUtils.java", new Allowed(1, "moves onto Workers in PR 3"))); + + // Signal itself; events inside a subsystem or a control, which are not state (section 1); owners not on signals yet. + private static final Map LISTENER_LISTS = Map.ofEntries( + Map.entry("util/Signal.java", new Allowed(1, "the signal every owner uses")), + Map.entry("jdt/diagnostics/ASTCache.java", new Allowed(1, "the editor's analysis")), + Map.entry("notification/NotificationCenter.java", new Allowed(1, "notifications, an event")), + Map.entry("script/EditorScriptRunService.java", new Allowed(2, "script runs, an event")), + Map.entry("session/CompanionSession.java", new Allowed(1, "script results, an event")), + Map.entry("search/SearchManager.java", new Allowed(2, "search matches, an event")), + Map.entry("ui/components/SegmentedToggle.java", new Allowed(1, "a control's choice, an event")), + Map.entry("ui/components/global/EditorTabs.java", new Allowed(1, "the tab chosen, an event")), + Map.entry("ui/components/inspection/DataView.java", new Allowed(1, "speed search, an event")), + Map.entry("ui/components/treeView/lazyFileTree/LazyFileJTree.java", new Allowed(1, "a double click, an event")), + Map.entry("ui/theme/ThemeManager.java", new Allowed(1, "the theme, which stays as it is")), + Map.entry("game/GameLocation.java", new Allowed(1, "moves onto signals in PR 3")), + Map.entry("inspection/ItemIconService.java", new Allowed(1, "moves onto signals in PR 3")), + Map.entry("catalog/WorldReadings.java", new Allowed(1, "replaced by CurrentWorld in PR 4")), + Map.entry("util/FileUtils.java", new Allowed(1, "becomes FileWatch in PR 4")), + Map.entry("pack/ExternalEdits.java", new Allowed(1, "becomes an adoption in PR 5"))); + + private static final Map WATCHERS = Map.ofEntries( + Map.entry("util/FileUtils.java", new Allowed(1, "becomes FileWatch in PR 4")), + Map.entry("catalog/KeyAssignments.java", new Allowed(1, "becomes a file reading in PR 4")), + Map.entry("pack/ExternalEdits.java", new Allowed(1, "becomes an adoption in PR 5"))); + + @BeforeAll + static void scan() throws IOException { + Map threads = new TreeMap<>(); + Map sharedPool = new TreeMap<>(); + Map listenerLists = new TreeMap<>(); + Map watchers = new TreeMap<>(); + List files; + try (Stream walk = Files.walk(SOURCES)) { + files = walk.filter(path -> path.toString().endsWith(".java")).sorted().toList(); + } + for (Path file : files) { + String name = SOURCES.relativize(file).toString().replace('\\', '/'); + parse(file).accept(new ASTVisitor() { + @Override + public boolean visit(MethodInvocation call) { + String method = call.getName().getIdentifier(); + String target = call.getExpression() instanceof SimpleName simple ? simple.getIdentifier() : ""; + if (target.equals("Executors") && method.startsWith("new") + || target.equals("Thread") && (method.equals("ofPlatform") || method.equals("ofVirtual"))) { + threads.merge(name, 1, Integer::sum); + } + if ((method.equals("supplyAsync") || method.equals("runAsync")) && call.arguments().size() == 1) { + sharedPool.merge(name, 1, Integer::sum); + } + if (method.equals("newWatchService")) watchers.merge(name, 1, Integer::sum); + return true; + } + + @Override + public boolean visit(ClassInstanceCreation creation) { + if (THREAD_TYPES.contains(creation.getType().toString())) threads.merge(name, 1, Integer::sum); + return true; + } + + @Override + public boolean visit(FieldDeclaration field) { + if (!LISTENER_COLLECTION.matcher(field.getType().toString()).matches()) return true; + for (Object fragment : field.fragments()) { + // A collection of callbacks kept to be told, not of what removes subscriptions. + String variable = ((VariableDeclarationFragment) fragment).getName().getIdentifier(); + if (variable.toLowerCase(Locale.ROOT).contains("listener")) listenerLists.merge(name, 1, Integer::sum); + } + return true; + } + }); + } + found = new Found(threads, sharedPool, listenerLists, watchers); + } + + private static CompilationUnit parse(Path file) throws IOException { + ASTParser parser = ASTParser.newParser(AST.getJLSLatest()); + parser.setKind(ASTParser.K_COMPILATION_UNIT); + Map options = JavaCore.getOptions(); + JavaCore.setComplianceOptions(JavaCore.VERSION_21, options); + parser.setCompilerOptions(options); + parser.setSource(Files.readString(file, StandardCharsets.UTF_8).toCharArray()); + return (CompilationUnit) parser.createAST(null); + } + + @Test + void threadsAndExecutorsComeFromTheirOwners() { + check("creates a thread or executor", found.threads(), THREADS); + } + + @Test + void workRunsOnANamedWorkerNotTheSharedPool() { + check("runs supplyAsync or runAsync on the shared pool", found.sharedPool(), SHARED_POOL); + } + + @Test + void stateIsFollowedThroughSignals() { + check("keeps a list of listeners", found.listenerLists(), LISTENER_LISTS); + } + + @Test + void filesAreWatchedInOnePlace() { + check("watches files", found.watchers(), WATCHERS); + } + + private static void check(String breaks, Map actual, Map allowed) { + List problems = new ArrayList<>(); + actual.forEach((file, times) -> { + Allowed exception = allowed.get(file); + if (exception == null) problems.add(file + " " + breaks + " " + times + " times; use the system of docs/SYSTEMS.md"); + else if (times > exception.times()) problems.add(file + " " + breaks + " " + times + " times, " + exception.times() + " allowed"); + }); + allowed.forEach((file, exception) -> { + int times = actual.getOrDefault(file, 0); + if (times < exception.times()) problems.add(file + " " + breaks + " " + times + " times now, not " + exception.times() + + "; lower its exception, or remove it at 0"); + }); + assertTrue(problems.isEmpty(), () -> String.join("\n", problems)); + } +} diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/ConfigChangesTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/ConfigChangesTest.java index 2fdb2c19d..058f0e339 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/ConfigChangesTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/ConfigChangesTest.java @@ -1,5 +1,7 @@ package com.github.minecraft_ta.totalDebugCompanion.catalog; +import java.util.ArrayList; +import org.junit.jupiter.api.AfterEach; import com.github.minecraft_ta.totalDebugCompanion.change.ChangePipeline; import com.github.minecraft_ta.totalDebugCompanion.change.Effect; import com.github.minecraft_ta.totalDebugCompanion.game.GameLocation; @@ -31,6 +33,20 @@ class ConfigChangesTest { @TempDir Path directory; + + private final List assignments = new ArrayList<>(); + + @AfterEach + void closeAssignments() { + this.assignments.forEach(KeyAssignments::close); + } + + /** The key assignments of the test's {@code options.txt}, watched until the test ends. */ + private KeyAssignments assignments() { + KeyAssignments assignments = new KeyAssignments(this.directory.resolve("options.txt")); + this.assignments.add(assignments); + return assignments; + } private GameLocation location; @BeforeEach @@ -99,7 +115,7 @@ void anOfflineKeyChangeTakenBeforeClosingIsWrittenAndRecorded() throws Exception Files.writeString(options, "key_key.jump:key.keyboard.space\n"); ChangeRecord record = ChangeRecord.inMemory(); ConfigChanges changes = new ConfigChanges(this.location, record); - KeyBindingControl keys = new KeyBindingControl(new ChangePipeline(this.location, record, changes.writes()), listener -> () -> { }); + KeyBindingControl keys = new KeyBindingControl(new ChangePipeline(this.location, record, changes.writes()), assignments()); CountDownLatch release = new CountDownLatch(1); changes.write(() -> { try { diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java index aeb291ccb..c454f51f2 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java @@ -21,7 +21,7 @@ void aKeyReboundIsToldAndAnotherOptionIsNot() throws Exception { Files.writeString(options, "soundCategory_master:1.0\nkey_key.jump:key.keyboard.space\n"); AtomicInteger told = new AtomicInteger(); try (KeyAssignments assignments = new KeyAssignments(options)) { - assignments.addListener(told::incrementAndGet); + assignments.changed().subscribe(told::incrementAndGet); // The first read only learns what the file assigns. Thread.sleep(500); @@ -42,7 +42,7 @@ void aKeyReboundIsToldAndAnotherOptionIsNot() throws Exception { @Test void aGameFolderThatCannotBeWatchedTellsNothing() throws Exception { try (KeyAssignments assignments = new KeyAssignments(this.directory.resolve("missing/options.txt"))) { - assignments.addListener(() -> { }); + assignments.changed().subscribe(() -> { }); } } diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControlTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControlTest.java index b4d895292..047d8bdc1 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControlTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControlTest.java @@ -1,5 +1,6 @@ package com.github.minecraft_ta.totalDebugCompanion.catalog; +import org.junit.jupiter.api.AfterEach; import com.github.minecraft_ta.totalDebugCompanion.change.ChangePipeline; import com.github.minecraft_ta.totalDebugCompanion.game.GameLocation; import com.github.minecraft_ta.totalDebugCompanion.game.GameLocations; @@ -26,6 +27,20 @@ class KeyBindingControlTest { @TempDir Path directory; + private final List assignments = new ArrayList<>(); + + @AfterEach + void closeAssignments() { + this.assignments.forEach(KeyAssignments::close); + } + + /** The key assignments of the test's {@code options.txt}, watched until the test ends. */ + private KeyAssignments assignments() { + KeyAssignments assignments = new KeyAssignments(this.directory.resolve("options.txt")); + this.assignments.add(assignments); + return assignments; + } + @Test void aClosedGameGetsItsKeysInOptions() throws Exception { Path options = this.directory.resolve("options.txt"); @@ -103,7 +118,7 @@ void bindingsChangedTogetherAllStayInOptions() throws Exception { void aRunningGameMakesTheChangeAndAnswers() throws Exception { GameLocation location = GameLocations.of(this.directory, true); ChangePipeline pipeline = new ChangePipeline(location, ChangeRecord.inMemory(), Runnable::run); - KeyBindingControl control = new KeyBindingControl(pipeline, listener -> () -> { }); + KeyBindingControl control = new KeyBindingControl(pipeline, assignments()); List sent = new ArrayList<>(); location.connected(message -> { if (message instanceof ChangeMessage change) sent.add(change.payload()); @@ -138,7 +153,7 @@ void aRunningGameMakesTheChangeAndAnswers() throws Exception { void anAnswerAfterTheCallerStoppedWaitingIsStillRecorded() throws Exception { GameLocation location = GameLocations.of(this.directory, true); ChangePipeline pipeline = new ChangePipeline(location, ChangeRecord.inMemory(), Runnable::run); - KeyBindingControl control = new KeyBindingControl(pipeline, listener -> () -> { }); + KeyBindingControl control = new KeyBindingControl(pipeline, assignments()); List sent = new ArrayList<>(); location.connected(message -> { if (message instanceof ChangeMessage change) sent.add(change.payload()); @@ -159,8 +174,8 @@ void keysAreWrittenTheWayOptionsWritesThem() { assertEquals(new KeyBindings.Assignment("key.keyboard.e", "SHIFT"), KeyBindings.Assignment.decode("key.keyboard.e:SHIFT")); } - private static KeyBindingControl control(GameLocation location) { - return new KeyBindingControl(new ChangePipeline(location, ChangeRecord.inMemory(), Runnable::run), listener -> () -> { }); + private KeyBindingControl control(GameLocation location) { + return new KeyBindingControl(new ChangePipeline(location, ChangeRecord.inMemory(), Runnable::run), assignments()); } private static String set(KeyBindingControl control, KeyBindingControl.Change... changes) throws Exception { diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/PackCatalogServiceTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/PackCatalogServiceTest.java index fd1f97dc9..a6457f222 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/PackCatalogServiceTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/PackCatalogServiceTest.java @@ -30,7 +30,7 @@ void restoresTheSavedCatalogOfTheSavedRuntime() throws Exception { CatalogFixtures.catalog(jar).write(paths.catalog()); PackCatalogService service = new PackCatalogService(paths); AtomicInteger changes = new AtomicInteger(); - service.addListener(changes::incrementAndGet); + service.changed().subscribe(changes::incrementAndGet); service.restore(); SwingUtilities.invokeAndWait(() -> { }); @@ -51,7 +51,7 @@ void aCatalogCapturedAgainStaysShownUntilTheNewOneIsReady() throws Exception { SwingUtilities.invokeAndWait(() -> { }); CatalogIndex shown = service.index().orElseThrow(); AtomicInteger changes = new AtomicInteger(); - service.addListener(changes::incrementAndGet); + service.changed().subscribe(changes::incrementAndGet); // As after every resource reload: Minecraft captures the catalog again. service.capturing(); diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/navigation/PageReadsTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/navigation/PageReadsTest.java new file mode 100644 index 000000000..b031d3ac5 --- /dev/null +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/navigation/PageReadsTest.java @@ -0,0 +1,90 @@ +package com.github.minecraft_ta.totalDebugCompanion.navigation; + +import com.github.minecraft_ta.totalDebugCompanion.CompanionApplication; +import com.github.minecraft_ta.totalDebugCompanion.session.CompanionLaunchConfiguration; +import com.github.minecraft_ta.totalDebugCompanion.GlobalConfig; +import com.github.minecraft_ta.totalDebugCompanion.catalog.CatalogFixtures; +import com.github.minecraft_ta.totalDebugCompanion.model.KeyBindingsView; +import com.github.minecraft_ta.totalDebugCompanion.session.CompanionProfile; +import com.github.minecraft_ta.totalDebugCompanion.project.ProjectScope; +import com.github.minecraft_ta.totalDebugCompanion.testui.UiTest; +import com.github.minecraft_ta.totalDebugCompanion.testui.UiTestScope; +import com.github.minecraft_ta.totalDebugCompanion.ui.components.catalog.KeyBindingsPanel; +import com.github.minecraft_ta.totalDebugCompanion.ui.views.MainWindow; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import javax.swing.SwingUtilities; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.concurrent.TimeUnit; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; + +/** + * Pages opened the way a click opens them read once, and again only after a change they follow (docs/SYSTEMS.md, Tests): + * built by the application, opened through navigation, shown in the window. + */ +@UiTest +class PageReadsTest { + @TempDir Path directory; + + @Test + void theKeyBindingsPageReadsOnceWhenOpenedAndAgainOnlyAfterAChangeItMissed() throws Exception { + Path home = Files.createDirectory(this.directory.resolve("home")); + GlobalConfig.getInstance().loadFrom(home); + Path game = Files.createDirectory(this.directory.resolve("game")); + Files.writeString(game.resolve("options.txt"), "key_key.drop:key.keyboard.q\n"); + try (CompanionApplication app = new CompanionApplication(new CompanionLaunchConfiguration(home), "test-token")) { + app.openProject(CompanionProfile.forGame(game)).get(10, TimeUnit.SECONDS); + ProjectScope scope = app.currentScope(); + CatalogFixtures.catalog(CatalogFixtures.modJar(this.directory)).write(scope.paths().catalog()); + scope.catalog().accept(CatalogFixtures.INVENTORY, scope.paths().catalog(), Runnable::run); + MainWindow window = UiTestScope.onEdt(app::createWindow); + UiTestScope.onEdt(() -> { + window.setSize(1280, 720); + UiTestScope.show(window); + }); + + open(window, new NavigationTarget.KeyBindings("")); + KeyBindingsPanel panel = UiTestScope.onEdt(() -> (KeyBindingsPanel) assertInstanceOf(KeyBindingsView.class, + window.getEditorTabs().getSelectedEditor()).getComponent()); + UiTestScope.await(() -> panel.reads() == 1); + settle(); + assertEquals(1, reads(panel), "opening the page reads it once"); + + open(window, new NavigationTarget.KeyBindings("key.drop")); + assertEquals(1, reads(panel), "navigating to the page it shows reads nothing"); + + open(window, new NavigationTarget.Changes()); + scope.keyBindings().assignmentsChanged().fire(); + settle(); + assertEquals(1, reads(panel), "a hidden page does not read"); + open(window, new NavigationTarget.KeyBindings("")); + UiTestScope.await(() -> panel.reads() == 2); + settle(); + assertEquals(2, reads(panel), "shown again, it reads the change it missed once"); + + open(window, new NavigationTarget.Changes()); + open(window, new NavigationTarget.KeyBindings("")); + assertEquals(2, reads(panel), "shown again without a change, it reads nothing"); + } + } + + private static void open(MainWindow window, NavigationTarget target) throws Exception { + window.navigation().navigate(target, NavigationService.Activation.KEEP_CURRENT_WINDOW).get(5, TimeUnit.SECONDS); + settle(); + } + + private static int reads(KeyBindingsPanel panel) throws Exception { + return UiTestScope.onEdt(panel::reads); + } + + /** Lets the Swing steps queued by showing and reading run, and a read they started finish. */ + private static void settle() throws Exception { + for (int step = 0; step < 3; step++) SwingUtilities.invokeAndWait(() -> { }); + Thread.sleep(200); + SwingUtilities.invokeAndWait(() -> { }); + } +} diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/GamePacksTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/GamePacksTest.java index 18d423473..f8b1c3d19 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/GamePacksTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/GamePacksTest.java @@ -61,8 +61,8 @@ void aFileIsSuppliedByTheSideOfItsFolder() { } private GamePacks follow(GamePacks packs) { - packs.addResourcePackListener(this.resources::incrementAndGet); - packs.addDatapackListener(this.data::incrementAndGet); + packs.changed(ChangeRecord.PackSide.RESOURCES).subscribe(this.resources::incrementAndGet); + packs.changed(ChangeRecord.PackSide.DATA).subscribe(this.data::incrementAndGet); return packs; } diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/PackSelectionsTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/PackSelectionsTest.java index 3b19ff2e4..f410c76f5 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/PackSelectionsTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/PackSelectionsTest.java @@ -55,7 +55,7 @@ void aClosedGamesResourcePacksAreWrittenToOptionsAndRevertedFromThere() throws E ResourceEdits edits = edits(record, false); PackSelections selections = selections(record, edits); AtomicInteger told = new AtomicInteger(); - edits.packs().addResourcePackListener(told::incrementAndGet); + edits.packs().changed(ChangeRecord.PackSide.RESOURCES).subscribe(told::incrementAndGet); PackSelections.Applied applied = selections.set(ChangeRecord.PackSide.RESOURCES, null, List.of("vanilla", "file/New", "mod_resources")).get(5, TimeUnit.SECONDS); diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEditsTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEditsTest.java index e7166a81a..28f5fffa7 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEditsTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEditsTest.java @@ -118,7 +118,7 @@ void editListenersHearOfASaveOnceTheOptionsEnableThePack() throws Exception { ResourceEdits edits = edits(ChangeRecord.inMemory()); edits.packs().named(new ClientPacksPayload(STACK, 48)); List seen = new CopyOnWriteArrayList<>(); - edits.addEditListener(() -> { + edits.edited().subscribe(() -> { try { seen.add(Files.readString(options)); } catch (IOException unreadable) { @@ -126,7 +126,7 @@ void editListenersHearOfASaveOnceTheOptionsEnableThePack() throws Exception { } }); AtomicInteger resourcePacks = new AtomicInteger(); - edits.packs().addResourcePackListener(resourcePacks::incrementAndGet); + edits.packs().changed(ChangeRecord.PackSide.RESOURCES).subscribe(resourcePacks::incrementAndGet); edits.save(LANG, bytes("{}")).get(5, TimeUnit.SECONDS); assertEquals(List.of("resourcePacks:[\"vanilla\",\"file/TotalDebug\"]\n"), seen, diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/storage/ChangeRecordTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/storage/ChangeRecordTest.java index 9f871a3ad..e9c86f217 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/storage/ChangeRecordTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/storage/ChangeRecordTest.java @@ -27,7 +27,7 @@ void keepsTheOriginalValueUntilItIsWrittenBack() { ChangeRecord record = ChangeRecord.inMemory(Clock.fixed(Instant.parse("2026-09-25T12:00:00Z"), ZoneOffset.UTC)); ChangeRecord.Setting speed = setting("speed"); List events = new ArrayList<>(); - record.addListener(() -> events.add("changed")); + record.changed().subscribe(() -> events.add("changed")); record.changed(speed, "9", "12"); record.changed(speed, "12", "14"); @@ -48,7 +48,7 @@ void keepsTheOriginalValueUntilItIsWrittenBack() { void aWriteThatLeavesTheRecordAsItWasTellsNobody() { ChangeRecord record = ChangeRecord.inMemory(); AtomicInteger told = new AtomicInteger(); - record.addListener(told::incrementAndGet); + record.changed().subscribe(told::incrementAndGet); ChangeRecord.Setting speed = setting("speed"); record.changed(speed, "9", "9"); diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/testui/UiTestScope.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/testui/UiTestScope.java index 0244a43b9..d513163f4 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/testui/UiTestScope.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/testui/UiTestScope.java @@ -1,5 +1,6 @@ package com.github.minecraft_ta.totalDebugCompanion.testui; +import java.awt.GridLayout; import com.formdev.flatlaf.FlatLaf; import com.github.minecraft_ta.totalDebugCompanion.ui.theme.CompanionTheme; import com.github.minecraft_ta.totalDebugCompanion.ui.theme.ThemeManager; @@ -120,6 +121,20 @@ public static void show(Window window) { window.setVisible(true); } + /** + * Shows {@code pages} side by side in a window of their own, as open tabs show them, offscreen and unfocusable; the + * scope disposes it. Pages read when they are shown (docs/SYSTEMS.md, section 3). + */ + public static JFrame showPages(Component... pages) { + requireEdt(); + JFrame window = new JFrame(); + window.setLayout(new GridLayout(1, pages.length)); + for (Component page : pages) window.add(page); + window.setSize(800 * pages.length, 600); + show(window); + return window; + } + /** Used by previews in both interactive and offscreen modes. */ public static void place(Window window, Window owner, int x, int y) { requireEdt(); diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java index 5a8aa462d..856f5d16c 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java @@ -1,5 +1,7 @@ package com.github.minecraft_ta.totalDebugCompanion.ui.components; +import java.util.concurrent.atomic.AtomicBoolean; +import com.github.minecraft_ta.totalDebugCompanion.util.Signal; import org.junit.jupiter.api.Test; import javax.swing.SwingUtilities; @@ -173,6 +175,102 @@ void aLoaderWithoutAPageReadsEveryChange() throws Exception { assertEquals(1, this.prepared.get()); } + @Test + void aPageReadsWhenFirstShownAndThenOnlyAfterItsSignals() throws Exception { + ShowablePage page = new ShowablePage(); + Signal signal = new Signal(); + PageLoader loader = onEdt(() -> loader().page(page).follows(signal)); + + assertEquals(0, this.prepared.get(), "a page does not read before it is shown"); + show(page, loader, true); + assertEquals(1, this.prepared.get(), "shown, it reads"); + show(page, loader, false); + show(page, loader, true); + assertEquals(1, this.prepared.get(), "shown again without a change, it reads nothing"); + + show(page, loader, false); + fire(signal); + fire(signal); + assertEquals(1, this.prepared.get(), "a hidden page does not read"); + show(page, loader, true); + assertEquals(2, this.prepared.get(), "shown again, it reads what it missed once"); + + fire(signal); + settle(loader); + assertEquals(3, this.prepared.get(), "a shown page reads a change at once"); + } + + @Test + void aChangeThatDoesNotConcernThePageReadsNothing() throws Exception { + ShowablePage page = new ShowablePage(); + Signal signal = new Signal(); + AtomicBoolean concerns = new AtomicBoolean(); + PageLoader loader = onEdt(() -> loader().page(page).follows(signal, concerns::get)); + show(page, loader, true); + + fire(signal); + settle(loader); + assertEquals(1, this.prepared.get(), "another file's change, say, leaves the page alone"); + concerns.set(true); + fire(signal); + settle(loader); + assertEquals(2, this.prepared.get()); + } + + @Test + void aHeldPageReadsWhatItMissedOnceReleased() throws Exception { + ShowablePage page = new ShowablePage(); + Signal signal = new Signal(); + AtomicBoolean concerns = new AtomicBoolean(true); + PageLoader loader = onEdt(() -> loader().page(page).follows(signal, concerns::get)); + show(page, loader, true); + + SwingUtilities.invokeAndWait(loader::hold); + fire(signal); + SwingUtilities.invokeAndWait(loader::load); + assertEquals(1, this.prepared.get(), "a page holding its reads, as while it saves, reads nothing"); + SwingUtilities.invokeAndWait(loader::release); + settle(loader); + assertEquals(2, this.prepared.get(), "released, it reads what it missed once"); + + // The change came from the page's own save: by the time it is released, the change does not concern it. + SwingUtilities.invokeAndWait(loader::hold); + fire(signal); + concerns.set(false); + SwingUtilities.invokeAndWait(loader::release); + settle(loader); + assertEquals(2, this.prepared.get(), "whether a change concerns the page is asked when it would read"); + } + + @Test + void aReadUnderWayWhenThePageHoldsIsNotShownAndIsReadAgain() throws Exception { + CountDownLatch release = new CountDownLatch(1); + AtomicInteger reads = new AtomicInteger(); + List shown = new CopyOnWriteArrayList<>(); + PageLoader loader = new PageLoader<>(() -> () -> { + int read = reads.incrementAndGet(); + if (read == 1) release.await(5, TimeUnit.SECONDS); + return read; + }, shown::add, failure -> { }); + + SwingUtilities.invokeAndWait(() -> { + loader.load(); + loader.hold(); + }); + release.countDown(); + loader.current().get(5, TimeUnit.SECONDS); + SwingUtilities.invokeAndWait(() -> { }); + assertEquals(List.of(), shown, "a read that may predate the write is not shown"); + SwingUtilities.invokeAndWait(loader::release); + settle(loader); + assertEquals(List.of(2), shown, "what it was for is read again after the write"); + } + + private static void fire(Signal signal) throws Exception { + signal.fire(); + SwingUtilities.invokeAndWait(() -> { }); + } + private PageLoader loader() { return new PageLoader<>(() -> { this.prepared.incrementAndGet(); diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/CatalogPanelsTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/CatalogPanelsTest.java index f8fa3673b..f88bbc195 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/CatalogPanelsTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/CatalogPanelsTest.java @@ -1,5 +1,9 @@ package com.github.minecraft_ta.totalDebugCompanion.ui.components.catalog; +import com.github.minecraft_ta.totalDebugCompanion.testui.UiTestScope; +import com.github.minecraft_ta.totalDebugCompanion.testui.UiTest; +import org.junit.jupiter.api.AfterEach; +import com.github.minecraft_ta.totalDebugCompanion.catalog.KeyAssignments; import com.github.minecraft_ta.totalDebugCompanion.catalog.CatalogFixtures; import com.github.minecraft_ta.totalDebugCompanion.catalog.ConfigSettingsFixture; import com.github.minecraft_ta.totalDebugCompanion.catalog.KeyBindingControl; @@ -49,6 +53,20 @@ class CatalogPanelsTest { @TempDir Path directory; + private final List assignments = new ArrayList<>(); + + @AfterEach + void closeAssignments() { + this.assignments.forEach(KeyAssignments::close); + } + + /** The key assignments of the test's {@code options.txt}, watched until the test ends. */ + private KeyAssignments assignments() { + KeyAssignments assignments = new KeyAssignments(this.directory.resolve("options.txt")); + this.assignments.add(assignments); + return assignments; + } + @Test void wideModLogosStayReadableAndFitInsideTheirHeader() throws Exception { BufferedImage banner = new BufferedImage(600, 240, BufferedImage.TYPE_INT_ARGB); @@ -99,7 +117,7 @@ void aModPageListsWhatTheModRegisteredAndOpensDefinitions() throws Exception { try (ItemIconService icons = new ItemIconService()) { onEdt(() -> { ModPanel panel = new ModPanel("testmod", catalog, RuntimeSourceCatalog::empty, icons, this.directory, ConfigSettingsFixture.of(GameLocations.of(this.directory, false), ChangeRecord.inMemory()), - new KeyBindingControl(new ChangePipeline(GameLocations.of(this.directory, false), ChangeRecord.inMemory(), Runnable::run), listener -> () -> { }), opened::add, listener -> () -> { }); + new KeyBindingControl(new ChangePipeline(GameLocations.of(this.directory, false), ChangeRecord.inMemory(), Runnable::run), assignments()), opened::add, listener -> () -> { }); try { assertEquals("Test Mod", panel.title()); assertTrue(labels(panel).contains("1.2.3"), labels(panel)::toString); @@ -135,7 +153,7 @@ void aModOverviewLinksInstalledDependencies() throws Exception { try (ItemIconService icons = new ItemIconService()) { onEdt(() -> { ModPanel panel = new ModPanel("testmod", catalog, RuntimeSourceCatalog::empty, icons, this.directory, ConfigSettingsFixture.of(GameLocations.of(this.directory, false), ChangeRecord.inMemory()), - new KeyBindingControl(new ChangePipeline(GameLocations.of(this.directory, false), ChangeRecord.inMemory(), Runnable::run), listener -> () -> { }), target -> { }, listener -> () -> { }); + new KeyBindingControl(new ChangePipeline(GameLocations.of(this.directory, false), ChangeRecord.inMemory(), Runnable::run), assignments()), target -> { }, listener -> () -> { }); try { List sections = panel.sections(catalog.index().orElseThrow().mod("testmod").orElseThrow()); assertEquals(List.of("Mod", "Dependencies"), sections.stream().map(FactSection::title).toList()); @@ -156,7 +174,7 @@ void anUnknownModSaysSo() throws Exception { try (ItemIconService icons = new ItemIconService()) { onEdt(() -> { ModPanel panel = new ModPanel("absent", catalog, RuntimeSourceCatalog::empty, icons, this.directory, ConfigSettingsFixture.of(GameLocations.of(this.directory, false), ChangeRecord.inMemory()), - new KeyBindingControl(new ChangePipeline(GameLocations.of(this.directory, false), ChangeRecord.inMemory(), Runnable::run), listener -> () -> { }), target -> { }, listener -> () -> { }); + new KeyBindingControl(new ChangePipeline(GameLocations.of(this.directory, false), ChangeRecord.inMemory(), Runnable::run), assignments()), target -> { }, listener -> () -> { }); try { assertTrue(labels(panel).contains("absent is not an installed mod"), labels(panel)::toString); assertEquals(1, panel.tabs().getTabCount(), "Only the Overview has something to show"); @@ -236,16 +254,18 @@ void aHiddenDefinitionPageNamesItsTabFromTheCatalogCapturedSince() throws Except } @Test + @UiTest void keyBindingsKeepTheirSelectionWhenTheirKeysAreReadAgain() throws Exception { PackCatalogService catalog = readyCatalog(); Files.writeString(this.directory.resolve("options.txt"), "key_key.drop:key.keyboard.q\n"); KeyBindingControl control = new KeyBindingControl(new ChangePipeline(GameLocations.of(this.directory, false), - ChangeRecord.inMemory(), Runnable::run), listener -> () -> { }); + ChangeRecord.inMemory(), Runnable::run), assignments()); KeyBindingsPanel[] panel = new KeyBindingsPanel[1]; onEdt(() -> panel[0] = new KeyBindingsPanel(catalog, control, "", target -> { })); try { - panel[0].loading().get(10, TimeUnit.SECONDS); - SwingUtilities.invokeAndWait(() -> { }); + assertEquals(0, (int) UiTestScope.onEdt(panel[0]::reads), "a page does not read before it is shown"); + onEdt(() -> UiTestScope.showPages(panel[0])); + UiTestScope.await(() -> panel[0].table().getRowCount() > 0); onEdt(() -> { JTable table = panel[0].table(); int drop = -1; @@ -256,16 +276,16 @@ void keyBindingsKeepTheirSelectionWhenTheirKeysAreReadAgain() throws Exception { table.setRowSelectionInterval(drop, drop); }); - // As when the game saved a key rebound in its controls screen. + // As when the game saved a key rebound in its controls screen: the watch of options.txt tells the page. Files.writeString(this.directory.resolve("options.txt"), "key_key.drop:key.keyboard.g\n"); - onEdt(panel[0]::load); - panel[0].loading().get(10, TimeUnit.SECONDS); + UiTestScope.await(() -> panel[0].reads() == 2 && panel[0].loading().isDone()); SwingUtilities.invokeAndWait(() -> { }); onEdt(() -> { JTable table = panel[0].table(); assertEquals(1, table.getSelectedRowCount()); assertTrue(String.valueOf(table.getValueAt(table.getSelectedRow(), 0)).contains("Drop Selected Item"), "the binding stays selected when its keys are read again"); + assertTrue(String.valueOf(table.getValueAt(table.getSelectedRow(), 1)).contains("G"), "and shows its new key"); }); } finally { onEdt(() -> panel[0].dispose()); diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ChangesPanelTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ChangesPanelTest.java index ff833987e..0b77ef6e0 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ChangesPanelTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/ChangesPanelTest.java @@ -1,5 +1,7 @@ package com.github.minecraft_ta.totalDebugCompanion.ui.components.catalog; +import org.junit.jupiter.api.AfterEach; +import com.github.minecraft_ta.totalDebugCompanion.catalog.KeyAssignments; import com.github.minecraft_ta.totalDebugCompanion.catalog.ConfigLabels; import com.github.minecraft_ta.totalDebugCompanion.catalog.ConfigSettings; import com.github.minecraft_ta.totalDebugCompanion.catalog.ConfigSettingsFixture; @@ -43,6 +45,20 @@ class ChangesPanelTest { @TempDir Path directory; + private final List assignments = new ArrayList<>(); + + @AfterEach + void closeAssignments() { + this.assignments.forEach(KeyAssignments::close); + } + + /** The key assignments of the test's {@code options.txt}, watched until the test ends. */ + private KeyAssignments assignments() { + KeyAssignments assignments = new KeyAssignments(this.directory.resolve("options.txt")); + this.assignments.add(assignments); + return assignments; + } + @Test void listsRecordedChangesUntilTheFileHoldsTheOriginalAgain() throws Exception { Path jar = CatalogFixtures.modJar(this.directory); @@ -63,7 +79,7 @@ void listsRecordedChangesUntilTheFileHoldsTheOriginalAgain() throws Exception { Runnable::run, InstanceState.inMemory()); ConfigSettings settings = ConfigSettingsFixture.of(GameLocations.of(this.directory, false), record); List labels = List.of(new ConfigLabels(settings), - new KeyBindingLabels(new KeyBindingControl(new ChangePipeline(GameLocations.of(this.directory, false), record, Runnable::run), listener -> () -> { })), + new KeyBindingLabels(new KeyBindingControl(new ChangePipeline(GameLocations.of(this.directory, false), record, Runnable::run), assignments())), new ResourceLabels(edits), new PackLabels(new PackSelections(edits))); SwingUtilities.invokeAndWait(() -> panel[0] = new ChangesPanel(catalog, record, labels, target -> { })); try { diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceTextEditorTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceTextEditorTest.java index 385dfaf1d..3094a5e00 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceTextEditorTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceTextEditorTest.java @@ -1,5 +1,7 @@ package com.github.minecraft_ta.totalDebugCompanion.ui.components.editors; +import com.github.minecraft_ta.totalDebugCompanion.testui.UiTestScope; +import com.github.minecraft_ta.totalDebugCompanion.testui.UiTest; import com.github.minecraft_ta.totalDebugCompanion.change.ChangePipeline; import com.github.minecraft_ta.totalDebugCompanion.game.GameLocations; import com.github.minecraft_ta.totalDebugCompanion.storage.InstanceState; @@ -27,6 +29,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; +@UiTest class ResourceTextEditorTest { private static final String LANG = "assets/testmod/lang/en_us.json"; @@ -46,6 +49,7 @@ void aRevertElsewhereShowsTheOpenedFileAgain() throws Exception { ResourceTextEditor[] editor = new ResourceTextEditor[1]; SwingUtilities.invokeAndWait(() -> editor[0] = new ResourceTextEditor(LANG, "testmod.jar", null, new LoadedResource.Text(inJar, "text/json", "UTF-8", inJar.length()), edits)); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(editor[0])); try { awaitOnSwing(() -> editor[0].textPanel().text().equals(saved)); @@ -71,6 +75,7 @@ void aDeletedFileOfTheManagedPackStaysAsUnsavedText() throws Exception { ResourceTextEditor[] editor = new ResourceTextEditor[1]; SwingUtilities.invokeAndWait(() -> editor[0] = new ResourceTextEditor(LANG, "en_us.json", pack, new LoadedResource.Text(added, "text/json", "UTF-8", added.length()), edits)); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(editor[0])); try { edits.revert(record.changes().getFirst()).get(5, TimeUnit.SECONDS); awaitOnSwing(() -> editor[0].textPanel().modified()); @@ -100,6 +105,7 @@ void aSaveRightAfterChoosingAnotherPackGoesIntoThatPack() throws Exception { assertFalse(editor[0].textPanel().editorPane.isEditable(), "until the working pack's copy is read, typing would edit the mod's text and save it over that copy"); }); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(editor[0])); try { awaitOnSwing(() -> editor[0].targetBox().getItemCount() == 2); SwingUtilities.invokeAndWait(() -> assertTrue(editor[0].textPanel().editorPane.isEditable())); @@ -136,6 +142,7 @@ void theWarningThatTheGameDoesNotUseTheCopyEndsWhenItDoes() throws Exception { ResourceTextEditor[] editor = new ResourceTextEditor[1]; SwingUtilities.invokeAndWait(() -> editor[0] = new ResourceTextEditor(LANG, "testmod.jar", null, new LoadedResource.Text(inJar, "text/json", "UTF-8", inJar.length()), edits)); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(editor[0])); try { awaitOnSwing(() -> editor[0].noticeText().contains("not enabled")); // The player enables the pack in the game. @@ -161,6 +168,7 @@ void aSaveOverTextAnotherTabSavedSinceAsksFirst() throws Exception { new LoadedResource.Text(inJar, "text/json", "UTF-8", inJar.length()), edits); } }); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(tabs[0], tabs[1])); try { awaitOnSwing(() -> tabs[0].targetBox().getItemCount() > 0 && tabs[1].targetBox().getItemCount() > 0); List asked = new CopyOnWriteArrayList<>(); @@ -213,6 +221,7 @@ void aTabWhoseChangesAnotherTabSavedSavesItsNextChangeWithoutAsking() throws Exc new LoadedResource.Text(inJar, "text/json", "UTF-8", inJar.length()), edits); } }); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(tabs[0], tabs[1])); try { awaitOnSwing(() -> tabs[0].targetBox().getItemCount() > 0 && tabs[1].targetBox().getItemCount() > 0); List asked = new CopyOnWriteArrayList<>(); @@ -258,6 +267,7 @@ void changesCarriedToAnotherPackAreSavedThereWithoutAsking() throws Exception { ResourceTextEditor[] editor = new ResourceTextEditor[1]; SwingUtilities.invokeAndWait(() -> editor[0] = new ResourceTextEditor(LANG, "testmod.jar", null, new LoadedResource.Text(inJar, "text/json", "UTF-8", inJar.length()), edits)); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(editor[0])); try { awaitOnSwing(() -> editor[0].targetBox().getItemCount() == 2); List asked = new CopyOnWriteArrayList<>(); @@ -293,6 +303,7 @@ void aMinifiedLanguageFileIsReformattedAsOneEditThatUndoTakesBack() throws Excep editor[0].reformat(); assertEquals(inJar, editor[0].textPanel().text(), "until the working pack's copy is read, the text stays"); }); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(editor[0])); try { awaitOnSwing(() -> editor[0].targetBox().getItemCount() > 0); SwingUtilities.invokeAndWait(() -> { diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/TextureEditorTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/TextureEditorTest.java index 3eab2ed83..40dd4efb6 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/TextureEditorTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/TextureEditorTest.java @@ -1,5 +1,7 @@ package com.github.minecraft_ta.totalDebugCompanion.ui.components.editors; +import com.github.minecraft_ta.totalDebugCompanion.testui.UiTestScope; +import com.github.minecraft_ta.totalDebugCompanion.testui.UiTest; import com.github.minecraft_ta.totalDebugCompanion.change.ChangePipeline; import com.github.minecraft_ta.totalDebugCompanion.game.GameLocations; import com.github.minecraft_ta.totalDebugCompanion.pack.GamePacks; @@ -34,6 +36,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; +@UiTest class TextureEditorTest { private static final String TEXTURE = "assets/testmod/textures/item/gear.png"; private static final int GRAY = 0xFF808080; @@ -57,6 +60,7 @@ void anErasedPixelIsSavedIntoTheWorkingPackAndLaterStrokesAreDiscarded() throws assertFalse(editor[0].view().painter().paints(), "until the pack's copy is read, a stroke would be drawn on the mod's copy and saved over the pack's"); }); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(editor[0])); try { awaitOnSwing(() -> editor[0].targetBox().getItemCount() > 0); SwingUtilities.invokeAndWait(() -> { @@ -99,6 +103,7 @@ void thePencilStartsWithTheMostUsedColorAndTheColorPickerTakesAnother() throws E TextureEditor[] editor = new TextureEditor[1]; SwingUtilities.invokeAndWait(() -> editor[0] = new TextureEditor(TEXTURE, "testmod.jar", null, new LoadedResource.Image(gear, 100), edits, ignored -> { })); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(editor[0])); try { awaitOnSwing(() -> editor[0].targetBox().getItemCount() > 0); SwingUtilities.invokeAndWait(() -> { @@ -139,6 +144,7 @@ void aWorkingPackCopyTooLargeToEditIsRefusedBeforeItIsDecoded() throws Exception TextureEditor[] editor = new TextureEditor[1]; SwingUtilities.invokeAndWait(() -> editor[0] = new TextureEditor(TEXTURE, "testmod.jar", null, new LoadedResource.Image(new BufferedImage(4, 4, BufferedImage.TYPE_INT_ARGB), 100), edits, ignored -> { })); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(editor[0])); try { awaitOnSwing(() -> editor[0].noticeText().contains("2049 x 2048")); SwingUtilities.invokeAndWait(() -> { @@ -163,6 +169,7 @@ void thePacksCopyPlaysItsOwnAnimation() throws Exception { TextureEditor[] editor = new TextureEditor[1]; SwingUtilities.invokeAndWait(() -> editor[0] = new TextureEditor(TEXTURE, "testmod.jar", null, new LoadedResource.Image(new BufferedImage(4, 8, BufferedImage.TYPE_INT_ARGB), 100), edits, ignored -> { })); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(editor[0])); try { awaitOnSwing(() -> editor[0].targetBox().getItemCount() > 0); SwingUtilities.invokeAndWait(() -> assertEquals(4, editor[0].view().shownRegion().height, diff --git a/docs/SYSTEMS.md b/docs/SYSTEMS.md new file mode 100644 index 000000000..587cc9e58 --- /dev/null +++ b/docs/SYSTEMS.md @@ -0,0 +1,219 @@ +# Companion's systems + +The few systems every feature of Companion uses to learn that something changed, read it, show it, write a change and receive what the game sends, and the rules that keep features on them. Written on 2026-09-30, after the reload work (#104 to #108) and an audit showed that most edge cases of the last weeks came from each feature solving these jobs again in its own way. Revised the same day after three reviews of the draft. + +A feature adds only what is its own: how to read and write its values, and how to show them. Everything else is one of the systems below. A feature that needs something none of them offers changes the system, with a decision recorded here, instead of building beside it. + +## Why + +Companion shows state that others own: the game, and files on disk that the game, the player and Companion all write. For each piece of it, a feature has to answer the same questions: where the value comes from, how it learns of a change, on which thread it reads, what happens while its page is hidden, when two reads overlap, who else must hear of it, and which answer belongs to which request. Every feature answered them again: + +| Job | Ways it is done today | +|---|---| +| Tell that something changed | 27 `addโ€ฆListener` methods; about a dozen classes keep their own list of listeners, which run on whichever thread saw the change; some pass details, as `GameLocation.Change` | +| Know which read is the newest | A counter per class for the same read repeated: `KeyAssignments.generation`, `PageLoader`'s `again`; owners also guard between requests of different kinds, as `ItemIconService.adoptions` and `PackCatalogService.generation`, which is their job | +| Bring a page up to date | `PageLoader` in three modes (`whenShown`, `waitsWhileHidden`, `readsWhenShown`, 11 pages); `ShownUpdates` (6 pages); subscriptions pages hold themselves (`PackResourceEditor`, `TextureEditor`, `FileTreeView`, `MainWindow`, `CatalogIcons`); reads in the constructor (5 pages, and 2 that build from memory); `refresh()` on every navigation (`NavigationService`); `CompanionUi.catalogChanged` and `changesRecorded`; `InspectionSession`'s timer | +| Share what one page read | `WorldReadings` holds what the World page read last, so the World page reads while hidden for the tree and its tab | +| Notice a file changed | `FileUtils` polls a watcher every second for created and deleted files; `ExternalEdits` and `KeyAssignments` each run their own watcher and scheduler with a 300 ms settle; only the first pauses for Windows folder renames, and they key paths differently | +| Work off the Swing thread | 22 classes create their own threads or executors; most of the 47 `supplyAsync` and `runAsync` calls use the JVM's shared pool, also for blocking file reads | +| Write | The change pipeline, on a queue that `ConfigChanges` owns and names "Configuration writes"; page reads that change the change record (`ChangeRecord.observed` from the Changes page's labels) | +| Receive a game message | `CompanionSession.Listener` with one method per message, relayed by `CompanionApplication`, which checks the scope is still current; in the mod, `CompanionAppClient` has a setter per message and repeats an authentication guard, which `RetryRuntimeInventoryMessage` lacks | + +Each way was reasonable where it was added. Together they are the edge cases: a page read twice or three times when it opens, a hidden tab reading on every reload, a status bar flickering when tabs are renamed, a folder rename that one watcher pauses for and two do not, a check missing from one handler of seven. Review rounds made it worse: a finding about timing or staleness was fixed with one more flag, counter or retry where it showed, instead of in a system that owns the problem. + +## The flow + +State moves one way. A page learns of a change only through a signal, and reads only through its loader. + +```text +game message โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ” +file on disk โ”€โ–บ watch โ”€โ–บ reading โ”€โ”ผโ”€โ–บ owner compares โ”€โ–บ signal (if different) โ”€โ–บ page loader โ”€โ–บ read โ”€โ–บ show +pipeline write โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”˜ + +page action โ”€โ–บ change pipeline โ”€โ–บ project's write queue โ”€โ–บ owner โ”€โ–บ signal +``` + +## 1. Owners and signals + +Every piece of state Companion shows has one owner: a service of the project scope or of the application. The owner holds the current value and compares every new one with it. It fires a `Signal` only when the value differs, so everything downstream can trust that a signal means a change. The comparison is the owner's, of its domain values (parsed assignments, the enabled packs, the catalog), which have a meaningful equality; nothing compares what a page shows, such as images or editor models. + +```java +public final class Signal { + /** Runs {@code listener} after each change, on the thread that fired it; returns what removes it. */ + public Runnable subscribe(Runnable listener); + /** Tells every listener; never while holding the owner's lock. */ + public void fire(); +} +``` + +`subscribe` has the shape of today's `addListener` methods, so owners move to it without touching their followers. + +- **A signal carries nothing; the follower asks the owner.** Payloads invite followers to keep their own copy of the state, a second truth that goes stale. Where followers need to tell two changes apart, the owner has two signals, as `GamePacks` has one for the resource packs and one for the datapacks. +- **State, outcomes and events are different things.** + - *State* is what an owner holds, and is followed through signals. + - *The outcome of an action* goes to whoever started it, through the action's future: a write's result, a save's success. An outcome someone else caused becomes state: the last outcome of a file followed in an external editor is held by that follow. + - *A failure that must not be missed*, such as the runtime index failing, is published by the owner to `NotificationCenter` when it happens, instead of followers watching for a brief state. +- **Connection-bound work compares connections.** `GameLocation`'s connection value includes the connection's number and the game's process. A request waiting for an answer keeps the number of the connection it was sent on, and is failed when the connection signal shows another. This replaces reacting to `DISCONNECTED` as an event, which a quick reconnect can hide. +- **The current project is state too.** The application owns it and signals a switch. UI that outlives a project (the tab strip, the Project tree, the status bar, search) follows "the current project's catalog" through its loader, which subscribes again on a switch. This deletes the `project.get() != scope` and `currentScope() == scope` checks. + +**Newest wins, where it is the same read.** A page loader runs one read at a time: a request during a read makes one more read after it, and the older result is dropped. When file readings (section 2) need the same, it moves into a class of its own, `LatestRead`, that both use, which also removes `KeyAssignments.generation`. It does not replace an owner's protection between requests of different kinds, where a later request must win over an earlier one that finishes last: `ItemIconService` adopting an archive the game announced over restoring the newest from disk, `PackCatalogService` taking a prepared catalog over a restore. Those counters stay in their owners; a signal counts changes, a counter of requests tells which answer is the newest, and they are different jobs. + +Owners: + +| Owner | Signals | Replaces | +|---|---|---| +| The application's projects | current project | the scope checks in `FileTreeView`, `CompanionApplication` and the UI | +| `PackCatalogService` | catalog | `addListener` | +| `GamePacks` | resource packs, datapacks | `addListener(side)`, `addResourcePackListener`, `addDatapackListener` | +| `ChangeRecord` | changes | `addListener` | +| `GameLocation` | connection (with the process), playing | `addListener(Consumer)` | +| `CurrentWorld` (new) | world | `WorldReadings`, which is deleted: it reads the current world's `level.dat` itself | +| `KeyAssignments` | assignments | its `addListener` and `KeyBindingControl.addAssignmentListener` | +| `GameLogs` | the listed logs and crash reports | the Logs page's and the tree's own listing | +| `ItemIconService` | icons | `addListener` | +| `ResourceEdits` | edits, working pack | `addEditListener`, `addWorkingPackListener` | +| `RuntimeIndexService` | status | `addStatusListener`, and `CompanionApplication.lastIndexStatus` | + +**Not state, and staying as they are:** events inside a subsystem that carry what happened, such as the debugger's session events (`DebuggerSessionController`, `MicrosoftJavaDebugEngine`, `DebuggerEditorPresentation`), the editor's caret and analysis (`AbstractCodeViewPanel`, `ASTCache`), search matches (`SearchManager`), the editor tabs' selection, script runs (`EditorScriptRunService`, `CompanionSession.addExecutionResultListener`) and `NotificationCenter`. Settings and the theme (`GlobalConfig`'s property listeners, `ThemeManager`) also keep theirs: they work, are not game or project state, and caused none of the problems above; they move only if a feature needs them to. The architecture test (section 7) lists them. + +## 2. Files on disk + +Files Companion shows that others write are followed by an owner through a `FileReading`: a watch, a reader, the last value, a comparison and a signal. It is what `KeyAssignments` is today, made once for all. + +```java +FileReading assignments = files.reading( + options, // the file or folder, resolved to one path form + Assignments::read, // reads the whole file; runs on file work + Duration.ofMillis(300)); // settle: read once writes stopped for this long +``` + +- **One `FileWatch` for the application** does the watching. A registration lives only while its reading has a follower. +- **A folder that does not exist yet** is watched through its nearest existing ancestor, and moves down when it appears: a new world's datapacks, `crash-reports`, `config`. +- **One path form.** Every registration resolves to the real path of its nearest existing ancestor plus the rest, so a linked folder is not watched twice or missed. +- **Settle and maximum wait.** A reading waits until writes stopped for its settle time, but at most a maximum wait, so a file written without pause still reads. +- **A read that fails is retried** after 1, 5 and 30 seconds, as a file written in place can be read half written, and a watch that fails or overflows reads everything it follows once. +- **Folders that move are the owner's.** `CurrentWorld` follows `playing` and moves its reading to the world the game plays, or with the game closed, the last played. +- **Windows renames.** A watched folder and its ancestors cannot be renamed while watched. `FileWatch` pauses around Companion's own renames; registrations sit on the smallest folders that answer the question, and readings of a world are dropped when the game leaves it. Whether the game deleting or renaming a world while it is followed works is tested on Windows in the step that adds world readings. +- **A reading's value is a domain value with a meaningful equality**, such as parsed assignments or a list of names; the reader supplies it, and a reading whose value cannot be compared fires on every change. +- **What each reading holds decides what fires.** The logs' reading holds the names of the logs and crash reports, not their sizes, so `latest.log` growing does not fire; the Logs page reads the log a row shows when the row is shown. A configuration folder's reading ignores Companion's own `*.totaldebug-original` files. +- **Readings replace every "read whenever shown":** `options.txt`, configuration files, logs and crash reports, the current world's `level.dat` and icon, the `saves` and `resourcepacks` folders. +- **Caches are allowed where keyed by what they cache**, a file's size and time or its hash, as `TextureThumbnails`, `CatalogIcons` and the parsed logs are. A cache is never the truth another part reads. + +**Adopting a file is a write, not a reading.** A texture saved in an external editor is taken into the pack: `ExternalEdits` registers with `FileWatch`, keeps its baseline and checks that the image is complete, and its adoption runs on the project's write queue through the pipeline. Before it records a change or asks for a reload, the adoption compares the saved content with what the pack already holds, by hash, and does nothing when they are the same: a notification of content Companion itself wrote, or a second notification of one save, must not record or reload again. Deduplication happens before side effects, in the owner, never after them in a page. The watchers of `FileUtils`, `ExternalEdits` and `KeyAssignments` become registrations. + +## 3. Pages + +A page reads and shows through `PageLoader`, and only through it. It names the signals it follows; the loader does the rest, the same way for every page: + +```java +this.loader = new PageLoader<>(this::read, this::show, this::fail) + .page(this) + .follows(catalog.changed()) + .follows(record.changed(), () -> !Objects.equals(record.change(target), this.seen)); +``` + +- **It reads when the page is first shown**, never in a constructor and never because of a navigation. +- **While the page is hidden, it only notes that a followed signal fired**, and reads once when the page is shown again. A page shown again with nothing changed reads nothing. +- **One read at a time, newest wins.** The loader shows every result it gets; it does not compare results, since owners only signal real changes. +- **Reads run on file work, results are shown on the Swing thread.** +- **`show` keeps what the user chose:** the selection and scroll position of rows that are still there (`Tables.keepingSelection`), and a selection a navigation asked for, such as a key binding to show, which the page keeps until a result contains it. +- **A page that writes holds its reads** (`hold`, `release`): while its save runs, a read under way is not shown, since it may predate the write, and followed changes wait as while the page is hidden; released, the page reads once what concerned it meanwhile. A page with unsaved changes that must not be replaced by a read (`ConfigPanel`) holds its reads the same way while they last. A page does not read again because it saved; it hears the owner's signal like every other follower. +- **A follow may say whether a change concerns the page**, asked when the page would read: the resource editor follows the whole change record, but only its own file's entry, and not its own save's. +- **A read never changes an owner.** What the Changes page's labels do today with `ChangeRecord.observed` moves to the owners, which notice a value put back outside Companion on their own readings, on the write queue. +- A part of a page that reads on its own, such as the resources of a definition, has its own loader with that part as its page. +- Work that only redraws from values in memory, such as a tab's title and icon, uses `loader.updates(runnable)`: the same waiting and merging, with no read. + +**Navigation never reads files.** It shows a page, and passes a selection to it. A navigation that carries a new request to the game, as inspecting a subject again, asks the owner of that request. `InspectionSession`'s timer, which polls the game for live values, stays until live channels (C2) push them, and is listed in the architecture test. + +This replaces `ShownUpdates`, the three modes, the subscriptions pages hold themselves, the reads in constructors, the `refresh()` calls on navigation and in the `model/*View` classes, and `CompanionUi.catalogChanged` and `changesRecorded`. + +## 4. Threads + +The Swing thread runs Swing, and nothing that waits or grows with the data: no file access, and no measuring or parsing every row, as the freeze fixed in #104 did. Other work runs on workers from one class, `Workers`: + +| Worker | For | Instead of | +|---|---|---| +| File work (a bounded pool of platform threads) | Reads for pages and readings | The shared pool in `PageLoader` and the one-argument `supplyAsync` calls; `ResourceViewPanel`'s pool | +| Serial workers (one thread each, by name) | Components whose state is confined to one thread, or whose order matters | The own executors of `TextureThumbnails`, `ModLogoIcons`, the item icon renderer, `JsonStateWriter` | +| The project's write queue (a serial worker the pipeline owns) | Every write of the pipeline, adoptions, and owners noticing values put back | `ConfigChanges`' executor, which every category borrows today | +| Timers (one scheduler) | Settle delays, retries, timeouts; a timer only hands work to another worker | `KeyAssignments`' and `ExternalEdits`' schedulers, `JsonStateWriter`'s scheduled flush | + +- Platform threads, not virtual ones: on Java 21 a virtual thread is pinned inside `synchronized` and by `ZipFile`, which most reads use. +- Swing timers stay for delays and animations on the Swing thread. +- **Allowed own threads**, each owned by a service that closes it: the debugger (`DebuggerSessionQueue`, `DebuggerEvaluationRunner`), the editor's Java analysis, script compilation, decompilation, search (also `SearchEverywherePopup`'s) and the runtime index, the MCP job service, `CompanionApplication`'s project switching and MCP lifecycle, and `ProjectSelectionServer`. The list lives in the architecture test; adding to it is a decision recorded here. + +## 5. Writes + +The change pipeline ([CHANGE_PIPELINE.md](CHANGE_PIPELINE.md)) stays the one way to change the game's and the packs' files and what the running game keeps. + +- **The pipeline owns the project's write queue.** `ConfigChanges` stops creating it; `ResourceEdits` gets it from the pipeline. +- **A category may write beside its value inside its own write task**, as `ResourceEdits` enables the managed pack in `options.txt` and writes its `pack.mcmeta`. Nothing writes the game's or the packs' files outside a write task. +- **Companion's own files** (its state, script files, originals, decompiled sources) are not the pipeline's; they are written by their owners on serial workers. +- **After a write, the category's owner fires its signal** if the value differs. + +## 6. Game messages + +A message reaches the owners that handle it directly, without a relay: + +```java +// When the project scope attaches to a connection: +connection.on(PackStackMessage.class, packs::told); +``` + +- **Handlers belong to the connection attached to the scope**, registered when the scope attaches and removed when it detaches, so a scope never receives another connection's messages. This replaces `CompanionApplication`'s checks that the scope is still current. +- **Several handlers per message**, run on the receiving thread in arrival order: `PLAYING` reaches the game location and the server scripts, and the datapacks are checked against what the game plays in the order the game sent them. +- **Answers are matched on connection and request**, as the change and reload answers are since #107. `RELAY_FAILED` goes to the owner of the refused request by its id, instead of a switch in `CompanionApplication`. +- **In the mod**, `CompanionAppClient` registers a handler through one method that checks the connection is authenticated and passes its number; a request that has no handler is refused, as now. `ProtocolBindings` keeps its two lists, which say which way each message goes. +- What stays in `CompanionApplication` is the application's own messages: connection, focus and inspection. +- This is the first part of A4, category registration on the roadmap; A4 then lets a category register its pages and Modpack rows the same way. + +## 7. The rules + +These go into [AGENTS.md](../AGENTS.md) and are checked by an architecture test that reads the sources, so a new listener list or thread is found by the build, not by a review round: + +1. State Companion shows has one owner, which compares and fires a `Signal` only on a change. No other listener lists for state; outcomes go through the action's future. +2. A page reads and shows through `PageLoader` and follows signals only through it: no subscriptions, file reads or threads of its own, no reads in constructors or on navigation, and a read never changes an owner. +3. Threads, executors and schedulers come from `Workers`, except the listed ones; no one-argument `supplyAsync` or `runAsync`. +4. Only `FileWatch` watches files. A file others write is followed through a `FileReading`; caches are keyed by what they cache and are never the truth. +5. The game's and the packs' files are written only inside a pipeline write task, on the project's write queue. +6. A game message is registered by the owners that handle it, on the connection. +7. The Swing thread does nothing that waits or grows with the data. +8. A problem of timing, staleness or reading twice is fixed in the system that owns it, never with a flag in a feature. +9. Moving a feature onto a system deletes its old way in the same PR: no superseded route stays. A PR says how many production lines it adds and removes, as information; no PR is sized to balance them. The finished migration is judged as a whole, for being simpler than what it replaced. + +The test lists each exception with its reason. Adding one is a decision recorded in this document. + +## Order of work + +Six PRs on 1.21.1, each reviewed until clean. A shared mechanism comes with its first users; the PRs that carry risk (1, 2 and 4) are kept small, the ones that repeat a proven pattern (3, 5 and 6) move many users at once. A moved feature keeps no part of its old way. Until the last user of an old way has moved, the old way stays for those not moved yet, as `PageLoader`'s modes do; the PR that moves the last user deletes it. Decided on 2026-09-30 over a finer split of about 22 PRs, whose extra review rounds bought no safety for the mechanical moves. + +| PR | Content | Deletes | +|---|---|---| +| 1 | The slice: `Signal` and the new `PageLoader` (`page`, `follows`, `hold`), with `Tables.keepingSelection`; its first users the Key bindings page, which only shows, and the resource editor, which edits, with the owners they follow on signals (catalog, key assignments, change record, packs, resource edits), key assignments read again after Companion's own write; the architecture test with every exception of today listed | the two pages' own subscriptions, constructor reads, navigation refreshes and selection keeping, `KeyBindingControl.addAssignmentListener`, `PackResourceEditor`'s read and write counters and follow flags | +| 2 | Game messages registered by their owners on the connection, in Companion and the mod | `CompanionSession.Listener`'s message methods, the relay and scope checks in `CompanionApplication`, the `RELAY_FAILED` switch, the mod's setters and repeated guards | +| 3 | The remaining owners on signals; the current project as state; connection numbers for waiting requests; the pipeline owning the write queue; the remaining executors onto `Workers` | the listener lists, `ConfigChanges`' executor, the scope checks, the UI classes' executors | +| 4 | `Workers`' timers, `FileWatch` and `FileReading`, with `KeyAssignments` and `CurrentWorld` (the World page and the tree) as first users | the watchers and schedulers of `FileUtils` and `KeyAssignments`, `WorldReadings` | +| 5 | The remaining readings (`GameLogs`, the configuration folders, `saves` and `resourcepacks`) with their pages; `ExternalEdits` as an adoption | `ExternalEdits`' watcher and scheduler, the `readsWhenShown` mode | +| 6 | All remaining pages, the Project tree and the tab strip; no file checks on the Swing thread | `ShownUpdates`, the old modes, the pages' own subscriptions, the reads in constructors, the navigation refreshes, the `CompanionUi` relays, `ChangeRecord.observed` from page reads | + +If PR 1 shows a flaw, the design is revised here before anything else moves. PR 3 needs `Workers` before PR 4; it brings the file work and serial workers, PR 4 the timers. + +After the messages, A4 continues with categories registering their pages and Modpack rows. Splitting `CompanionApplication` (the game connection and the MCP server into their own classes) and the mod's `CompanionAppClient` (launching Companion) is easier then and is decided at that point. + +UI work that follows no system, such as every page's loading, empty and failed states through `BrowserBody`, the wrong reason when the catalog is missing, and command text against [UI_GUIDE.md](UI_GUIDE.md), goes in on its own meanwhile. + +## Tests + +- `Signal`: listeners in order, removal of one subscription. `LatestRead`, with PR 4: a request during a read, several during one, a failure. +- `FileReading` and `FileWatch`, through a clock the test advances instead of real waits: settle and maximum wait, a folder that appears later, a linked folder, overflow, a failed read retried, a pause for a rename, a reading whose value did not change. +- `PageLoader`: hidden and shown, a signal during a read, several while hidden, a change that does not concern the page, a held page, a read under way when the page holds, a part of a page. The old modes' tests go with the modes. +- Messages: two handlers in order, a handler of a detached scope not called, an answer to another connection not taken, a request without a handler refused, an unauthenticated message refused in the mod. +- The architecture test: no listener list, thread, executor, watcher or one-argument async call outside the owners and exceptions it lists. +- **Each moved page is tested along the user's path**, not only through its parts: the page built as the application builds it, opened through `NavigationService` as a click opens it, then shown. The test counts the reads: one when it opens; none when it is shown again with nothing changed; one after a signal while it was hidden; none when navigating to it while it is showing, apart from the selection asked for. `LatestRead` passing its own tests does not show that opening a page reads once. +- Each migration PR keeps the tests of what it moves. + +## Not in this + +- The debugger, the editor's analysis and search keep their own events and threads; they are self-contained and not project state. +- The mod keeps its own threads for captures (`PackCatalogPublisher`, `ResourceSnapshots`) and reads game state on the game's thread, as now. Only its message handling changes (section 6). +- No reactive library and no general event bus: a signal is a list of listeners, a loader a running read and a flag, and every other rule is about who owns what. diff --git a/docs/UI_GUIDE.md b/docs/UI_GUIDE.md index a337f3ea5..527930deb 100644 --- a/docs/UI_GUIDE.md +++ b/docs/UI_GUIDE.md @@ -242,7 +242,7 @@ A component's own inner padding, such as a text field's or an editor's, and a ga ## Building and checking -- Swing work on the event thread; reading files and the catalog off it, through [`PageLoader`](../companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java): one read at a time, a request during a read reads once more and the older result is dropped. A page follows only the sources it shows, such as the resource packs or the datapacks, not both; a change while it is hidden loads once it is shown ([`ShownUpdates`](../companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/ShownUpdates.java) for pages that update themselves). Its editor tab stays current meanwhile: the tab strip reads a title when it asks, so titles are looked up then, and the editor tabs draw each tab's icon again when the catalog or the item icons change (`IEditorPanel.refreshTabIcon`). +- Swing work on the event thread; reading files and the catalog off it, through [`PageLoader`](../companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java), as [SYSTEMS.md](SYSTEMS.md) describes: a page reads when it is first shown and after a signal it follows, once it is shown; one read at a time, a request during a read reads once more and the older result is dropped. A table read again keeps its selection and scroll (`Tables.keepingSelection`). Its editor tab stays current meanwhile: the tab strip reads a title when it asks, so titles are looked up then, and the editor tabs draw each tab's icon again when the catalog or the item icons change (`IEditorPanel.refreshTabIcon`). Pages not moved yet use `PageLoader`'s older modes or [`ShownUpdates`](../companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/ShownUpdates.java). - A browser's filter bar, notice line and in-place message come from [`BrowserBody`](../companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/BrowserBody.java). - Everything follows a theme switch: SVG icons with dark pairs, colors from roles, and no colors cached in fields. - Sizes go through `UIScale` and `UiMetrics`; no fixed pixel sizes for text. From db26501686de4273659ed3b733eadbdb7a24240d Mon Sep 17 00:00:00 2001 From: Pelotrio <45769595+Pelotrio@users.noreply.github.com> Date: Wed, 30 Sep 2026 11:32:32 +0200 Subject: [PATCH 02/12] Systems slice: owners hold what they read, failed reads retry, the rules test counts more From an adversarial review of the slice: - KeyAssignments always reads its first value, also where the game's folder cannot be watched yet, and keeps it when a newer write overtakes that read, so no later change goes untold. The Key bindings page shows the keys it holds rather than reading options.txt itself; unwatched, it reads them whenever the page is shown. - A page whose read failed reads again when it is shown again. - The resource editor releases its held reads in finally, so nothing thrown can leave it holding them. - SystemsRulesTest also counts then...Async calls without an executor, commonPool, startVirtualThread, java.util.Timer, statically imported executor factories, Thread subclasses, and callbacks kept in any collection; the files that do so today are listed. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../catalog/KeyAssignments.java | 36 +++++- .../catalog/KeyBindingControl.java | 10 ++ .../ui/components/PageLoader.java | 14 ++- .../components/catalog/KeyBindingsPanel.java | 4 +- .../editors/PackResourceEditor.java | 108 ++++++++++-------- .../totalDebugCompanion/SystemsRulesTest.java | 60 +++++++--- .../catalog/KeyAssignmentsTest.java | 21 +++- .../ui/components/PageLoaderTest.java | 25 ++++ docs/SYSTEMS.md | 2 +- 9 files changed, 203 insertions(+), 77 deletions(-) diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java index c6a92d72f..466a741e9 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java @@ -46,15 +46,37 @@ public final class KeyAssignments implements AutoCloseable { /** Whether the last read failed: a page may show that failure, so the next read that succeeds is told. */ private boolean unreadable; - /** Watches {@code options}; where its folder, the game's, cannot be watched, nothing is told. */ + /** + * Watches {@code options}. Where its folder, the game's, cannot be watched, as before the game first ran, only + * Companion's own writes are told, and {@link #assignments()} reads the file each time. + */ public KeyAssignments(Path options) { this.options = Objects.requireNonNull(options, "options").toAbsolutePath().normalize(); this.watcher = watcher(this.options.getParent()); - if (this.watcher == null) return; - Thread.ofPlatform().daemon().name("Companion options.txt watcher").start(this::watch); + if (this.watcher != null) Thread.ofPlatform().daemon().name("Companion options.txt watcher").start(this::watch); this.timer.execute(() -> read(0, 0)); } + /** Whether writes of others are seen; without, a page reads {@link #assignments()} whenever it is shown. */ + public boolean watched() { + return this.watcher != null; + } + + /** + * The keys the file assigns: as last read while it is watched, and read now where it has not been read yet or is + * not watched. Blocking where it reads. + */ + public Map assignments() throws IOException { + synchronized (this) { + if (this.read != null && this.watcher != null) return this.read; + } + Map now = KeyBindings.readOptions(this.options); + synchronized (this) { + if (this.read == null || this.watcher == null) this.read = now; + return this.read; + } + } + private static WatchService watcher(Path folder) { WatchService watcher = null; try { @@ -64,7 +86,7 @@ private static WatchService watcher(Path folder) { return watcher; } catch (IOException | RuntimeException unwatchable) { System.getLogger(KeyAssignments.class.getName()).log(System.Logger.Level.WARNING, - "Key assignments in " + folder + " are read when a page is shown, not watched: " + unwatchable.getMessage()); + "Key assignments in " + folder + " are read when their page is shown, not watched: " + unwatchable.getMessage()); if (watcher != null) { try { watcher.close(); @@ -137,7 +159,11 @@ private void read(int attempt, long generation) { } boolean changed; synchronized (this) { - if (generation != this.generation) return; + // A newer write is read after this one; what this one read is still what the next compares with. + if (generation != this.generation) { + if (this.read == null) this.read = now; + return; + } changed = this.unreadable || this.read != null && !this.read.equals(now); this.read = now; this.unreadable = false; diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControl.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControl.java index 2b636a373..6bec98ebe 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControl.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControl.java @@ -53,6 +53,16 @@ public Signal assignmentsChanged() { return this.assignments.changed(); } + /** The keys {@code options.txt} assigns now ({@link KeyAssignments#assignments()}). Blocking. */ + public Map assignments() throws IOException { + return this.assignments.assignments(); + } + + /** Whether others' writes of {@code options.txt} are told; without, a page reads the keys whenever it is shown. */ + public boolean assignmentsWatched() { + return this.assignments.watched(); + } + /** The change record the bindings' changes are entered in. */ public ChangeRecord record() { return this.pipeline.record(); diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java index 29582d9b5..4a6715507 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java @@ -24,8 +24,9 @@ * *

A page names itself ({@link #page}) and the signals it follows ({@link #follows}); the loader reads when the page is * first shown, and after a followed signal fires, at once while the page is shown, otherwise once it is shown again. A - * page shown again with nothing changed reads nothing. While the page holds its reads ({@link #hold}), as during a save, - * signals wait in the same way. The page never reads in its constructor or because a navigation showed it.

+ * page shown again with nothing changed reads nothing, unless its last read failed. While the page holds its reads + * ({@link #hold}), as during a save, signals wait in the same way. The page never reads in its constructor or because a + * navigation showed it.

* *

Pages not moved to {@link #page} yet use the older modes {@link #whenShown}, {@link #waitsWhileHidden} and * {@link #readsWhenShown} with {@link #follow}, and read in their constructors; the last of them to move deletes those.

@@ -242,8 +243,13 @@ private void showRead(T value, Throwable failure) { return; } if (this.cancelled) return; - if (failure == null) this.show.accept(value); - else this.fail.accept(failure instanceof CompletionException && failure.getCause() != null ? failure.getCause() : failure); + if (failure == null) { + this.show.accept(value); + return; + } + // A page shown again after a failed read tries once more, as when the file was being written. + if (this.page != null) this.missed.add(() -> true); + this.fail.accept(failure instanceof CompletionException && failure.getCause() != null ? failure.getCause() : failure); } /** The read that runs or ran last, which completes when the read has finished, before it is shown. */ diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/KeyBindingsPanel.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/KeyBindingsPanel.java index 92c88acbe..cb15253d0 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/KeyBindingsPanel.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/KeyBindingsPanel.java @@ -162,6 +162,8 @@ public void mousePressed(MouseEvent event) { this.loader = new PageLoader<>(this::prepareLoad, loaded -> show(loaded.index(), loaded.bindings(), ""), failure -> show(this.catalog.index().orElse(null), null, "Could not read options.txt: " + failure.getMessage())) .page(this).follows(catalog.changed()).follows(control.assignmentsChanged()); + // Until the game's folder can be watched, as before the game first ran, others' writes are seen when shown. + if (!control.assignmentsWatched()) this.loader.readsWhenShown(this); addHierarchyListener(event -> { if ((event.getChangeFlags() & HierarchyEvent.SHOWING_CHANGED) != 0 && !isShowing()) stopCapture(); }); @@ -246,7 +248,7 @@ private Callable prepareLoad() { } PackCatalog captured = index.catalog(); return () -> new Loaded(index, new KeyBindings(captured.keyBindings(), captured.keyContexts(), - KeyBindings.readOptions(this.control.options()), captured.keyNames())); + this.control.assignments(), captured.keyNames())); } /** Shows the binding named {@code name}, such as {@code key.jump}, once the keys are read. */ diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java index 5f43fd3a7..fb861b379 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java @@ -423,23 +423,26 @@ protected final void saveThen(Consumer after) { throw new CompletionException(exception); } }).whenComplete((held, failure) -> SwingUtilities.invokeLater(() -> { - this.busy = false; - changed(); - if (this.disposed) return; - if (failure != null) { - showNotice("Not opened: " + message(failure), ThemeColors::error); - this.loader.release(); - } else if (held.equals(read)) { - this.loader.release(); - after.accept(pack); - } else if (this.askToReplace.test(new ChangePipeline.Stale(this.path.substring(this.path.lastIndexOf('/') + 1) - + " changed in " + this.packName + " since this tab read it"))) { - this.baseline = null; - // The save holds the reads on; where it does not start, they go on. - if (!save(after)) this.loader.release(); - } else { - readAfterSave(); - this.loader.release(); + // The reads go on afterwards, whatever happens here, unless a save takes the hold over. + boolean savingAgain = false; + try { + this.busy = false; + changed(); + if (this.disposed) return; + if (failure != null) { + showNotice("Not opened: " + message(failure), ThemeColors::error); + } else if (held.equals(read)) { + this.loader.release(); + after.accept(pack); + } else if (this.askToReplace.test(new ChangePipeline.Stale(this.path.substring(this.path.lastIndexOf('/') + 1) + + " changed in " + this.packName + " since this tab read it"))) { + this.baseline = null; + savingAgain = save(after); + } else { + readAfterSave(); + } + } finally { + if (!savingAgain) this.loader.release(); } })); } @@ -474,43 +477,48 @@ private boolean save(Consumer after) { } }).thenCompose(bytes -> this.edits.save(this.path, into, bytes, alongside, expected)) .whenComplete((saved, failure) -> SwingUtilities.invokeLater(() -> { - this.busy = false; - this.saving = null; - if (this.disposed) return; - if (failure != null) { - showState(""); - changed(); - // Asked before following a change that came during the save, which would hold the overwrite back. - if (cause(failure) instanceof ChangePipeline.Stale changedSince && this.askToReplace.test(changedSince)) { - this.baseline = null; - if (!save(after)) this.loader.release(); + // The reads go on afterwards, whatever happens here, unless another save takes the hold over. + boolean savingAgain = false; + try { + this.busy = false; + this.saving = null; + if (this.disposed) return; + if (failure != null) { + showState(""); + changed(); + // Asked before following a change that came during the save, which would hold the overwrite back. + if (cause(failure) instanceof ChangePipeline.Stale changedSince && this.askToReplace.test(changedSince)) { + this.baseline = null; + savingAgain = save(after); + return; + } + showNotice("Not saved: " + message(failure), ThemeColors::error); + // What the record got during the attempt is the save's to tell, not a change that ends its notice. + this.seen = recorded(); + // A copy written since, which the save refused to replace, is read, so Discard goes back to it; + // the changes on screen stay, still unsaved. + readAfterSave(); return; } - showNotice("Not saved: " + message(failure), ThemeColors::error); - // What the record got during the attempt is the save's to tell, not a change that ends its notice. - this.seen = recorded(); - // A copy written since, which the save refused to replace, is read, so Discard goes back to it; - // the changes on screen stay, still unsaved. - readAfterSave(); + this.baseline = written[0]; + this.baselinePack = saved.pack(); + this.packHash = written[0]; + boolean keepEdits = !same(shown(), edited); + showPack(saved.pack()); + this.packContent = edited; + this.managed = true; + if (keepEdits) markSaved(edited); + else load(edited); + showSaved(saved); + changed(); + // Another tab's save that came while this one ran was not read then. + ChangeRecord.Change now = recorded(); + if (now != null && !now.current().equals(written[0])) readAfterSave(); this.loader.release(); - return; + if (after != null) after.accept(saved.pack()); + } finally { + if (!savingAgain) this.loader.release(); } - this.baseline = written[0]; - this.baselinePack = saved.pack(); - this.packHash = written[0]; - boolean keepEdits = !same(shown(), edited); - showPack(saved.pack()); - this.packContent = edited; - this.managed = true; - if (keepEdits) markSaved(edited); - else load(edited); - showSaved(saved); - changed(); - // Another tab's save that came while this one ran was not read then. - ChangeRecord.Change now = recorded(); - if (now != null && !now.current().equals(written[0])) readAfterSave(); - this.loader.release(); - if (after != null) after.accept(saved.pack()); })); return true; } diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/SystemsRulesTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/SystemsRulesTest.java index 2e11c14e0..ffd93e5e5 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/SystemsRulesTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/SystemsRulesTest.java @@ -9,6 +9,7 @@ import org.eclipse.jdt.core.dom.FieldDeclaration; import org.eclipse.jdt.core.dom.MethodInvocation; import org.eclipse.jdt.core.dom.SimpleName; +import org.eclipse.jdt.core.dom.TypeDeclaration; import org.eclipse.jdt.core.dom.VariableDeclarationFragment; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -35,9 +36,24 @@ */ class SystemsRulesTest { private static final Path SOURCES = Path.of("src/main/java/com/github/minecraft_ta/totalDebugCompanion"); + /** A collection of callbacks: any list, set, map, queue or deque whose elements are called. */ private static final Pattern LISTENER_COLLECTION = Pattern.compile( - "(List|Set|Collection|Map)<.*\\b(Runnable|Consumer|BiConsumer|IntConsumer|[A-Z]\\w*Listener)\\b.*>"); - private static final Set THREAD_TYPES = Set.of("Thread", "ThreadPoolExecutor", "ScheduledThreadPoolExecutor", "ForkJoinPool"); + "\\w*(List|Set|Collection|Map|Queue|Deque)<.*\\b(Runnable|Consumer|BiConsumer|IntConsumer|\\w*Listener|\\w*Callback)\\b.*>"); + /** Names of fields that keep callbacks to tell, rather than, say, what removes subscriptions. */ + private static final Pattern LISTENER_NAME = Pattern.compile(".*(listener|callback|subscriber|observer|handler).*"); + private static final Set THREAD_TYPES = Set.of("Thread", "java.lang.Thread", "ThreadPoolExecutor", + "ScheduledThreadPoolExecutor", "ForkJoinPool", "java.util.Timer"); + /** Executor factories of {@code Executors}, also when imported statically. */ + private static final Set EXECUTOR_FACTORIES = Set.of("newFixedThreadPool", "newCachedThreadPool", + "newSingleThreadExecutor", "newSingleThreadScheduledExecutor", "newScheduledThreadPool", "newWorkStealingPool", + "newVirtualThreadPerTaskExecutor", "newThreadPerTaskExecutor", "unconfigurableExecutorService"); + /** {@code CompletableFuture}'s async methods, by how many arguments they take without an executor. */ + private static final Map ASYNC_WITHOUT_EXECUTOR = Map.ofEntries( + Map.entry("supplyAsync", 1), Map.entry("runAsync", 1), Map.entry("thenApplyAsync", 1), Map.entry("thenAcceptAsync", 1), + Map.entry("thenRunAsync", 1), Map.entry("thenComposeAsync", 1), Map.entry("whenCompleteAsync", 1), + Map.entry("handleAsync", 1), Map.entry("exceptionallyAsync", 1), Map.entry("exceptionallyComposeAsync", 1), + Map.entry("thenCombineAsync", 2), Map.entry("thenAcceptBothAsync", 2), Map.entry("runAfterBothAsync", 2), + Map.entry("applyToEitherAsync", 2), Map.entry("acceptEitherAsync", 2), Map.entry("runAfterEitherAsync", 2)); /** What a file does against a rule, and how often. */ private record Found(Map threads, Map sharedPool, Map listenerLists, @@ -78,7 +94,9 @@ private record Allowed(int times, String why) { // All move onto Workers' file work in PR 3. private static final Map SHARED_POOL = Map.ofEntries( - Map.entry("CompanionApplication.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("CompanionApplication.java", new Allowed(2, "moves onto Workers in PR 3")), + Map.entry("inspection/ItemIconService.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("ui/components/inspection/DataView.java", new Allowed(1, "moves onto Workers in PR 3")), Map.entry("inspection/InspectionSession.java", new Allowed(2, "moves onto Workers in PR 3")), Map.entry("model/CodeView.java", new Allowed(1, "moves onto Workers in PR 3")), Map.entry("navigation/NavigationService.java", new Allowed(2, "moves onto Workers in PR 3")), @@ -88,7 +106,7 @@ private record Allowed(int times, String why) { Map.entry("ui/components/catalog/ModPanel.java", new Allowed(1, "moves onto Workers in PR 3")), Map.entry("ui/components/editors/PackResourceEditor.java", new Allowed(2, "its saves move onto Workers in PR 3")), Map.entry("ui/components/editors/ResourceTextEditor.java", new Allowed(1, "moves onto Workers in PR 3")), - Map.entry("ui/components/editors/ScriptPanel.java", new Allowed(1, "moves onto Workers in PR 3")), + Map.entry("ui/components/editors/ScriptPanel.java", new Allowed(3, "moves onto Workers in PR 3")), Map.entry("ui/components/global/NotificationWidget.java", new Allowed(1, "moves onto Workers in PR 3")), Map.entry("ui/components/global/ProjectSelector.java", new Allowed(1, "moves onto Workers in PR 3")), Map.entry("ui/components/treeView/FileTreeView.java", new Allowed(1, "moves onto Workers in PR 3")), @@ -103,6 +121,9 @@ private record Allowed(int times, String why) { private static final Map LISTENER_LISTS = Map.ofEntries( Map.entry("util/Signal.java", new Allowed(1, "the signal every owner uses")), Map.entry("jdt/diagnostics/ASTCache.java", new Allowed(1, "the editor's analysis")), + Map.entry("debugger/DebuggerSessionController.java", new Allowed(1, "the debugger's session events")), + Map.entry("debugger/MicrosoftJavaDebugEngine.java", new Allowed(1, "the debugger's session events")), + Map.entry("ui/components/editors/DebuggerEditorPresentation.java", new Allowed(1, "the debugger's session events")), Map.entry("notification/NotificationCenter.java", new Allowed(1, "notifications, an event")), Map.entry("script/EditorScriptRunService.java", new Allowed(2, "script runs, an event")), Map.entry("session/CompanionSession.java", new Allowed(1, "script results, an event")), @@ -114,6 +135,7 @@ private record Allowed(int times, String why) { Map.entry("ui/theme/ThemeManager.java", new Allowed(1, "the theme, which stays as it is")), Map.entry("game/GameLocation.java", new Allowed(1, "moves onto signals in PR 3")), Map.entry("inspection/ItemIconService.java", new Allowed(1, "moves onto signals in PR 3")), + Map.entry("runtime/RuntimeIndexService.java", new Allowed(1, "moves onto signals in PR 3")), Map.entry("catalog/WorldReadings.java", new Allowed(1, "replaced by CurrentWorld in PR 4")), Map.entry("util/FileUtils.java", new Allowed(1, "becomes FileWatch in PR 4")), Map.entry("pack/ExternalEdits.java", new Allowed(1, "becomes an adoption in PR 5"))); @@ -135,16 +157,20 @@ static void scan() throws IOException { } for (Path file : files) { String name = SOURCES.relativize(file).toString().replace('\\', '/'); - parse(file).accept(new ASTVisitor() { + CompilationUnit unit = parse(file); + // A Timer is Swing's, which runs on the Swing thread, unless the file names java.util's. + boolean utilTimer = unit.imports().stream().anyMatch(imported -> imported.toString().contains("java.util.Timer;")); + unit.accept(new ASTVisitor() { @Override public boolean visit(MethodInvocation call) { String method = call.getName().getIdentifier(); String target = call.getExpression() instanceof SimpleName simple ? simple.getIdentifier() : ""; - if (target.equals("Executors") && method.startsWith("new") - || target.equals("Thread") && (method.equals("ofPlatform") || method.equals("ofVirtual"))) { + if (EXECUTOR_FACTORIES.contains(method) && (target.equals("Executors") || call.getExpression() == null) + || target.equals("Thread") && Set.of("ofPlatform", "ofVirtual", "startVirtualThread").contains(method)) { threads.merge(name, 1, Integer::sum); } - if ((method.equals("supplyAsync") || method.equals("runAsync")) && call.arguments().size() == 1) { + Integer withoutExecutor = ASYNC_WITHOUT_EXECUTOR.get(method); + if (withoutExecutor != null && call.arguments().size() == withoutExecutor || method.equals("commonPool")) { sharedPool.merge(name, 1, Integer::sum); } if (method.equals("newWatchService")) watchers.merge(name, 1, Integer::sum); @@ -153,17 +179,25 @@ public boolean visit(MethodInvocation call) { @Override public boolean visit(ClassInstanceCreation creation) { - if (THREAD_TYPES.contains(creation.getType().toString())) threads.merge(name, 1, Integer::sum); + String type = creation.getType().toString(); + if (THREAD_TYPES.contains(type) || utilTimer && type.equals("Timer")) threads.merge(name, 1, Integer::sum); + return true; + } + + @Override + public boolean visit(TypeDeclaration type) { + if (type.getSuperclassType() != null && THREAD_TYPES.contains(type.getSuperclassType().toString())) { + threads.merge(name, 1, Integer::sum); + } return true; } @Override public boolean visit(FieldDeclaration field) { - if (!LISTENER_COLLECTION.matcher(field.getType().toString()).matches()) return true; + if (!LISTENER_COLLECTION.matcher(field.getType().toString()).find()) return true; for (Object fragment : field.fragments()) { - // A collection of callbacks kept to be told, not of what removes subscriptions. String variable = ((VariableDeclarationFragment) fragment).getName().getIdentifier(); - if (variable.toLowerCase(Locale.ROOT).contains("listener")) listenerLists.merge(name, 1, Integer::sum); + if (LISTENER_NAME.matcher(variable.toLowerCase(Locale.ROOT)).matches()) listenerLists.merge(name, 1, Integer::sum); } return true; } @@ -189,7 +223,7 @@ void threadsAndExecutorsComeFromTheirOwners() { @Test void workRunsOnANamedWorkerNotTheSharedPool() { - check("runs supplyAsync or runAsync on the shared pool", found.sharedPool(), SHARED_POOL); + check("runs async work on the shared pool", found.sharedPool(), SHARED_POOL); } @Test diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java index c454f51f2..15c39740f 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java @@ -6,11 +6,13 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardCopyOption; +import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.IntSupplier; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; class KeyAssignmentsTest { @TempDir Path directory; @@ -40,9 +42,22 @@ void aKeyReboundIsToldAndAnotherOptionIsNot() throws Exception { } @Test - void aGameFolderThatCannotBeWatchedTellsNothing() throws Exception { - try (KeyAssignments assignments = new KeyAssignments(this.directory.resolve("missing/options.txt"))) { - assignments.changed().subscribe(() -> { }); + void aGameFolderThatCannotBeWatchedYetReadsTheFileWhenAskedAndTellsCompanionsOwnWrites() throws Exception { + Path options = this.directory.resolve("game/options.txt"); + AtomicInteger told = new AtomicInteger(); + try (KeyAssignments assignments = new KeyAssignments(options)) { + assignments.changed().subscribe(told::incrementAndGet); + assertFalse(assignments.watched(), "the game has not run yet, so its folder is missing"); + Thread.sleep(300); + + Files.createDirectories(options.getParent()); + Files.writeString(options, "key_key.jump:key.keyboard.g\n"); + assertEquals(Map.of("key.jump", KeyBindings.Assignment.decode("key.keyboard.g")), assignments.assignments(), + "unwatched, the keys are read when asked, as when their page is shown"); + + Files.writeString(options, "key_key.jump:key.keyboard.h\n"); + assignments.readNow(); + await(told::get, 1); } } diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java index 856f5d16c..f5859208b 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java @@ -266,6 +266,31 @@ void aReadUnderWayWhenThePageHoldsIsNotShownAndIsReadAgain() throws Exception { assertEquals(List.of(2), shown, "what it was for is read again after the write"); } + @Test + void aPageWhoseReadFailedReadsAgainWhenShownAgain() throws Exception { + ShowablePage page = new ShowablePage(); + AtomicBoolean failing = new AtomicBoolean(true); + AtomicInteger failures = new AtomicInteger(); + PageLoader loader = onEdt(() -> new PageLoader(() -> { + this.prepared.incrementAndGet(); + return () -> { + if (failing.get()) throw new IOException("written in parts"); + return "read"; + }; + }, read -> { }, failure -> failures.incrementAndGet()).page(page)); + + show(page, loader, true); + settle(loader); + assertEquals(1, failures.get()); + failing.set(false); + show(page, loader, false); + show(page, loader, true); + assertEquals(2, this.prepared.get(), "shown again, a page whose read failed tries once more"); + show(page, loader, false); + show(page, loader, true); + assertEquals(2, this.prepared.get(), "and once it read, shown again without a change it reads nothing"); + } + private static void fire(Signal signal) throws Exception { signal.fire(); SwingUtilities.invokeAndWait(() -> { }); diff --git a/docs/SYSTEMS.md b/docs/SYSTEMS.md index 587cc9e58..d09e776a6 100644 --- a/docs/SYSTEMS.md +++ b/docs/SYSTEMS.md @@ -113,7 +113,7 @@ this.loader = new PageLoader<>(this::read, this::show, this::fail) ``` - **It reads when the page is first shown**, never in a constructor and never because of a navigation. -- **While the page is hidden, it only notes that a followed signal fired**, and reads once when the page is shown again. A page shown again with nothing changed reads nothing. +- **While the page is hidden, it only notes that a followed signal fired**, and reads once when the page is shown again. A page shown again with nothing changed reads nothing, unless its last read failed, as for a file read while it was written. - **One read at a time, newest wins.** The loader shows every result it gets; it does not compare results, since owners only signal real changes. - **Reads run on file work, results are shown on the Swing thread.** - **`show` keeps what the user chose:** the selection and scroll position of rows that are still there (`Tables.keepingSelection`), and a selection a navigation asked for, such as a key binding to show, which the page keeps until a result contains it. From 393f5f39da2c10e99e8d0075c17d8f2b0440f20f Mon Sep 17 00:00:00 2001 From: Pelotrio <45769595+Pelotrio@users.noreply.github.com> Date: Wed, 30 Sep 2026 11:49:39 +0200 Subject: [PATCH 03/12] Resource editor: a record change during a read is checked against the pack the read found The pack a mod's resource is saved into is known only once the copies are read. A change of its entry in the change record while the tab opened, or moved to another working pack, was taken as not concerning the tab and dropped, leaving the content read before on screen. After each read the entry is compared again, as before the slice. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../ui/components/editors/PackResourceEditor.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java index fb861b379..59d5ed460 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java @@ -350,6 +350,9 @@ private void showCopies(Found found) { showNotice(this.readNotice, ThemeColors::warning); } changed(); + // The pack is known only once read, as when the tab opened or moved to another working pack: a change of its + // entry in the record after the read looked at it did not concern the pack the tab showed then, so it is read now. + if (!Objects.equals(recorded(), this.seen)) this.loader.load(); } /** Lists the packs the content could be saved into, the current one selected; an opened pack's file shows none. */ From cd3c0a5ee5fc00c4e3338e256add7aaac0dca562 Mon Sep 17 00:00:00 2001 From: Pelotrio <45769595+Pelotrio@users.noreply.github.com> Date: Wed, 30 Sep 2026 11:56:47 +0200 Subject: [PATCH 04/12] Resource editor: Save waits only for a read that may move the tab to another pack The slice made every read of the copies hold Save back, also one after a failed save or an edit elsewhere, so a Save pressed during it did nothing; CI's slower machine hit that in aSaveOverTextAnotherTabSavedSinceAsksFirst. As before the slice, only the first read and one after a pack or working pack change hold it. The page loader tells a read's preparation which followed signals led to it (fired), rather than signals carrying what changed. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../ui/components/PageLoader.java | 18 +++++++++++++++++- .../editors/PackResourceEditor.java | 13 ++++++++++--- .../ui/components/PageLoaderTest.java | 19 +++++++++++++++++++ docs/SYSTEMS.md | 2 +- 4 files changed, 47 insertions(+), 5 deletions(-) diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java index 4a6715507..d69cecb52 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java @@ -7,8 +7,10 @@ import java.awt.event.HierarchyEvent; import java.awt.event.HierarchyListener; import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Objects; +import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; @@ -56,6 +58,8 @@ public interface Read { * the page would read. Empty when there is nothing to read. */ private final List missed = new ArrayList<>(); + /** The followed signals fired since the last read started, which that read's preparation may ask about. */ + private final Set fired = new HashSet<>(); /** Whether the page holds its reads, as while it saves. */ private boolean held; /** How many reads started. */ @@ -99,10 +103,21 @@ public PageLoader follows(Signal signal) { */ public PageLoader follows(Signal signal, BooleanSupplier concerns) { Objects.requireNonNull(concerns, "concerns"); - this.unsubscribe.add(signal.subscribe(() -> SwingUtilities.invokeLater(() -> changed(concerns)))); + this.unsubscribe.add(signal.subscribe(() -> SwingUtilities.invokeLater(() -> { + this.fired.add(signal); + changed(concerns); + }))); return this; } + /** + * Whether {@code signal} fired since the last read started, for {@link Read#prepare} to tell what a read is for, such + * as a pack change that may move a tab to another pack, as against an edit elsewhere. Swing thread only. + */ + public boolean fired(Signal signal) { + return this.fired.contains(signal); + } + /** * Waits while {@code page} is hidden and reads every time it is shown, since what it shows may have changed while it * was hidden without a source telling, such as a file the game writes. @@ -214,6 +229,7 @@ public void load() { return; } Callable task = this.read.prepare(); + this.fired.clear(); if (task == null) return; this.running = true; this.cancelled = false; diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java index 59d5ed460..e4b843853 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java @@ -95,7 +95,10 @@ private record Found(Path pack, ChangeRecord.Change recorded, V managed, Stri private String packName = "the working pack"; private boolean managed; private boolean busy; - /** The copies are being read; a save waits for them, so it goes into the pack the tab shows. */ + /** + * The copies are being read where the tab may move to another pack, as when it opened or the packs or the working + * pack changed; a save waits for them, so it goes into the pack the tab shows. + */ private boolean following; /** * What a read of the copies last put in the notice, such as why the game does not use the shown copy, or null. The @@ -287,8 +290,12 @@ private Callable> prepareCopies() { // A new working pack's copy is read before anything is edited, as when the tab opened. A pack stack change, which // comes after every reload and rarely changes the pack, leaves typing and drawing alone. if (working != null && !working.equals(this.readWorking)) setEditable(false); - this.following = true; - changed(); + // A read for an edit elsewhere, such as a revert or another tab's save, leaves the pack alone, and saving goes on. + if (this.pack == null || this.loader.fired(this.edits.packs().changed(GamePacks.side(this.path))) + || this.loader.fired(this.edits.workingPackChosen(this.path))) { + this.following = true; + changed(); + } return () -> { Path pack = this.opened != null ? this.opened : this.edits.pack(this.path); // The record is looked at before the file, so a change between the two is caught afterwards. diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java index f5859208b..307fa9f66 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java @@ -291,6 +291,25 @@ void aPageWhoseReadFailedReadsAgainWhenShownAgain() throws Exception { assertEquals(2, this.prepared.get(), "and once it read, shown again without a change it reads nothing"); } + @Test + void aReadCanTellWhichOfItsSignalsLedToIt() throws Exception { + ShowablePage page = new ShowablePage(); + Signal packs = new Signal(); + Signal record = new Signal(); + List causes = new CopyOnWriteArrayList<>(); + PageLoader[] loader = new PageLoader[1]; + loader[0] = onEdt(() -> new PageLoader(() -> { + causes.add((loader[0].fired(packs) ? "packs" : "") + (loader[0].fired(record) ? "record" : "")); + return () -> "read"; + }, read -> { }, failure -> { }).page(page).follows(packs).follows(record)); + show(page, loader[0], true); + fire(record); + settle(loader[0]); + fire(packs); + settle(loader[0]); + assertEquals(List.of("", "record", "packs"), causes, "each read knows what it is for, and no more"); + } + private static void fire(Signal signal) throws Exception { signal.fire(); SwingUtilities.invokeAndWait(() -> { }); diff --git a/docs/SYSTEMS.md b/docs/SYSTEMS.md index d09e776a6..4317eaea7 100644 --- a/docs/SYSTEMS.md +++ b/docs/SYSTEMS.md @@ -118,7 +118,7 @@ this.loader = new PageLoader<>(this::read, this::show, this::fail) - **Reads run on file work, results are shown on the Swing thread.** - **`show` keeps what the user chose:** the selection and scroll position of rows that are still there (`Tables.keepingSelection`), and a selection a navigation asked for, such as a key binding to show, which the page keeps until a result contains it. - **A page that writes holds its reads** (`hold`, `release`): while its save runs, a read under way is not shown, since it may predate the write, and followed changes wait as while the page is hidden; released, the page reads once what concerned it meanwhile. A page with unsaved changes that must not be replaced by a read (`ConfigPanel`) holds its reads the same way while they last. A page does not read again because it saved; it hears the owner's signal like every other follower. -- **A follow may say whether a change concerns the page**, asked when the page would read: the resource editor follows the whole change record, but only its own file's entry, and not its own save's. +- **A follow may say whether a change concerns the page**, asked when the page would read: the resource editor follows the whole change record, but only its own file's entry, and not its own save's. A read's preparation may also ask which followed signals led to it (`fired`): the resource editor lets Save wait only for a read after a pack change, which may move it to another pack, not for one after an edit elsewhere. - **A read never changes an owner.** What the Changes page's labels do today with `ChangeRecord.observed` moves to the owners, which notice a value put back outside Companion on their own readings, on the write queue. - A part of a page that reads on its own, such as the resources of a definition, has its own loader with that part as its page. - Work that only redraws from values in memory, such as a tab's title and icon, uses `loader.updates(runnable)`: the same waiting and merging, with no read. From 4c7b76abb7a708050435c543497762d63ace31bd Mon Sep 17 00:00:00 2001 From: Pelotrio <45769595+Pelotrio@users.noreply.github.com> Date: Wed, 30 Sep 2026 12:13:56 +0200 Subject: [PATCH 05/12] Systems slice: hidden pages never read, whoever asks; saves check their target when made From the review's finish line for the slice, each behavior now proven with a read paused while something else happens: - A page named with page() reads nothing while hidden, also when the page itself asks, as the resource editor does to check a finished read against a later change; it reads once when shown. Changes missed while hidden are kept once per source, however often they came. - The resource editor's save checks the pack it goes into when it is made: a working pack or world that changed before the tab heard of it is refused, and the tab reads the pack it saves into now, keeping the edit unsaved. - ResourceEdits tells its edits only when a save, revert or adoption wrote something. - ResourceTabReadsTest opens a mod's resource through navigation and counts its reads, as PageReadsTest does for the Key bindings page; PackResourceEditorReadsTest pauses the editor's first read while its tab is left and the file is saved elsewhere. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../pack/ResourceEdits.java | 11 +- .../ui/components/PageLoader.java | 33 ++++-- .../editors/PackResourceEditor.java | 16 +++ .../components/editors/ResourceViewPanel.java | 5 + .../pack/ResourceEditsTest.java | 16 +++ .../ui/components/PageLoaderTest.java | 48 ++++++++ .../editors/PackResourceEditorReadsTest.java | 103 +++++++++++++++++ .../editors/ResourceTabReadsTest.java | 108 ++++++++++++++++++ .../editors/ResourceTextEditorTest.java | 33 ++++++ docs/SYSTEMS.md | 3 +- 10 files changed, 361 insertions(+), 15 deletions(-) create mode 100644 companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditorReadsTest.java create mode 100644 companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceTabReadsTest.java diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java index 0ec9ee097..8a1b6fe3a 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java @@ -137,14 +137,19 @@ public void close() { this.external.close(); } - /** Fires after each save or revert has finished, the managed pack enabled and the game reloaded. */ + /** + * Fires after each save, revert or adoption that wrote, once the managed pack is enabled and the game reloaded; one + * that failed or found nothing to write leaves the packs as they were and tells nothing. + */ public Signal edited() { return this.edited; } - /** Fires {@link #edited()} once {@code edit} has finished, whether it worked or not. */ + /** Fires {@link #edited()} once {@code edit} has written. */ private CompletableFuture finished(CompletableFuture edit) { - return edit.whenComplete((ignored, failure) -> this.edited.fire()); + return edit.whenComplete((saved, failure) -> { + if (failure == null && saved != null) this.edited.fire(); + }); } /** diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java index d69cecb52..85dae1a68 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoader.java @@ -8,6 +8,7 @@ import java.awt.event.HierarchyListener; import java.util.ArrayList; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Objects; import java.util.Set; @@ -53,15 +54,20 @@ public interface Read { private boolean disposed; /** The page whose visibility decides when a followed change loads, or null to load at once. */ private JComponent page; + /** A change the page reads whatever it is, such as its first read or one it asked for. */ + private static final BooleanSupplier ALWAYS = () -> true; + /** - * What the page missed while it was hidden or held: for each followed change, whether it concerns the page, asked when - * the page would read. Empty when there is nothing to read. + * What the page missed while it was hidden or held: for each followed source that changed, whether that concerns the + * page, asked when the page would read; once per source however often it changed. Empty when there is nothing to read. */ - private final List missed = new ArrayList<>(); + private final Set missed = new LinkedHashSet<>(); /** The followed signals fired since the last read started, which that read's preparation may ask about. */ private final Set fired = new HashSet<>(); /** Whether the page holds its reads, as while it saves. */ private boolean held; + /** Whether no read runs while the page is hidden, whoever asks for it: set by {@link #page}. */ + private boolean waitsForShow; /** How many reads started. */ private int reads; /** Removes the listeners on the components whose showing this loader watches. */ @@ -85,7 +91,8 @@ public PageLoader(Read read, Consumer show, Consumer fail) { */ public PageLoader page(JComponent page) { this.page = Objects.requireNonNull(page, "page"); - this.missed.add(() -> true); + this.waitsForShow = true; + this.missed.add(ALWAYS); watch(page, false); if (page.isShowing()) SwingUtilities.invokeLater(this::resume); return this; @@ -93,7 +100,7 @@ public PageLoader page(JComponent page) { /** Reads again whenever {@code signal} fires. */ public PageLoader follows(Signal signal) { - return follows(signal, () -> true); + return follows(signal, ALWAYS); } /** @@ -166,7 +173,7 @@ private PageLoader watch(JComponent component, boolean always) { * and returns what removes it, as {@code catalog.changed()::subscribe} does; the listener may be called on any thread. */ public PageLoader follow(Function subscribe) { - this.unsubscribe.add(subscribe.apply(() -> SwingUtilities.invokeLater(() -> changed(() -> true)))); + this.unsubscribe.add(subscribe.apply(() -> SwingUtilities.invokeLater(() -> changed(ALWAYS)))); return this; } @@ -201,7 +208,7 @@ public void hold() { this.held = true; if (this.running || this.again) { // What that read was for is read again once released. - this.missed.add(() -> true); + this.missed.add(ALWAYS); cancel(); } } @@ -212,15 +219,19 @@ public void release() { resume(); } - /** Reads again, now or once the running read has finished; while held, once released. */ + /** + * Reads again, now or once the running read has finished. A page named with {@link #page} reads only while it is + * shown and not held, whoever asks, the page itself too, as to check a read against a later change: otherwise it reads + * once it is shown and released. + */ public void load() { if (!SwingUtilities.isEventDispatchThread()) { SwingUtilities.invokeLater(this::load); return; } if (this.disposed) return; - if (this.held) { - this.missed.add(() -> true); + if (this.held || this.waitsForShow && !this.page.isShowing()) { + this.missed.add(ALWAYS); return; } this.missed.clear(); @@ -264,7 +275,7 @@ private void showRead(T value, Throwable failure) { return; } // A page shown again after a failed read tries once more, as when the file was being written. - if (this.page != null) this.missed.add(() -> true); + if (this.page != null) this.missed.add(ALWAYS); this.fail.accept(failure instanceof CompletionException && failure.getCause() != null ? failure.getCause() : failure); } diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java index e4b843853..35726b57e 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditor.java @@ -277,6 +277,16 @@ JComboBox targetBox() { return this.target; } + /** How many times the tab read the pack's copies, which tests count. */ + int reads() { + return this.loader.reads(); + } + + /** The read of the copies that runs or ran last, for tests. */ + CompletableFuture reading() { + return this.loader.current(); + } + String noticeText() { return this.notice.getText(); } @@ -479,6 +489,12 @@ private boolean save(Consumer after) { // Encoding a large texture takes a while, so it runs with the rest of the save, and so does its hash. CompletableFuture.supplyAsync(() -> { try { + // The working pack or the world may have changed since the tab read its pack, before the tab could hear of + // it: the save goes nowhere else than where the tab saves now, and the tab reads that pack's copy instead. + Path now = this.opened != null ? this.opened : this.edits.pack(this.path); + if (!now.equals(into)) { + throw new IOException("the " + noun() + " is saved into the " + PackFolders.label(now) + " now; its copy is read"); + } byte[] bytes = encode(edited); written[0] = ResourceOriginals.hash(bytes); return bytes; diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceViewPanel.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceViewPanel.java index f4e796df4..8421cc7c7 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceViewPanel.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceViewPanel.java @@ -48,6 +48,11 @@ private record Opened(LoadedResource content, Path pack) { /** The folder pack the loaded file lies in, which its editor saves into, or null for a file of a mod or archive. */ private Path openedPack; private Component activeView; + + /** The editor the panel shows, or null while it shows the resource read-only, for tests. */ + PackResourceEditor editor() { + return this.activeView instanceof PackResourceEditor editor ? editor : null; + } /** Where to show the text once it has loaded, or -1. */ private int pendingOffset = -1; /** When a local file was last read, so an offset into a file written since reads it again first; null otherwise. */ diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEditsTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEditsTest.java index 28f5fffa7..18e65e2ff 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEditsTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEditsTest.java @@ -112,6 +112,22 @@ void aRevertLeavesAFileChangedOutsideCompanionAlone() throws Exception { assertEquals(0, record.size()); } + @Test + void aSaveThatWritesNothingTellsNothing() throws Exception { + ResourceEdits edits = edits(ChangeRecord.inMemory()); + edits.packs().named(new ClientPacksPayload(STACK, 48)); + edits.save(LANG, bytes("{}")).get(5, TimeUnit.SECONDS); + AtomicInteger told = new AtomicInteger(); + edits.edited().subscribe(told::incrementAndGet); + + Path pack = edits.pack(LANG); + assertThrows(ExecutionException.class, () -> edits.save(LANG, pack, bytes("{\"a\":1}"), Map.of(), "not what the pack holds") + .get(5, TimeUnit.SECONDS), "a save over a copy changed since is refused"); + assertEquals(0, told.get(), "the packs are as they were, so the views that show them read nothing"); + edits.save(LANG, bytes("{\"a\":2}")).get(5, TimeUnit.SECONDS); + assertEquals(1, told.get()); + } + @Test void editListenersHearOfASaveOnceTheOptionsEnableThePack() throws Exception { Path options = Files.writeString(this.directory.resolve("options.txt"), "resourcePacks:[\"vanilla\"]\n"); diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java index 307fa9f66..c19e7c949 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java @@ -310,6 +310,54 @@ void aReadCanTellWhichOfItsSignalsLedToIt() throws Exception { assertEquals(List.of("", "record", "packs"), causes, "each read knows what it is for, and no more"); } + @Test + void aPageHiddenWhileItReadsReadsNothingMoreUntilShownWhoeverAsks() throws Exception { + ShowablePage page = new ShowablePage(); + Signal signal = new Signal(); + CountDownLatch release = new CountDownLatch(1); + AtomicInteger reads = new AtomicInteger(); + PageLoader loader = onEdt(() -> new PageLoader(() -> { + this.prepared.incrementAndGet(); + return () -> { + if (reads.incrementAndGet() == 1) release.await(5, TimeUnit.SECONDS); + return "read"; + }; + }, read -> { }, failure -> { }).page(page).follows(signal)); + SwingUtilities.invokeAndWait(() -> page.setShown(true)); + SwingUtilities.invokeAndWait(() -> { }); + assertEquals(1, this.prepared.get(), "the first read runs"); + + SwingUtilities.invokeAndWait(() -> page.setShown(false)); + fire(signal); + // As a page checking its read against a later change asks for another, and a request during the read does. + SwingUtilities.invokeAndWait(loader::load); + release.countDown(); + settle(loader); + SwingUtilities.invokeAndWait(() -> { }); + assertEquals(1, this.prepared.get(), "hidden, the page reads nothing more, whoever asked"); + + show(page, loader, true); + settle(loader); + assertEquals(2, this.prepared.get(), "shown, it reads once for all that came meanwhile"); + } + + @Test + void manyChangesWhileHiddenAreAskedAboutOncePerSource() throws Exception { + ShowablePage page = new ShowablePage(); + Signal signal = new Signal(); + AtomicInteger asked = new AtomicInteger(); + PageLoader loader = onEdt(() -> loader().page(page).follows(signal, () -> { + asked.incrementAndGet(); + return false; + })); + show(page, loader, true); + show(page, loader, false); + for (int change = 0; change < 1_000; change++) signal.fire(); + SwingUtilities.invokeAndWait(() -> { }); + show(page, loader, true); + assertEquals(1, asked.get(), "a hidden page keeps one question per source, however often it changed"); + } + private static void fire(Signal signal) throws Exception { signal.fire(); SwingUtilities.invokeAndWait(() -> { }); diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditorReadsTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditorReadsTest.java new file mode 100644 index 000000000..ea39c250e --- /dev/null +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/PackResourceEditorReadsTest.java @@ -0,0 +1,103 @@ +package com.github.minecraft_ta.totalDebugCompanion.ui.components.editors; + +import com.github.minecraft_ta.totalDebugCompanion.game.GameLocations; +import com.github.minecraft_ta.totalDebugCompanion.pack.ResourceEdits; +import com.github.minecraft_ta.totalDebugCompanion.pack.ResourceEditsFixture; +import com.github.minecraft_ta.totalDebugCompanion.storage.ChangeRecord; +import com.github.minecraft_ta.totalDebugCompanion.storage.InstanceState; +import com.github.minecraft_ta.totalDebugCompanion.storage.ResourceOriginals; +import com.github.minecraft_ta.totalDebugCompanion.testui.UiTest; +import com.github.minecraft_ta.totalDebugCompanion.testui.UiTestScope; +import com.github.minecraft_ta.totaldebug.protocol.message.ClientPacksPayload; +import com.github.minecraft_ta.totaldebug.protocol.message.PackStackPayload; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import javax.swing.JPanel; +import javax.swing.SwingUtilities; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Path; +import java.util.List; +import java.util.Objects; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * The resource editor's reads, with a read paused while something else happens, as docs/SYSTEMS.md asks: a hidden tab + * reads nothing more, also to check its read against a later change, and reads once when shown again. + */ +@UiTest +class PackResourceEditorReadsTest { + private static final String LANG = "assets/testmod/lang/en_us.json"; + + @TempDir Path directory; + + @Test + void aTabHiddenWhileItReadsChecksAChangeElsewhereOnlyWhenShownAgain() throws Exception { + ResourceEdits edits = ResourceEditsFixture.edits(GameLocations.of(this.directory, false), ChangeRecord.inMemory(), + new ResourceOriginals(this.directory.resolve("total-debug/originals")), Runnable::run, InstanceState.inMemory()); + edits.packs().named(new ClientPacksPayload(new PackStackPayload(34, List.of(new PackStackPayload.Pack(ResourceEdits.PACK_ID, "TotalDebug", ""))), 48)); + edits.save(LANG, "{\"a\":\"saved\"}".getBytes(StandardCharsets.UTF_8)).get(5, TimeUnit.SECONDS); + + GatedEditor[] editor = new GatedEditor[1]; + SwingUtilities.invokeAndWait(() -> editor[0] = new GatedEditor(edits)); + try { + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(editor[0])); + UiTestScope.await(() -> editor[0].decodes.get() == 1); + + // The first read waits in its decode while the tab is left and another tab saves the same file. + SwingUtilities.invokeAndWait(() -> editor[0].setVisible(false)); + edits.save(LANG, "{\"a\":\"elsewhere\"}".getBytes(StandardCharsets.UTF_8)).get(5, TimeUnit.SECONDS); + editor[0].gate.countDown(); + editor[0].reading().get(5, TimeUnit.SECONDS); + for (int step = 0; step < 3; step++) SwingUtilities.invokeAndWait(() -> { }); + Thread.sleep(200); + assertEquals(1, editor[0].decodes.get(), "hidden, the tab reads nothing more, also to check its read"); + + SwingUtilities.invokeAndWait(() -> editor[0].setVisible(true)); + UiTestScope.await(() -> "{\"a\":\"elsewhere\"}".equals(editor[0].content)); + assertEquals(2, editor[0].decodes.get(), "shown again, it reads the other tab's save once"); + } finally { + SwingUtilities.invokeAndWait(() -> editor[0].dispose()); + } + } + + /** An editor of text whose first decode waits until the test lets it go. */ + private static final class GatedEditor extends PackResourceEditor { + final CountDownLatch gate = new CountDownLatch(1); + final AtomicInteger decodes = new AtomicInteger(); + volatile String content = "{}"; + private String saved = "{}"; + + GatedEditor(ResourceEdits edits) { + super(LANG, "testmod.jar", null, "{}", edits); + start(new JPanel()); + } + + @Override protected String shown() { return this.content; } + @Override protected boolean same(String first, String second) { return Objects.equals(first, second); } + @Override protected void load(String content) { this.content = content; this.saved = content; } + @Override protected void markSaved(String content) { this.saved = content; } + @Override protected boolean modified() { return !this.content.equals(this.saved); } + @Override protected String none() { return ""; } + @Override protected String noun() { return "text"; } + @Override protected void setEditable(boolean editable) { } + @Override protected byte[] encode(String content) { return content.getBytes(StandardCharsets.UTF_8); } + + @Override + protected String decode(byte[] bytes) throws IOException { + if (this.decodes.incrementAndGet() == 1) { + try { + this.gate.await(5, TimeUnit.SECONDS); + } catch (InterruptedException interrupted) { + Thread.currentThread().interrupt(); + } + } + return new String(bytes, StandardCharsets.UTF_8); + } + } +} diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceTabReadsTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceTabReadsTest.java new file mode 100644 index 000000000..cabcdd73f --- /dev/null +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceTabReadsTest.java @@ -0,0 +1,108 @@ +package com.github.minecraft_ta.totalDebugCompanion.ui.components.editors; + +import com.github.minecraft_ta.totalDebugCompanion.CompanionApplication; +import com.github.minecraft_ta.totalDebugCompanion.GlobalConfig; +import com.github.minecraft_ta.totalDebugCompanion.navigation.NavigationService; +import com.github.minecraft_ta.totalDebugCompanion.pack.ResourceEdits; +import com.github.minecraft_ta.totalDebugCompanion.navigation.NavigationTarget; +import com.github.minecraft_ta.totalDebugCompanion.project.ProjectScope; +import com.github.minecraft_ta.totalDebugCompanion.session.CompanionLaunchConfiguration; +import com.github.minecraft_ta.totalDebugCompanion.session.CompanionProfile; +import com.github.minecraft_ta.totalDebugCompanion.testui.UiTest; +import com.github.minecraft_ta.totalDebugCompanion.testui.UiTestScope; +import com.github.minecraft_ta.totalDebugCompanion.ui.views.MainWindow; +import com.github.minecraft_ta.totaldebug.protocol.message.ClientPacksPayload; +import com.github.minecraft_ta.totaldebug.protocol.message.PackStackPayload; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import javax.swing.SwingUtilities; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.concurrent.TimeUnit; +import java.util.zip.ZipEntry; +import java.util.zip.ZipOutputStream; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * A mod's resource opened the way a click opens it reads its pack's copy once, and again only after a change it missed + * (docs/SYSTEMS.md, Tests): built by the application, opened through navigation, shown in the window. + */ +@UiTest +class ResourceTabReadsTest { + private static final String LANG = "assets/testmod/lang/en_us.json"; + + @TempDir Path directory; + + @Test + void aResourceTabReadsOnceWhenOpenedAndAgainOnlyAfterAChangeItMissed() throws Exception { + Path home = Files.createDirectory(this.directory.resolve("home")); + GlobalConfig.getInstance().loadFrom(home); + Path game = Files.createDirectory(this.directory.resolve("game")); + Path jar = this.directory.resolve("testmod.jar"); + try (ZipOutputStream zip = new ZipOutputStream(Files.newOutputStream(jar))) { + zip.putNextEntry(new ZipEntry(LANG)); + zip.write("{\"a\":\"jar\"}".getBytes(StandardCharsets.UTF_8)); + zip.closeEntry(); + } + try (CompanionApplication app = new CompanionApplication(new CompanionLaunchConfiguration(home), "test-token")) { + app.openProject(CompanionProfile.forGame(game)).get(10, TimeUnit.SECONDS); + ProjectScope scope = app.currentScope(); + // As the connected game names its packs, whose format a first save into the managed pack needs. + scope.packs().named(new ClientPacksPayload(new PackStackPayload(34, + List.of(new PackStackPayload.Pack(ResourceEdits.PACK_ID, "TotalDebug", ""))), 48)); + MainWindow window = UiTestScope.onEdt(app::createWindow); + UiTestScope.onEdt(() -> { + window.setSize(1280, 720); + UiTestScope.show(window); + }); + NavigationTarget resource = new NavigationTarget.ArchiveEntry(jar, LANG); + + open(window, resource); + PackResourceEditor editor = UiTestScope.onEdt(() -> + ((ResourceViewPanel) window.getEditorTabs().getSelectedEditor().getComponent()).editor()); + assertNotNull(editor, "a mod's language file opens in its editor"); + UiTestScope.await(() -> editor.reads() == 1); + editor.reading().get(5, TimeUnit.SECONDS); + settle(); + assertEquals(1, reads(editor), "opening the tab reads the pack's copy once"); + + open(window, resource); + assertEquals(1, reads(editor), "navigating to the tab it shows reads nothing"); + + open(window, new NavigationTarget.Changes()); + scope.resources().save(LANG, "{\"a\":\"elsewhere\"}".getBytes(StandardCharsets.UTF_8)).get(10, TimeUnit.SECONDS); + settle(); + assertEquals(1, reads(editor), "a hidden tab does not read"); + open(window, resource); + UiTestScope.await(() -> editor.reads() == 2); + editor.reading().get(5, TimeUnit.SECONDS); + settle(); + assertEquals(2, reads(editor), "shown again, it reads the save it missed once"); + + open(window, new NavigationTarget.Changes()); + open(window, resource); + assertEquals(2, reads(editor), "shown again without a change, it reads nothing"); + } + } + + private static void open(MainWindow window, NavigationTarget target) throws Exception { + window.navigation().navigate(target, NavigationService.Activation.KEEP_CURRENT_WINDOW).get(10, TimeUnit.SECONDS); + settle(); + } + + private static int reads(PackResourceEditor editor) throws Exception { + return UiTestScope.onEdt(editor::reads); + } + + /** Lets the Swing steps queued by showing and reading run, and a read they started finish. */ + private static void settle() throws Exception { + for (int step = 0; step < 3; step++) SwingUtilities.invokeAndWait(() -> { }); + Thread.sleep(200); + SwingUtilities.invokeAndWait(() -> { }); + } +} diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceTextEditorTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceTextEditorTest.java index 3094a5e00..06eb93dd9 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceTextEditorTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/editors/ResourceTextEditorTest.java @@ -127,6 +127,39 @@ void aSaveRightAfterChoosingAnotherPackGoesIntoThatPack() throws Exception { } } + @Test + void aSaveAfterTheWorkingPackMovedBeforeTheTabHeardGoesIntoNoPack() throws Exception { + InstanceState state = InstanceState.inMemory(); + ResourceEdits edits = ResourceEditsFixture.edits(GameLocations.of(this.directory, false), ChangeRecord.inMemory(), + new ResourceOriginals(this.directory.resolve("total-debug/originals")), Runnable::run, state); + edits.packs().named(new ClientPacksPayload(new PackStackPayload(34, List.of(new PackStackPayload.Pack(ResourceEdits.PACK_ID, "TotalDebug", ""))), 48)); + Path mine = Files.createDirectories(this.directory.resolve("resourcepacks/MyPack")); + Files.writeString(mine.resolve("pack.mcmeta"), "{\"pack\":{\"pack_format\":34,\"description\":\"\"}}"); + Path managed = edits.pack(LANG); + String inJar = "{\"a\":\"jar\"}"; + + ResourceTextEditor[] editor = new ResourceTextEditor[1]; + SwingUtilities.invokeAndWait(() -> editor[0] = new ResourceTextEditor(LANG, "testmod.jar", null, + new LoadedResource.Text(inJar, "text/json", "UTF-8", inJar.length()), edits)); + SwingUtilities.invokeAndWait(() -> UiTestScope.showPages(editor[0])); + try { + awaitOnSwing(() -> editor[0].targetBox().getItemCount() == 2); + // Another tab chose the player's pack, and this tab has not heard of it yet, as while one of its reads runs. + state.setWorkingPack(ResourceEdits.side(LANG), "MyPack"); + SwingUtilities.invokeAndWait(() -> { + editor[0].textPanel().editorPane.setText("{\"a\":\"edited\"}"); + editor[0].save(); + }); + // The save is refused, and the tab reads the pack it saves into now, keeping the edit unsaved. + awaitOnSwing(() -> mine.equals(editor[0].targetBox().getSelectedItem())); + SwingUtilities.invokeAndWait(() -> assertTrue(editor[0].textPanel().modified(), "the edit is still there, unsaved")); + assertFalse(Files.exists(managed.resolve(LANG)), "nothing is saved into the pack the tab showed"); + assertFalse(Files.exists(mine.resolve(LANG)), "nor into the one it had not read yet"); + } finally { + SwingUtilities.invokeAndWait(editor[0]::dispose); + } + } + @Test void theWarningThatTheGameDoesNotUseTheCopyEndsWhenItDoes() throws Exception { ResourceEdits edits = ResourceEditsFixture.edits(GameLocations.of(this.directory, false), ChangeRecord.inMemory(), diff --git a/docs/SYSTEMS.md b/docs/SYSTEMS.md index 4317eaea7..208fa1605 100644 --- a/docs/SYSTEMS.md +++ b/docs/SYSTEMS.md @@ -113,10 +113,11 @@ this.loader = new PageLoader<>(this::read, this::show, this::fail) ``` - **It reads when the page is first shown**, never in a constructor and never because of a navigation. -- **While the page is hidden, it only notes that a followed signal fired**, and reads once when the page is shown again. A page shown again with nothing changed reads nothing, unless its last read failed, as for a file read while it was written. +- **While the page is hidden, it only notes that a followed signal fired**, once per source however often it fired, and reads once when the page is shown again. No read runs for a hidden page, whoever asks, the page itself too, as to check a finished read against a later change. A page shown again with nothing changed reads nothing, unless its last read failed, as for a file read while it was written. - **One read at a time, newest wins.** The loader shows every result it gets; it does not compare results, since owners only signal real changes. - **Reads run on file work, results are shown on the Swing thread.** - **`show` keeps what the user chose:** the selection and scroll position of rows that are still there (`Tables.keepingSelection`), and a selection a navigation asked for, such as a key binding to show, which the page keeps until a result contains it. +- **A write checks where it goes when it is made**, not by what the page last read: the resource editor's save refuses a pack the tab no longer saves into, as the change pipeline refuses a copy changed since. - **A page that writes holds its reads** (`hold`, `release`): while its save runs, a read under way is not shown, since it may predate the write, and followed changes wait as while the page is hidden; released, the page reads once what concerned it meanwhile. A page with unsaved changes that must not be replaced by a read (`ConfigPanel`) holds its reads the same way while they last. A page does not read again because it saved; it hears the owner's signal like every other follower. - **A follow may say whether a change concerns the page**, asked when the page would read: the resource editor follows the whole change record, but only its own file's entry, and not its own save's. A read's preparation may also ask which followed signals led to it (`fired`): the resource editor lets Save wait only for a read after a pack change, which may move it to another pack, not for one after an edit elsewhere. - **A read never changes an owner.** What the Changes page's labels do today with `ChangeRecord.observed` moves to the owners, which notice a value put back outside Companion on their own readings, on the write queue. From c150e7484c7be8704267b7f4bc3a32f5b217616b Mon Sep 17 00:00:00 2001 From: Pelotrio <45769595+Pelotrio@users.noreply.github.com> Date: Wed, 30 Sep 2026 12:22:44 +0200 Subject: [PATCH 06/12] ResourceEdits: a save that failed while writing still tells its edits A texture's animation lands before its image; when the image then fails, what landed stays and is recorded, so the resource listing must read it. Only a save refused before it wrote, as over a copy changed since, or one that found nothing to write, tells nothing. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../totalDebugCompanion/pack/ResourceEdits.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java index 8a1b6fe3a..e22cb2de9 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java @@ -138,17 +138,21 @@ public void close() { } /** - * Fires after each save, revert or adoption that wrote, once the managed pack is enabled and the game reloaded; one - * that failed or found nothing to write leaves the packs as they were and tells nothing. + * Fires after each save, revert or adoption that may have written, once the managed pack is enabled and the game + * reloaded. One refused before it wrote, as over a copy changed since, or that found nothing to write, tells nothing; + * one that failed while writing tells, since what landed before stays, as a texture's animation beside it. */ public Signal edited() { return this.edited; } - /** Fires {@link #edited()} once {@code edit} has written. */ + /** Fires {@link #edited()} once {@code edit} has finished, unless it wrote nothing. */ private CompletableFuture finished(CompletableFuture edit) { return edit.whenComplete((saved, failure) -> { - if (failure == null && saved != null) this.edited.fire(); + Throwable cause = failure; + while (cause instanceof CompletionException && cause.getCause() != null) cause = cause.getCause(); + boolean wrote = failure == null ? saved != null : !(cause instanceof ChangePipeline.Stale); + if (wrote) this.edited.fire(); }); } From 0a5e2412bd96c8e6fb0ed8b5fd68a2760a964d7e Mon Sep 17 00:00:00 2001 From: Pelotrio <45769595+Pelotrio@users.noreply.github.com> Date: Wed, 30 Sep 2026 12:37:14 +0200 Subject: [PATCH 07/12] ResourceEdits tells its edits by the files that landed, not by how an edit ended The last two rounds guessed from the outcome: a failure told nothing, then every failure but a refusal told. Both were wrong somewhere, a texture's animation landing before its image failed, or a save failing before it wrote. ResourceEdits now counts each file it writes into a pack or the options, and each program's save it takes, and an edit tells when that count moved while it ran. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../pack/ResourceEdits.java | 91 ++++++++++--------- 1 file changed, 50 insertions(+), 41 deletions(-) diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java index e22cb2de9..20079cbe7 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/pack/ResourceEdits.java @@ -34,6 +34,7 @@ import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; +import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.Executor; @@ -85,6 +86,8 @@ public record Saved(Effect effect, Path pack, List problems, String relo private final InstanceState state; /** Run after a save or revert has written its file and the game used it, or failed to. */ private final Signal edited = new Signal(); + /** Counts the files Companion wrote into packs and the options, so an edit tells only what landed. */ + private final AtomicLong landed = new AtomicLong(); /** Fired by side when its working pack is chosen, which changes where resource tabs save. */ private final Map workingPackChosen = Map.of("assets", new Signal(), "data", new Signal()); /** The hash of what Companion last wrote to each resource, by the write queue, so a program's save is told from it. */ @@ -138,21 +141,19 @@ public void close() { } /** - * Fires after each save, revert or adoption that may have written, once the managed pack is enabled and the game - * reloaded. One refused before it wrote, as over a copy changed since, or that found nothing to write, tells nothing; - * one that failed while writing tells, since what landed before stays, as a texture's animation beside it. + * Fires after each save, revert or adoption that wrote a file, once the managed pack is enabled and the game reloaded, + * also where it failed after a first file landed, as a texture's animation beside it. One that wrote nothing tells + * nothing. */ public Signal edited() { return this.edited; } - /** Fires {@link #edited()} once {@code edit} has finished, unless it wrote nothing. */ - private CompletableFuture finished(CompletableFuture edit) { - return edit.whenComplete((saved, failure) -> { - Throwable cause = failure; - while (cause instanceof CompletionException && cause.getCause() != null) cause = cause.getCause(); - boolean wrote = failure == null ? saved != null : !(cause instanceof ChangePipeline.Stale); - if (wrote) this.edited.fire(); + /** Starts {@code edit} and fires {@link #edited()} once it has finished, where a file landed meanwhile. */ + private CompletableFuture finished(Supplier> edit) { + long before = this.landed.get(); + return edit.get().whenComplete((saved, failure) -> { + if (this.landed.get() != before) this.edited.fire(); }); } @@ -290,7 +291,7 @@ public CompletableFuture save(String path, Path into, byte[] content, Map Objects.requireNonNull(alongside, "alongside"); // The files written beside it, which the reload watches too: they may change what the game makes of the resource. List added = new CopyOnWriteArrayList<>(); - return finished(writeAndApply(path, added, () -> { + return finished(() -> writeAndApply(path, added, () -> { try { Path pack = into != null ? into : pack(path); if (managed(pack)) preparePack(pack, path.startsWith("assets/")); @@ -330,7 +331,7 @@ public CompletableFuture revert(ChangeRecord.Change change) { if (!(change.target() instanceof ChangeRecord.Resource target)) { return CompletableFuture.failedFuture(new IllegalArgumentException("Not a resource in the managed pack")); } - return finished(writeAndApply(target.path(), List.of(), () -> { + return finished(() -> writeAndApply(target.path(), List.of(), () -> { try { // Reverted already, such as twice from the Changes page before it refreshed. if (!change.equals(this.record.change(target))) return target.location(); @@ -360,36 +361,40 @@ public CompletableFuture revert(ChangeRecord.Change change) { public CompletableFuture adopt(String path, Path pack, byte[] before, byte[] seen) { ChangeRecord.Resource target = new ChangeRecord.Resource(path, pack); this.pipeline.reloads().writing(); - // Checked in the write queue, after every save and revert queued before it has written and been recorded. - CompletableFuture taken = write(() -> { - try { - Path file = pack.resolve(path); - String now = ResourceOriginals.hash(Files.isRegularFile(file) ? Files.readAllBytes(file) : null); - // Changed again since it was found whole: the program's next write is taken instead. - if (!now.equals(ResourceOriginals.hash(seen))) return false; - ChangeRecord.Change change = this.record.change(target); - // Without a change, what Companion wrote last is the original it put back. - String known = change == null ? this.lastWritten.get(target) : null; - String expected = change != null ? change.current() : known != null ? known : ResourceOriginals.hash(before); - if (expected.equals(now)) return false; - if (change == null) this.originals.keep(known != null ? this.originals.read(known) : before); - this.lastWritten.put(target, now); - this.record.changed(target, expected, now); - return true; - } catch (IOException exception) { - throw new CompletionException(exception); - } + return finished(() -> { + // Checked in the write queue, after every save and revert queued before it has written and been recorded. + CompletableFuture taken = write(() -> { + try { + Path file = pack.resolve(path); + String now = ResourceOriginals.hash(Files.isRegularFile(file) ? Files.readAllBytes(file) : null); + // Changed again since it was found whole: the program's next write is taken instead. + if (!now.equals(ResourceOriginals.hash(seen))) return false; + ChangeRecord.Change change = this.record.change(target); + // Without a change, what Companion wrote last is the original it put back. + String known = change == null ? this.lastWritten.get(target) : null; + String expected = change != null ? change.current() : known != null ? known : ResourceOriginals.hash(before); + if (expected.equals(now)) return false; + if (change == null) this.originals.keep(known != null ? this.originals.read(known) : before); + this.lastWritten.put(target, now); + this.record.changed(target, expected, now); + // The program wrote it; taken into the pack, it counts as written. + this.landed.incrementAndGet(); + return true; + } catch (IOException exception) { + throw new CompletionException(exception); + } + }); + return taken.handle((changed, failure) -> { + try { + if (failure != null) return CompletableFuture.failedFuture(failure); + if (!changed) return CompletableFuture.completedFuture(null); + return apply(path, List.of(), pack).thenApply(saved -> new Saved(saved.effect(), saved.pack(), saved.problems(), + saved.reloadFailure(), this.packs.unusedBecause(path, saved.pack()).orElse(""))); + } finally { + this.pipeline.reloads().written(); + } + }).thenCompose(Function.identity()); }); - return finished(taken.handle((changed, failure) -> { - try { - if (failure != null) return CompletableFuture.failedFuture(failure); - if (!changed) return CompletableFuture.completedFuture(null); - return apply(path, List.of(), pack).thenApply(saved -> new Saved(saved.effect(), saved.pack(), saved.problems(), - saved.reloadFailure(), this.packs.unusedBecause(path, saved.pack()).orElse(""))); - } finally { - this.pipeline.reloads().written(); - } - }).thenCompose(Function.identity())); } /** @@ -461,6 +466,7 @@ public void writeFile(List> writes, Consume if (write.value() == null) Files.deleteIfExists(file); else AtomicFiles.replace(file, staged -> Files.write(staged, write.value())); lastWritten.put(write.target(), text(write.value())); + ResourceEdits.this.landed.incrementAndGet(); landed.accept(write.target()); } } @@ -647,11 +653,13 @@ private void enableOffline() throws IOException { packs.add(PACK_ID); lines.set(index, "resourcePacks:" + packs); AtomicFiles.writeString(options, String.join("\n", lines) + "\n"); + this.landed.incrementAndGet(); this.packs.written(ChangeRecord.PackSide.RESOURCES); return; } lines.add("resourcePacks:[\"vanilla\",\"" + PACK_ID + "\"]"); AtomicFiles.writeString(options, String.join("\n", lines) + "\n"); + this.landed.incrementAndGet(); this.packs.written(ChangeRecord.PackSide.RESOURCES); } @@ -669,6 +677,7 @@ private void preparePack(Path pack, boolean assets) throws IOException { JsonObject json = new JsonObject(); json.add("pack", description); AtomicFiles.writeString(meta, json + "\n"); + this.landed.incrementAndGet(); this.packs.written(assets ? ChangeRecord.PackSide.RESOURCES : ChangeRecord.PackSide.DATA); } } From b72928fb767bbeb903f0ad89afba20b6f3cf658d Mon Sep 17 00:00:00 2001 From: Pelotrio <45769595+Pelotrio@users.noreply.github.com> Date: Wed, 30 Sep 2026 12:49:45 +0200 Subject: [PATCH 08/12] KeyAssignments watches the game's folder again once it exists A folder removed while watched, or missing when the project opened, left the keys read before on screen: its watch had ended, yet the page took the keys as watched. KeyAssignments now tries the watch again after 1 and 5 seconds and every 30 seconds after, reads the keys fresh whenever asked meanwhile, and once watching reads and tells what changed. A read on request is not what the next change is told against, so every follower hears it. The page's own read-when-shown fallback goes. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../catalog/KeyAssignments.java | 92 +++++++++++-------- .../catalog/KeyBindingControl.java | 5 - .../components/catalog/KeyBindingsPanel.java | 2 - .../catalog/KeyAssignmentsTest.java | 15 +-- 4 files changed, 62 insertions(+), 52 deletions(-) diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java index 466a741e9..396f13875 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java @@ -29,6 +29,7 @@ public final class KeyAssignments implements AutoCloseable { private static final long SETTLE_MILLIS = 300; private static final List RETRY_MILLIS = List.of(1_000L, 5_000L, 30_000L); + private static final System.Logger LOGGER = System.getLogger(KeyAssignments.class.getName()); private final Path options; private final Signal changed = new Signal(); @@ -36,8 +37,13 @@ public final class KeyAssignments implements AutoCloseable { .daemon() .name("Companion options.txt") .unstarted(task)); - /** Null where the game's folder cannot be watched, as before it exists. */ + /** Null only where the system has no file watching at all. */ private final WatchService watcher; + /** + * Whether the game's folder is watched now: not before it exists, or after it was removed; then its watch is tried + * again after 1 and 5 seconds and every 30 seconds after. + */ + private volatile boolean watching; private ScheduledFuture pending; /** Counts writes seen; only a read for the latest may tell, so a retry of an earlier one cannot read a write in parts. */ private long generation; @@ -46,55 +52,61 @@ public final class KeyAssignments implements AutoCloseable { /** Whether the last read failed: a page may show that failure, so the next read that succeeds is told. */ private boolean unreadable; - /** - * Watches {@code options}. Where its folder, the game's, cannot be watched, as before the game first ran, only - * Companion's own writes are told, and {@link #assignments()} reads the file each time. - */ + /** Watches {@code options}, whose folder, the game's, may not exist yet: it is watched once it does. */ public KeyAssignments(Path options) { this.options = Objects.requireNonNull(options, "options").toAbsolutePath().normalize(); - this.watcher = watcher(this.options.getParent()); - if (this.watcher != null) Thread.ofPlatform().daemon().name("Companion options.txt watcher").start(this::watch); + this.watcher = watchService(); + if (this.watcher != null) { + Thread.ofPlatform().daemon().name("Companion options.txt watcher").start(this::watch); + if (!register()) registerLater(0); + } this.timer.execute(() -> read(0, 0)); } - /** Whether writes of others are seen; without, a page reads {@link #assignments()} whenever it is shown. */ - public boolean watched() { - return this.watcher != null; - } - /** - * The keys the file assigns: as last read while it is watched, and read now where it has not been read yet or is - * not watched. Blocking where it reads. + * The keys the file assigns: as last read while its folder is watched, and read now where it has not been read yet + * or is not watched, since then a change would go unseen. A read made here is not what the next change is told + * against: only this owner's own reads are, so every follower hears of a change it has not seen. Blocking where it + * reads. */ public Map assignments() throws IOException { synchronized (this) { - if (this.read != null && this.watcher != null) return this.read; + if (this.read != null && this.watching) return this.read; } - Map now = KeyBindings.readOptions(this.options); - synchronized (this) { - if (this.read == null || this.watcher == null) this.read = now; - return this.read; + return KeyBindings.readOptions(this.options); + } + + private static WatchService watchService() { + try { + return FileSystems.getDefault().newWatchService(); + } catch (IOException | RuntimeException unavailable) { + LOGGER.log(System.Logger.Level.WARNING, "Key assignments are read when asked, not watched: " + unavailable.getMessage()); + return null; } } - private static WatchService watcher(Path folder) { - WatchService watcher = null; + /** Watches the game's folder; false where it cannot be, as before it exists. */ + private boolean register() { try { - watcher = FileSystems.getDefault().newWatchService(); - folder.register(watcher, StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_MODIFY, - StandardWatchEventKinds.ENTRY_DELETE); - return watcher; + this.options.getParent().register(this.watcher, StandardWatchEventKinds.ENTRY_CREATE, + StandardWatchEventKinds.ENTRY_MODIFY, StandardWatchEventKinds.ENTRY_DELETE); + this.watching = true; + return true; } catch (IOException | RuntimeException unwatchable) { - System.getLogger(KeyAssignments.class.getName()).log(System.Logger.Level.WARNING, - "Key assignments in " + folder + " are read when their page is shown, not watched: " + unwatchable.getMessage()); - if (watcher != null) { - try { - watcher.close(); - } catch (IOException ignored) { - // Nothing was registered with it. - } - } - return null; + return false; + } + } + + /** Tries to watch the game's folder again after a while, then reads what the file assigns by then. */ + private void registerLater(int attempt) { + long delay = RETRY_MILLIS.get(Math.min(attempt, RETRY_MILLIS.size() - 1)); + try { + this.timer.schedule(() -> { + if (register()) written(); + else registerLater(attempt + 1); + }, delay, TimeUnit.MILLISECONDS); + } catch (RuntimeException closed) { + // Closed with the project. } } @@ -120,7 +132,12 @@ private void watch() { || event.context() instanceof Path name && name.equals(this.options.getFileName()); } if (ours) written(); - if (!key.reset()) return; + if (!key.reset()) { + // The folder was removed: what the file assigns is read now, and the folder watched again once it is back. + this.watching = false; + written(); + registerLater(0); + } } } catch (InterruptedException | ClosedWatchServiceException closed) { // Closed with the project. @@ -179,8 +196,7 @@ public void close() { try { this.watcher.close(); } catch (IOException failure) { - System.getLogger(KeyAssignments.class.getName()).log(System.Logger.Level.WARNING, - "The watcher of " + this.options + " did not close: " + failure.getMessage()); + LOGGER.log(System.Logger.Level.WARNING, "The watcher of " + this.options + " did not close: " + failure.getMessage()); } } } diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControl.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControl.java index 6bec98ebe..eb94e8170 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControl.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyBindingControl.java @@ -58,11 +58,6 @@ public Map assignments() throws IOException { return this.assignments.assignments(); } - /** Whether others' writes of {@code options.txt} are told; without, a page reads the keys whenever it is shown. */ - public boolean assignmentsWatched() { - return this.assignments.watched(); - } - /** The change record the bindings' changes are entered in. */ public ChangeRecord record() { return this.pipeline.record(); diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/KeyBindingsPanel.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/KeyBindingsPanel.java index cb15253d0..8c2d0ff90 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/KeyBindingsPanel.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/catalog/KeyBindingsPanel.java @@ -162,8 +162,6 @@ public void mousePressed(MouseEvent event) { this.loader = new PageLoader<>(this::prepareLoad, loaded -> show(loaded.index(), loaded.bindings(), ""), failure -> show(this.catalog.index().orElse(null), null, "Could not read options.txt: " + failure.getMessage())) .page(this).follows(catalog.changed()).follows(control.assignmentsChanged()); - // Until the game's folder can be watched, as before the game first ran, others' writes are seen when shown. - if (!control.assignmentsWatched()) this.loader.readsWhenShown(this); addHierarchyListener(event -> { if ((event.getChangeFlags() & HierarchyEvent.SHOWING_CHANGED) != 0 && !isShowing()) stopCapture(); }); diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java index 15c39740f..566e69dc6 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java @@ -12,7 +12,6 @@ import java.util.function.IntSupplier; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; class KeyAssignmentsTest { @TempDir Path directory; @@ -42,22 +41,24 @@ void aKeyReboundIsToldAndAnotherOptionIsNot() throws Exception { } @Test - void aGameFolderThatCannotBeWatchedYetReadsTheFileWhenAskedAndTellsCompanionsOwnWrites() throws Exception { + void aGameFolderThatAppearsLaterIsWatchedOnceItDoes() throws Exception { Path options = this.directory.resolve("game/options.txt"); AtomicInteger told = new AtomicInteger(); try (KeyAssignments assignments = new KeyAssignments(options)) { assignments.changed().subscribe(told::incrementAndGet); - assertFalse(assignments.watched(), "the game has not run yet, so its folder is missing"); Thread.sleep(300); + // The game runs for the first time: its folder and options.txt appear. Files.createDirectories(options.getParent()); Files.writeString(options, "key_key.jump:key.keyboard.g\n"); assertEquals(Map.of("key.jump", KeyBindings.Assignment.decode("key.keyboard.g")), assignments.assignments(), - "unwatched, the keys are read when asked, as when their page is shown"); - - Files.writeString(options, "key_key.jump:key.keyboard.h\n"); - assignments.readNow(); + "until the folder is watched, the keys are read whenever asked"); await(told::get, 1); + + replace(options, "key_key.jump:key.keyboard.h\n"); + await(told::get, 2); + assertEquals(Map.of("key.jump", KeyBindings.Assignment.decode("key.keyboard.h")), assignments.assignments(), + "watched now, a key rebound in the game is told without Companion asking"); } } From 25672f112d1b3ce95a915c7479042b653be3e24e Mon Sep 17 00:00:00 2001 From: Pelotrio <45769595+Pelotrio@users.noreply.github.com> Date: Wed, 30 Sep 2026 12:56:41 +0200 Subject: [PATCH 09/12] PageLoaderTest: showing a page waits for the read it starts The helper waited before the read that showing starts had begun, a Swing step later; on CI's slower machine that read was still running when aHeldPageReadsWhatItMissedOnceReleased held the page, which then rightly read once more after release. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../totalDebugCompanion/ui/components/PageLoaderTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java index c19e7c949..cdd3e3228 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java @@ -382,8 +382,10 @@ private void changed() throws Exception { SwingUtilities.invokeAndWait(() -> { }); } + /** Shows or hides {@code page}, and waits for a read that showing starts, which runs a Swing step later. */ private static void show(ShowablePage page, PageLoader loader, boolean shown) throws Exception { SwingUtilities.invokeAndWait(() -> page.setShown(shown)); + SwingUtilities.invokeAndWait(() -> { }); settle(loader); } From 03c0ce8dca7f5f8939bbd9efe0635a41a969729d Mon Sep 17 00:00:00 2001 From: Pelotrio <45769595+Pelotrio@users.noreply.github.com> Date: Wed, 30 Sep 2026 12:58:44 +0200 Subject: [PATCH 10/12] KeyAssignments: the first read, whoever makes it, is what the next change is told against A page that read the keys before the owner's first read, followed by the game saving others, left the owner taking the new keys as its first and telling nobody; the page kept the old ones. The first read now sets what later reads compare with, whoever makes it; reads on request after it still leave that to the owner. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../catalog/KeyAssignments.java | 12 ++++++++---- .../catalog/KeyAssignmentsTest.java | 15 +++++++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java index 396f13875..3ac1920a1 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java @@ -65,15 +65,19 @@ public KeyAssignments(Path options) { /** * The keys the file assigns: as last read while its folder is watched, and read now where it has not been read yet - * or is not watched, since then a change would go unseen. A read made here is not what the next change is told - * against: only this owner's own reads are, so every follower hears of a change it has not seen. Blocking where it - * reads. + * or is not watched, since then a change would go unseen. The first read, whoever makes it, is what the next change + * is told against, so a follower that read before this owner did still hears of a later change; after it, only + * this owner's own reads are, so every follower hears of a change it has not seen. Blocking where it reads. */ public Map assignments() throws IOException { synchronized (this) { if (this.read != null && this.watching) return this.read; } - return KeyBindings.readOptions(this.options); + Map now = KeyBindings.readOptions(this.options); + synchronized (this) { + if (this.read == null) this.read = now; + } + return now; } private static WatchService watchService() { diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java index 566e69dc6..000bee49b 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignmentsTest.java @@ -62,6 +62,21 @@ void aGameFolderThatAppearsLaterIsWatchedOnceItDoes() throws Exception { } } + @Test + void aPageThatReadBeforeTheOwnerStillHearsOfTheNextChange() throws Exception { + Path options = this.directory.resolve("options.txt"); + Files.writeString(options, "key_key.jump:key.keyboard.space" + System.lineSeparator()); + AtomicInteger told = new AtomicInteger(); + try (KeyAssignments assignments = new KeyAssignments(options)) { + assignments.changed().subscribe(told::incrementAndGet); + // A page opened at once reads the keys, perhaps before the owner's own first read, and the game then saves. + Map shown = assignments.assignments(); + replace(options, "key_key.jump:key.keyboard.g" + System.lineSeparator()); + await(told::get, 1); + assertEquals(Map.of("key.jump", KeyBindings.Assignment.decode("key.keyboard.space")), shown); + } + } + /** Writes {@code text} beside {@code file} and moves it over the file, as the game and Companion save it. */ private static void replace(Path file, String text) throws Exception { Path staged = file.resolveSibling(file.getFileName() + ".tmp"); From a4361195401b053c2a3f1d15f65c42bf15d15ae8 Mon Sep 17 00:00:00 2001 From: Pelotrio <45769595+Pelotrio@users.noreply.github.com> Date: Wed, 30 Sep 2026 13:09:27 +0200 Subject: [PATCH 11/12] KeyAssignments reads the keys on every retry of its watch Where the game's folder can be read but not watched, the retries only tried the watch again, so a key rebound in the game was never found. Each retry now also reads the file and tells a change, at most every 30 seconds. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../totalDebugCompanion/catalog/KeyAssignments.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java index 3ac1920a1..929d2071c 100644 --- a/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java +++ b/companion/src/main/java/com/github/minecraft_ta/totalDebugCompanion/catalog/KeyAssignments.java @@ -101,13 +101,17 @@ private boolean register() { } } - /** Tries to watch the game's folder again after a while, then reads what the file assigns by then. */ + /** + * Tries to watch the game's folder again after a while, and reads what the file assigns by then either way: while it + * cannot be watched, a key rebound in the game is found by these reads. + */ private void registerLater(int attempt) { long delay = RETRY_MILLIS.get(Math.min(attempt, RETRY_MILLIS.size() - 1)); try { this.timer.schedule(() -> { - if (register()) written(); - else registerLater(attempt + 1); + boolean watchingNow = register(); + written(); + if (!watchingNow) registerLater(attempt + 1); }, delay, TimeUnit.MILLISECONDS); } catch (RuntimeException closed) { // Closed with the project. From 15a68aef6b334834b26ec9039a77404d473362b4 Mon Sep 17 00:00:00 2001 From: Pelotrio <45769595+Pelotrio@users.noreply.github.com> Date: Wed, 30 Sep 2026 13:27:53 +0200 Subject: [PATCH 12/12] PageLoaderTest waits for a read a slow machine starts late The tests asserted a read count right after a change, which CI's slower machine sometimes had not reached yet: a read under way was still being shown, so the next one started a moment later. They now wait for the count. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../ui/components/PageLoaderTest.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java index cdd3e3228..3a2122b13 100644 --- a/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java +++ b/companion/src/test/java/com/github/minecraft_ta/totalDebugCompanion/ui/components/PageLoaderTest.java @@ -196,8 +196,7 @@ void aPageReadsWhenFirstShownAndThenOnlyAfterItsSignals() throws Exception { assertEquals(2, this.prepared.get(), "shown again, it reads what it missed once"); fire(signal); - settle(loader); - assertEquals(3, this.prepared.get(), "a shown page reads a change at once"); + awaitPrepared(3, "a shown page reads a change at once"); } @Test @@ -213,8 +212,7 @@ void aChangeThatDoesNotConcernThePageReadsNothing() throws Exception { assertEquals(1, this.prepared.get(), "another file's change, say, leaves the page alone"); concerns.set(true); fire(signal); - settle(loader); - assertEquals(2, this.prepared.get()); + awaitPrepared(2, "a change that concerns the page reads it"); } @Test @@ -230,8 +228,8 @@ void aHeldPageReadsWhatItMissedOnceReleased() throws Exception { SwingUtilities.invokeAndWait(loader::load); assertEquals(1, this.prepared.get(), "a page holding its reads, as while it saves, reads nothing"); SwingUtilities.invokeAndWait(loader::release); + awaitPrepared(2, "released, it reads what it missed once"); settle(loader); - assertEquals(2, this.prepared.get(), "released, it reads what it missed once"); // The change came from the page's own save: by the time it is released, the change does not concern it. SwingUtilities.invokeAndWait(loader::hold); @@ -358,6 +356,13 @@ void manyChangesWhileHiddenAreAskedAboutOncePerSource() throws Exception { assertEquals(1, asked.get(), "a hidden page keeps one question per source, however often it changed"); } + /** Waits until {@code expected} reads were prepared, as a read that a slow machine starts late. */ + private void awaitPrepared(int expected, String message) throws Exception { + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); + while (this.prepared.get() < expected && System.nanoTime() < deadline) Thread.sleep(10); + assertEquals(expected, this.prepared.get(), message); + } + private static void fire(Signal signal) throws Exception { signal.fire(); SwingUtilities.invokeAndWait(() -> { });