CAMEL-23867: camel tui - Add settings dialog#24545
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 tested, 8 compile-only — current: 7 all testedMaveniverse Scalpel detected 17 affected modules (current approach: 7).
|
davsclaus
left a comment
There was a problem hiding this comment.
Thank you for this well-structured PR, @ammachado! The Settings dialog, config file rename with migration, and the per-key local/global routing in TuiUserConfig are nicely designed. The test coverage is thorough — 5 new test files covering migration idempotency, settings round-trip, popup rendering, popup interaction, and local/global key routing.
Two items need attention before this can be merged:
-
Unmerged dependency — The PR description states it depends on #24541 (CAMEL-23971: fix ordered properties containsKey support), which is still open.
TuiUserConfig.livesInLocal()explicitly works around the brokenOrderedProperties.containsKey()(documented in the comment), but this PR should wait for #24541 to land first. -
Default folder vs. last folder precedence — see inline comment on
FolderInputPopup.java.
One minor observation: CommandLineHelperMigrationTest.renamesLegacyLocalFileToNewName creates files in the actual CWD (not @TempDir) since local config paths are CWD-relative. The try/finally cleanup mitigates this, but it's fragile if the test is interrupted.
This review does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analyzers (SonarCloud).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
|
Thanks for the review, @davsclaus. On the #24541 dependency (item 1): I've removed the On the folder precedence (inline comment): fixed. On Claude Code on behalf of Adriano Machado (ammachado) |
|
Correction / course change to my previous comment: to keep this PR self-contained and mergeable independently of #24541, I've kept the existing The switch to Still included here: the Claude Code on behalf of Adriano Machado (ammachado) |
|
There is merge conflict now as TUI has been updated with themes |
oscerd
left a comment
There was a problem hiding this comment.
Nice feature, and the test coverage is thorough — the *RenderTest / real-KeyEvent patterns mirror the module well and the config migration is well covered. The two substantive items from @davsclaus's review look addressed: the OrderedProperties.containsKey dependency was dropped in favour of getProperty(key) != null (correct for java.util.Properties, with a regression test for the empty-value case), and the folder precedence lastFolder > defaultFolder > user.dir is fixed and tested.
The remaining blocker is the merge conflict @davsclaus mentioned (the TUI picked up themes in CAMEL-23839) — a rebase onto current main should clear it, then it's ready for re-review.
Reviewed with Claude Code on behalf of Andrea Cosentino. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Add a Settings dialog to the Camel TUI (F2 actions menu) for choosing the theme, the starting tab, and the default run-from-folder, with room for future settings. Settings are held in a single TuiSettings object (load -> mutate -> save) persisted under camel.tui.* keys. Bundle two related changes: - Rebrand the Camel CLI user configuration file from camel-jbang-user.properties to camel-cli.properties, keeping the existing dot convention (global ~/.camel-cli.properties hidden, local ./camel-cli.properties visible). A one-time best-effort migration renames a pre-existing global and local legacy file at CLI startup. - Make the TUI settings (theme, last folder, TuiSettings) local-aware: read/write ./camel-cli.properties when present, else ~/.camel-cli.properties. The starting tab is applied on launch; the default folder is used only when there is no remembered last folder. Docs and the 4.22 upgrade guide are updated accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fix the Run-from-Folder pre-fill precedence in FolderInputPopup to match the documented contract: the most recently used folder wins, the configured default folder is used only when there is no remembered folder, and user.dir is the final fallback. Adds FolderInputPopupTest covering all three cases. Also adds a TuiUserConfig regression test asserting that an explicitly-set empty-valued local key is still recognized as a local override. Note: TuiUserConfig.livesInLocal keeps the getProperty-based local-key check for now, so this PR stays independent of CAMEL-23971 (apache#24541). Switching to OrderedProperties.containsKey is left to a follow-up PR once apache#24541 lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
davsclaus
left a comment
There was a problem hiding this comment.
Nice work on this PR, @ammachado! The config file rename with migration, per-key local/global routing in TuiUserConfig, and the Settings dialog are well-structured. Test coverage is thorough — 5 new test files covering migration idempotency, settings round-trip, popup rendering, popup interaction, and local/global key routing.
The two items from the prior review by @davsclaus appear addressed:
- Folder precedence is now correctly
lastFolder > defaultFolder > user.dir(matching the docs), withlastFolderWinsOverDefaultFoldertest confirming it. - #24541 dependency has been merged, and
TuiUserConfig.livesInLocal()usesgetProperty(key) != nullwith an explanatory comment.
One design concern worth discussing — see inline comment on SettingsPopup.java.
This review does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analyzers (SonarCloud).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
Address review feedback on PR apache#24545 (discussion_r3551915429): the Theme row rendered all 15 ThemeMode ids inline via renderCycler(), overflowing the ~44-char field and truncating most theme names. Switch the Theme row to the same renderValue() pattern already used by Starting Tab, and go further by wiring it into the live-preview machinery ThemePopup already uses (Theme.preview/revertPreview/confirmPreview): cycling the Theme row applies the theme instantly without closing the dialog, Enter persists the previewed theme, Esc (or any global action that dismisses all popups) reverts it. Also fixes a pre-existing failing test, themeCyclerWrapsAroundValues, which assumed only 2 themes existed; it now cycles through the full ThemeMode count. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewed 3 PRs: apache#24566 (APPROVE, posted), apache#24565 (APPROVE, recorded), apache#24545 (APPROVE after re-review, recorded). Updated skill to never permanently skip CHANGES_REQUESTED PRs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…mes list The doc had drifted from Theme.java's REQUIRED_TOKENS (27 -> 28, missing the mnemonic token) and ThemeMode only listed dark/light instead of all 15 registered themes. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
gnodet
left a comment
There was a problem hiding this comment.
Re-reviewed after author addressed previous feedback. All three items from the prior review have been addressed:
- Folder precedence is now correctly
lastFolder > defaultFolder > user.dirwith theFolderInputPopupTest.lastFolderWinsOverDefaultFoldertest covering it. - #24541 dependency (
OrderedProperties.containsKey) has been resolved — the code uses thegetProperty(key) != nullworkaround with an explanatory comment. - Theme rendering overflow — now uses
renderValue()instead ofrenderCycler(), with live-preview during cycling matching ThemePopup behavior.
Test coverage is thorough: 7 new test files covering migration idempotency, settings round-trip, popup rendering, popup interaction, local/global key routing, folder precedence, and user config per-key routing. The TuiUserConfig per-key local/global routing design is clean — a key in the local file is treated as a project override and stays local, while personal preferences default to the global file.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
Description
Add a Settings dialog to the Camel TUI (F2 actions menu) for choosing the theme, the starting tab, and the default run-from-folder, with room for future settings. Settings are held in a single
TuiSettingsobject (load -> mutate -> save) persisted undercamel.tui.*keys.The Theme row live-previews the selected theme as you cycle through it (Space/arrows), the same way the standalone Theme popup does: the theme applies instantly without closing the dialog, Enter persists the previewed theme, and Esc (or dismissing the dialog via any other action) reverts to whatever was active before the dialog was opened.
Bundles two related changes:
camel-jbang-user.propertiestocamel-cli.properties, keeping the existing dot convention (global~/.camel-cli.propertieshidden, local./camel-cli.propertiesvisible). A one-time best-effort migration renames a pre-existing global and local legacy file at CLI startup.TuiSettings) local-aware: read/write./camel-cli.propertieswhen present, else~/.camel-cli.properties.The starting tab is applied on launch; the default folder is used only when there is no remembered last folder. Docs and the 4.22 upgrade guide are updated accordingly.
Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.AI-assisted contributions
Co-authored-bytrailers) and the PR description identifies the AI tool used.Claude Code on behalf of Adriano Machado (ammachado)