gnodet-bot commented on code in PR #26547:
URL: https://github.com/apache/camel/pull/26547#discussion_r4035113618
##########
dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/Theme.java:
##########
@@ -296,38 +297,74 @@ private static
dev.tamboui.markdown.MarkdownStyles.Builder markdownStylesBuilder
}
/**
- * Theme-aware syntax highlighting palette for fenced code blocks in
MarkdownView. Reuses the Monokai (dark) and
- * GitHub-inspired (light) palettes from {@link SyntaxHighlighter}.
+ * Theme-aware syntax highlighting palette for fenced code blocks in
MarkdownView. Uses the same optional
+ * {@code syntax-*} stylesheet tokens as {@link SyntaxHighlighter}, so the
Source tab and markdown code blocks
+ * agree.
*/
public static SyntaxTheme syntaxTheme() {
Review Comment:
⚠️ **`syntaxTheme()` is not `synchronized`** — the composite `SyntaxTheme`
can be built from colors belonging to two different themes.
Every `syntaxX()` it calls is individually `synchronized`, but the lock is
released between each call. A concurrent `toggle()` or `preview()` call hits
`activate()`, which clears `COLOR_CACHE` and re-initialises the engine while
`syntaxTheme()` is mid-build. The result: the first few tokens come from theme
A, the rest from theme B — a silently inconsistent palette that's hard to
reproduce in tests.
In the TUI this matters most in the preview flow: the user picks a theme
from the settings dialog (which calls `preview()`), and the markdown view calls
`syntaxTheme()` to re-render code blocks at the same time.
Fix: add `synchronized` to this method (same as `diagramBorder()` and all
other `Color`-returning methods in this class).
```suggestion
public static synchronized SyntaxTheme syntaxTheme() {
```
##########
dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/SyntaxHighlighter.java:
##########
@@ -76,40 +76,42 @@ enum Language {
static final Color MONOKAI_TEXT = Color.rgb(248, 248, 242);
// Light color palette (readable on light backgrounds)
- private static final Color LIGHT_COMMENT = Color.rgb(106, 115, 125);
- private static final Color LIGHT_STRING = Color.rgb(3, 47, 98);
- private static final Color LIGHT_KEYWORD = Color.rgb(215, 58, 73);
- private static final Color LIGHT_FUNCTION = Color.rgb(0, 92, 197);
- private static final Color LIGHT_TYPE = Color.rgb(0, 92, 197);
- private static final Color LIGHT_CONSTANT = Color.rgb(111, 66, 193);
- private static final Color LIGHT_TEXT = Color.rgb(36, 41, 46);
-
+ static final Color LIGHT_COMMENT = Color.rgb(106, 115, 125);
+ static final Color LIGHT_STRING = Color.rgb(3, 47, 98);
+ static final Color LIGHT_KEYWORD = Color.rgb(215, 58, 73);
+ static final Color LIGHT_FUNCTION = Color.rgb(0, 92, 197);
+ static final Color LIGHT_TYPE = Color.rgb(0, 92, 197);
Review Comment:
🔍 **Visibility widened without annotation** — these seven constants were
`private static final`; they are now package-private `static final`, which
exposes them as part of the implicit package API used in `ThemeTest`.
That's a reasonable choice (the test needs to compare against known values),
but a `@VisibleForTesting` annotation (Guava, or a local equivalent) would make
the intent explicit and prevent other callers from depending on them as stable
colours.
If Guava is not on the classpath, a brief Javadoc note (`/* package-private
for testing */`) on the block would serve the same purpose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
