Files others write are read when shown and when the user comes back - #122
Conversation
Nothing watches the files others write any more. A FileReading reads when first asked, when its owner refreshes it and when a window of Companion takes the focus from another program; pages that alone read their files read again then too. FileWatch only tells ExternalEdits that a file opened in an image editor was written. The current world refreshes on the game's signals and on return, without watches of saves, the world or its datapacks. KeyAssignments folds into KeyBindingControl. The Scripts tree relists its loaded folders on return, the decompiled sources follow the decompiler's cached signal, and the folder watches with their discovery retry and pauses for moves go. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Took 4 seconds
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92bafe4199
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /** The current world: its folder and what its level.dat holds, or why there is nothing to show. */ | ||
| public record World(Path directory, CurrentWorld.Saved saved, String problem) { |
There was a problem hiding this comment.
When only icon.png changes while the user is away, the refreshed World remains equal because this record contains only the directory, saved level data, and problem. FileReading therefore does not fire changed(), and the already-visible WorldPanel, which reloads its icon only after that signal, continues showing the old icon indefinitely. Preserve an icon timestamp or equivalent fingerprint in the owner's compared value so returning to Companion detects this update.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9042b16: the world's value holds when its icon was written again, so a new icon alone is a change (WorldReadingTest.aNewIconOfTheWorldIsAChange).
| public void close() { | ||
| this.stopFollowingFocus.run(); |
There was a problem hiding this comment.
If refresh() was queued or its reader was already running when close() executes, removing only the focus subscription does not stop that work: it can still replace last and fire changed() after the owning project has retired. This also permits an explicit refresh arriving after closure, such as the completion callback of a pending key-binding change, to read and publish normally. Restore a closed-state check both before reading and before publication so closing an owner prevents late state changes and notifications.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9042b16: a closed reading's queued or later refresh reads nothing; a value asked for explicitly is still read.
| this.unsubscribe.add(WindowFocus.returned().subscribe(() -> SwingUtilities.invokeLater(() -> { | ||
| if (!this.disposed && component.isShowing()) load(); | ||
| }))); |
There was a problem hiding this comment.
When a readsWhenShown page becomes showing at the same time Companion gains focus, such as when restoring a minimized window or on its initial display, the hierarchy listener queues one load and this focus listener queues another. The first starts, the second sets again, and the loader performs the filesystem read twice while discarding the first result; this is especially costly for logs and pack listings. Route both triggers through the existing queued-show coalescing so one return-and-show transition performs one read.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9042b16: the focus trigger goes through the same queued read as showing (readShown), so showing and coming back in one step read once; the test fails without it (4 reads instead of 3).
A world that cannot be read fails its read instead of becoming a value, so FileReading keeps the world read before and the World page its staged datapacks; the page names a missing world itself. The world's value holds when its icon was written. A closed reading refreshes no more. Showing a page and coming back to Companion queue one read. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Round 1 addressed in 9042b16: the three Codex threads above, plus a local Codex finding. A world read that failed (as while the game writes level.dat) became a value with a problem text, which replaced the last good world and made the World page discard staged datapacks. The reader now fails instead, so FileReading keeps the world read before, and the page names a missing world itself. Full :companion:test green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9042b16636
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var scripts = scriptsRoot(project.get()); | ||
| if (scripts != null) tree.refreshRoot(scripts, true); |
There was a problem hiding this comment.
When the Scripts directory did not exist during reloadProfile() and an external tool creates it while the user is away, scriptsRoot() returns null here and the focus refresh does nothing. Because the filesystem watcher was removed, no later event inserts the root, so the new scripts remain absent from the Project tree until another action explicitly calls ensureScriptsRoot() or the profile is reloaded. Reconcile the root's presence on return rather than refreshing only an existing root.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d1f7bd4: on return the tree refreshes the Scripts root, or where there is none, runs the existing ensureScriptsRoot(), which adds it off the Swing thread when the folder exists now. Test: FileTreeRefreshTest.comingBackToCompanionAddsAScriptsFolderMadeMeanwhile, which fails without it. Not a regression of this PR (the old watch only covered an existing Scripts folder), but cheap with what is there.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1f7bd4961
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.last = new Read<>(now); | ||
| this.unreadable = false; | ||
| this.failed = false; | ||
| if (tell) this.changed.fire(); |
There was a problem hiding this comment.
When close() runs after a refresh has passed the pre-read closed check but while its reader is blocked, this unconditional assignment still publishes the result and the following line can fire changed() into a retired project. The fresh evidence after the prior fix is that it added only the check before reader.read(); unlike the superseded implementation, there is still no post-read closure check. Recheck closed before publishing a refresh result so an in-flight world or options read cannot notify after shutdown.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4c02521: the closed check moved from before the read to just before publishing, so a read under way when the reading closes publishes and tells nothing (FileReadingTest.aReadUnderWayWhenTheReadingClosesPublishesNothing, which fails without it).
| Path followed = Objects.requireNonNull(folder, "folder").toAbsolutePath().normalize(); | ||
| long generation = this.generation.incrementAndGet(); | ||
| this.strand.execute(() -> follow(followed, generation)); | ||
| this.stopFollowingFocus = WindowFocus.returned().subscribe(this::refresh); |
There was a problem hiding this comment.
When a project has created a reading but no consumer has called value() yet, such as when Key bindings has never been opened, the first focus-return signal still invokes refresh(), reads options.txt, and publishes a first-value changed() event. This defeats the documented lazy-read behavior and, if the page is being shown during that focus transition, the first-value signal can make its initial PageLoader read run again. Gate the focus-triggered refresh on an already-published value while preserving explicit owner refreshes that intentionally publish a first value.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4c02521: coming back reads again only a value read before (or whose read failed); a reading nobody asked for stays unread. The owner's explicit refresh still reads and tells a first value, which the current world uses for the World tab's title. Test fails without it. SYSTEMS.md section 2 states the rule.
Returning to Companion reads again only a value read before, so a reading nobody asked for stays unread; the owner's refresh still reads and tells a first value. The closed check moves to just before publishing, so a read under way when the reading closes publishes nothing either. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c0252130d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| T now; | ||
| try { | ||
| now = this.reader.read(this.file); | ||
| now = this.reader.read(); |
There was a problem hiding this comment.
When a refresh is queued before or after project closure—for example, the KeyBindingControl.set() completion callback arriving after its owner closes—readAgain() still reaches reader.read() because closed is checked only after the filesystem access. The fresh evidence after the prior close fix is that the final code moved the check to after this call, so it prevents publication but still performs work against a retired project, contrary to close()'s “refreshes no more” contract; skip the reader for refresh work once closed while preserving an explicitly requested value() if intended.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 58edf96. FileReading now states its whole contract once (class doc and SYSTEMS.md section 2), including: once closed, no read starts and none publishes. The closed check sits both where a refresh would start and before publishing; a value asked for explicitly is still read. Tested: refreshesAskedForWhileOneWaitsReadOnceAndAClosedReadingStartsNone.
| if (windowEvent.getID() == WindowEvent.WINDOW_GAINED_FOCUS && windowEvent.getOppositeWindow() == null) { | ||
| WindowFocus.returned().fire(); |
There was a problem hiding this comment.
On the first display of MainWindow, WINDOW_GAINED_FOCUS also has a null opposite window even though the user did not return from another program, so this condition fires WindowFocus.returned() during startup. With an existing project, WorldReading has already queued its constructor refresh; the focus callback subsequently observes that value and performs another full world read, and a visible readsWhenShown page can likewise be requested again if its initial show load has started. Track an actual loss of application focus before emitting the return signal so startup does not read the same files twice.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 58edf96: WindowFocus now owns what counts as a return, a window of Companion taking the focus after Companion lost it to another program. MainWindow only passes focus events on, and the first window opening is no return. Tested in WindowFocusTest, including focus moving between Companion's own windows.
| boolean gone = !key.reset(); | ||
| if (gone) this.keys.values().remove(key); | ||
| for (Watched followed : List.copyOf(this.watched)) { | ||
| if (followed.key != key) continue; | ||
| if (gone || !followed.at.equals(followed.folder)) { | ||
| // Removed, or an ancestor whose entry toward the folder may have appeared: placed again. | ||
| Path toward = gone ? null : followed.folder.getName(followed.at.getNameCount()); | ||
| if (gone || lost || names.contains(toward)) { | ||
| if (gone) followed.key = null; | ||
| place(followed, 0); | ||
| tell.add(followed.changed); | ||
| } | ||
| } else if (lost || concerns(followed, followed.entriesOnly ? entries : names)) { | ||
| tell.add(followed.changed); | ||
| List<Watched> followers = gone ? this.watched.remove(key) : this.watched.get(key); |
There was a problem hiding this comment.
When an editor or pack-management tool removes and recreates the watched file's directory, reset() returns false and this removes its registration permanently. The removal callback reaches ExternalEdits, but that owner keeps the file in its followed map; consequently, opening the restored file in an editor again passes the existing-entry check in follow() and never calls FileWatch.watch(), so all later saves are silently ignored until the project is reopened. Either re-establish the watch or invalidate the corresponding external-edit follower when the directory watch is lost.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 58edf96: every opening of a file in an editor watches it anew, so a file whose folder was removed and made again is followed again once reopened. I checked on Windows that deleting and recreating a watched folder works and invalidates the watch. Test: aTextureWhoseFolderWasMadeAgainIsFollowedOnceOpenedAgain, which fails without it.
| private static World read(GameState game) throws IOException { | ||
| Path directory = CurrentWorld.directory(game).orElse(null); | ||
| if (directory == null) return new World(null, null, null); | ||
| return new World(directory, CurrentWorld.read(game, directory), iconWritten(directory)); |
There was a problem hiding this comment.
When the game changes from world A to world B and B's level.dat read fails, such as during an atomic save or a transient filesystem error, the generic FileReading failure behavior retains the complete World for A and fires no change. An already-open WorldPanel therefore remains editable against A; its datapack applier uses the stale saved directory, and because the connected game now plays B, the pipeline can write A's unlocked level.dat instead. On a world-identity change, publish B's directory with an error state or otherwise invalidate A rather than applying the same keep-last behavior used for a failed reread of one world.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 58edf96. Keeping the last value on a failed read is now only for the same world read again: another world that fails to read is published as current without what it holds, and the page says it could not be read, so nothing shows or changes the world before as current. Test: anotherWorldThatCannotBeReadIsCurrentWithoutWhatItHolds, which fails without it.
| this.stopFollowing = List.of(location.connectionChanged().subscribe(this.reading::refresh), | ||
| location.playingChanged().subscribe(this.reading::refresh), | ||
| // Also in the level.dat of a world the game does not hold. | ||
| packs.changed(ChangeRecord.PackSide.DATA).subscribe(this.reading::refresh)); |
There was a problem hiding this comment.
Whenever playingChanged() fires, the GamePacks listener registered earlier in project construction synchronously fires its DATA signal, which queues one refresh through packs.changed(...); the direct playingChanged() subscription here then queues another. FileReading serializes but does not coalesce these requests, so every transition between worlds parses level.dat twice and can drive redundant downstream page loads. Follow one of these equivalent transition notifications or coalesce refresh requests in the owner.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 58edf96, in FileReading rather than by choosing a signal: refresh requests made while one waits merge into it, as PageLoader's do. A world change told by both playing and the datapacks reads level.dat once. The test fails without it (5 reads instead of 3).
FileReading states its whole contract once: refresh requests made while one waits are that one, and a closed reading starts and publishes no read. WindowFocus owns what counts as a return, so opening Companion reads nothing twice. Another world that fails to read is current without what it holds, so the world before is not shown or changed as current. Every opening of a file in an editor watches it anew. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58edf967d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| Read<T> now = this.last; | ||
| read.complete(now != null ? now.value() : readHere(0)); | ||
| read.complete(now != null ? now.value() : read(false)); |
There was a problem hiding this comment.
If close() wins after value() observes last == null but before this queued strand task executes—for example, when a first Key bindings page read races project retirement—this branch still calls read(false), whose reader runs before checking closed. The future can therefore read and return data from a retired project despite the new class contract promising that no read starts once closed; check closure inside this queued value path before invoking the reader.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf3e15c: the closed rule now lives in read() alone, checked as a read starts and before it publishes, so neither value() nor a refresh reads once closed (the separate check in the refresh path went). Test: aValueAskedForOfAClosedReadingIsNotRead, which fails without it.
| Followed followed = this.followed.get(file); | ||
| if (followed == null) { | ||
| followed = new Followed(path, pack, Files.readAllBytes(file)); | ||
| this.followed.put(file, followed); |
There was a problem hiding this comment.
When FileWatch.watch throws—such as when the directory disappears between the initial isRegularFile check and registration, or the OS watch limit is reached—the new Followed is already stored here. Because open() calls follow() before entering its cleanup try, the failure leaves follows() true even though no program was opened and no watch exists; a later launch failure then treats this as a preexisting follower and retains the stale entry. Publish the follower only after registration succeeds, or remove it when registration fails.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf3e15c: follow() registers the watch first and publishes the follower only then, so a watch that fails leaves follows() false and nothing behind. Not covered by a test: making the registration fail between the file check and the watch would need a hook in production code, which we don't add for tests.
FileReading keeps its closed rule in read() alone, so neither value() nor a refresh starts a read once closed. ExternalEdits follows a file only after its watch was made, so a watch that fails leaves nothing behind. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf3e15c1db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Followed after the triggers are, so none is missed in between; on the strand, as every later look. | ||
| CompletableFuture.runAsync(this::follow, this.strand).join(); | ||
| Objects.requireNonNull(location, "location"); | ||
| this.reading = new FileReading<>(() -> read(location.read())); |
There was a problem hiding this comment.
When playingChanged() queues another refresh while CurrentWorld.read is still parsing the previous world, that first invocation has already captured the old GameState, and FileReading publishes its result before processing the queued refresh. WorldPanel can consequently display the old world and its datapack applier uses that stale saved directory, allowing an edit intended for the newly entered world to be written to the previous one while the newer read is pending. Preserve a request generation in WorldReading or otherwise prevent an overtaken world-identity read from publishing.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a1fa4e6, in FileReading rather than with a counter in WorldReading: a read under way when a refresh is asked for publishes nothing, and the read it waits for publishes, the newest-wins rule the page loader already follows. The world the game left is never published after the request for the new one. Test: aReadOvertakenByARequestMadeWhileItRanPublishesNothing, which fails without it (published Second, Third).
| super(); | ||
| this.project = project; | ||
| Objects.requireNonNull(navigator, "navigator"); | ||
| this.stopFollowingFocus = WindowFocus.returned().subscribe(() -> SwingUtilities.invokeLater(this::refreshScripts)); |
There was a problem hiding this comment.
When the Tools menu creates a script through InspectionSession.createTool, Companion retains focus because its name dialog is another Companion window, so this return-only refresh never runs. With the directory watchers removed, neither creating the tools folder nor adding the new script updates an already-loaded Scripts tree; the navigator opens the editor, but the file remains absent from the tree until the user switches to another application and back or causes an unrelated explicit refresh. Route this Companion-owned creation through the tree's existing refresh system.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a1fa4e6 where navigation shows the file: revealing a script the loaded tree lacks lists Scripts again (ensuring the root) and reveals it then, so the Tools menu's new tool, and any script Companion makes beside the tree's own actions, appears once navigated to. Test: aToolMadeFromTheToolsMenuIsShownInTheLoadedTree, which fails without it.
A read under way when a refresh is asked for publishes nothing, as in the page loader, so the world the game left is never shown as current after it left. Revealing a script the loaded tree lacks, as a tool the Tools menu made, lists Scripts again and reveals it then. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 participant
Files others write are no longer watched. Companion reads them when they are shown and when the user comes back to Companion from another program. The one watch left is for a texture opened in an image editor from Companion, whose saves must reach the game while the user stays in the editor.
Decision and reasons: docs/SYSTEMS.md, section 2, "Why nothing else is watched". In short, most review findings of #114 to #117 were about watching folders that others own (ancestors of folders that do not exist yet, links, retries, pauses so Windows can rename or delete a watched world). Reading on show and on return answers the same question without any of that.
How Companion learns of a change now
PLAYING, answers), unchangedWindowFocus.returned(), which the main window fires when a Companion window takes the focus from another programFileWatch, now only "tell me when this file is written"What changed
FileReadingkeeps only its reader, value, comparison and signal. It reads when first asked, onrefresh()and when the user comes back. No watch, settle, retries, counter ormoveTo.WorldReadingis aFileReadingwhose reader asks the game location for the current world. It refreshes on connection,playingand datapack signals, and once when it is made so the World tab names the world. The watches ofsaves, the world's folder and itsdatapacksare gone, and so is the toggle that let the game's Delete World work.KeyAssignmentsis folded intoKeyBindingControl, the category that writes those keys.FileWatchis cut from 322 to 103 lines: no ancestors, real paths, retries,watchEntriesorpausing. A folder that cannot be watched makes "open in editor" fail with the reason.PageLoader.readsWhenShownalso reads when the user comes back while the page is shown (logs, configuration, resource packs, Changes).cachedsignal from the decompiler. The "changed during discovery" retry, which existed only for watcher events, is gone.ScriptFileActionsno longer pauses watches around moves.Production code: +214, -653.
Known gap until the key binding message
A key rebound in the game while Companion is visible beside it but not focused shows once the user clicks into Companion, not at once. The follow-up adds a game message for key assignments, sent when a screen closes and the bindings changed.
Tests
:companion:testgreen locally. New:FileReadingTest(rewritten, with a read paused mid-flight),FileWatchTest(rewritten),WorldReadingTest(on return), a Key bindings page read after the user comes back,readsWhenShownon return, and the Scripts tree relisted on return with expansion and selection kept.🤖 Generated with Claude Code