feat(ui): add a Clear Log button to the log view - #725
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe log view now includes a ChangesLog clearing
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR adds a localized Clear Log control for accumulated launcher output and reports successful build and manual validation; no actionable merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/terasology/launcher/ui/LogViewController.java`:
- Around line 63-72: Update clearLogAction() and the scheduled flush
coordination to use a generation/token: increment it when clearing, capture it
with each buffer snapshot, and discard snapshots whose token is stale before
appending. Ensure all logArea.clear()/appendText() UI updates are dispatched
through Platform.runLater() on the JavaFX Application Thread.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 90a77567-85f9-48f2-a060-3ff767dba618
📒 Files selected for processing (2)
src/main/java/org/terasology/launcher/ui/LogViewController.javasrc/main/resources/org/terasology/launcher/views/log-view.fxml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/terasology/launcher/ui/LogViewController.java`:
- Around line 52-66: Update the buffer-draining task around generation and
buffer so it reads and increments generation while holding the buffer monitor,
making the generation snapshot atomic with buffer.toString() and
buffer.setLength(0). Apply the same change to the corresponding logic noted
around the second occurrence, while preserving the existing FX-thread append
guard.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e645ed7-db2b-4ea1-ad3c-6be8f1c77a55
📒 Files selected for processing (1)
src/main/java/org/terasology/launcher/ui/LogViewController.java
Fixes #661. The launcher's log view accumulates both its own output and everything the running game prints for as long as the launcher process stays alive. With the default 'close launcher after game start' setting that's rarely an issue - but a failed launch keeps the launcher open instead of closing it, and retrying (adjust settings, launch again) piles each attempt's output onto the last one in the same view. Same story for anyone who's turned that setting off specifically to keep watching logs across multiple play sessions. Clears the buffered-but-not-yet-flushed text before the visible TextArea, so a flush racing in right after the click can't silently re-append stale content and undo the clear. Verified: full ./gradlew build passes; manually launched the app (with 'close after start' unchecked to accumulate real content across multiple runs) and confirmed the button actually clears an already-populated log. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3dYofgD6GW7V21k6Mzfud
CodeRabbit review: my earlier fix (clear the buffer before logArea) assumed the flush task's snapshot-then-append was atomic - it isn't. The flush task releases the buffer lock right after taking its snapshot, then appends separately; a clear landing in that window still gets silently undone when the stale append finally runs. Also fixes a real pre-existing bug the review surfaced along the way: the flush task's appendText() call ran on its own background thread, never wrapped in Platform.runLater() - a Task's call() does not run on the JavaFX Application Thread, and JavaFX controls may only be mutated from that thread. Wrapped it. A generation counter, bumped by clearLogAction() and captured alongside each flush's buffer snapshot, lets the (now Platform.runLater'd) append discard itself if a clear happened since the snapshot was taken. The increment and the discard check both happen on the FX Application Thread, so there's no race between those two specifically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V3dYofgD6GW7V21k6Mzfud
… race The button's label was set in FXML, bypassing the mechanism every other control here uses: a placeholder in FXML plus `textProperty().bind(I18N.labelBinding(key))` in the controller. It would have read "Clear Log" in all seventeen supported languages, and — because `labelBinding` tracks `localeProperty` — it would have been the one control that didn't re-translate when the language is changed in Settings, sitting directly beneath tabs that do. Adds `tab_log_clear` to the key manifest and its English text, and binds it; other locales are left to translation as usual. The existing `fx:id` becomes load-bearing rather than decorative. The generation counter was read outside the buffer lock, which left the window it exists to close still open, just narrower and inverted: a clear landing between the read and the drain tags text drained *after* the clear with the pre-clear generation, so the append discards log lines that belong on screen. Reading it and bumping it under the lock makes "which generation does this text belong to" an invariant rather than a matter of timing. No change to the `Platform.runLater` wrapping, which is worth keeping on its own merits — `appendText` was being called off the FX Application Thread every two seconds before this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
5c35a8b to
8ec90fe
Compare
|
Rebased onto master (it had drifted behind after #727) and added a small follow-up commit with two things. Localization. The button sets its label in FXML, which bypasses the mechanism the rest of the launcher uses — a placeholder in FXML plus The generation counter. Right instinct, but the read sat outside the buffer lock, which left the window it exists to close still open — just narrower, and inverted. If a clear lands between One thing worth adding to the PR description rather than the code: wrapping Verified |
3 participants
Fixes #661.
Summary
The launcher's log view accumulates both its own output and everything the running game prints for as long as the launcher process stays alive. With the default "close launcher after game start" setting that's rarely an issue - but a failed launch keeps the launcher open instead of closing it, and retrying (adjust settings, launch again) piles each attempt's output onto the last one in the same view. Same story for anyone who's turned that setting off specifically to keep watching logs across multiple play sessions.
Adds a small "Clear Log" button above the log
TextArea. Clears the buffered-but-not-yet-flushed text before the visible area, so a periodic flush racing in right after the click can't silently re-append stale content and undo the clear.Test plan
./gradlew buildpasses (compile, checkstyle, PMD, tests).