feat: add getThemeAttribute and applyThemeAttribute methods AND remove!: remove deprecated methods and classes #127
Conversation
WalkthroughThe pull request updates the project to version 5.0.0-SNAPSHOT while removing deprecated GITHUB_SOURCE constant and its handling logic, deletes the RouteTabs class entirely, and refactors theme-related APIs in TabbedDemo from boolean parameters to string-based attributes. Several Component-based demo addition method overloads are also removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java (2)
230-251: Bug:language()is set fromcaption()instead oflanguage()In
createSourceCodeTab, this line looks incorrect:if (!annotation.language().equals(DemoSource.DEFAULT_VALUE)) { builder.language(annotation.caption()); }You’re populating
languagewith the caption value instead ofannotation.language(), so any explicit language setting on@DemoSourceis ignored/misapplied.Fix suggestion:
- if (!annotation.language().equals(DemoSource.DEFAULT_VALUE)) { - builder.language(annotation.caption()); - } + if (!annotation.language().equals(DemoSource.DEFAULT_VALUE)) { + builder.language(annotation.language()); + }
129-163: Breaking change confirmed: test code calls removedaddDemo(Component)overloadThe verification reveals a compilation-blocking issue. Line 32 of
src/test/java/com/flowingcode/vaadin/addons/demo/Demo.javacallsaddDemo(new LegacyDemo()), butLegacyDemoextendsDiv(aComponent), and the currentTabbedDemoAPI has no overload acceptingComponentinstances—onlyaddDemo(Class<? extends Component>)variants.The test code must be updated to pass the class instead:
addDemo(LegacyDemo.class);
🧹 Nitpick comments (4)
pom.xml (1)
8-8: Version bump to 5.x matches removal of deprecated APIsMajor version for this artifact is appropriate given the breaking removals in the PR. Just ensure release notes clearly call out the removed methods/classes so downstream demos can upgrade smoothly.
src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java (3)
317-323: Session-scoped theme getter is straightforward but assumes a current VaadinSession
getThemeAttribute()reads the theme string from a session attribute and defaults to"", which fits the use inshowRouterLayoutContentand the checkbox listener.If there’s any chance this method could be called outside a Vaadin request context (e.g., background thread), consider guarding against
VaadinSession.getCurrent()beingnulland defaulting to""in that case.You may want to verify all call sites for
getThemeAttribute()are on the UI thread (e.g., viarg -n "getThemeAttribute\(").
324-338:applyThemeAttributeimplementation is solid; consider small robustness tweaksThe method correctly:
- Persists the theme string in
VaadinSessionunder a stable key.- Applies the theme via JS to either the
iframe’s documentElement or the top-level document.- Notifies all
ThemeChangeObserverinstances in the component tree.Two minor robustness improvements you might consider:
Handle empty theme by removing the attribute
Instead of settingtheme="", you could remove the attribute whenthemeis empty to better match typical theming patterns:
- if (element.getTag().equalsIgnoreCase("iframe")) {
script = "this.contentWindow.document.documentElement.setAttribute('theme', $0)";- } else {
script = "document.documentElement.setAttribute('theme', $0);";- }
- if (element.getTag().equalsIgnoreCase("iframe")) {
script = "if ($0) { this.contentWindow.document.documentElement.setAttribute('theme', $0); } "+ "else { this.contentWindow.document.documentElement.removeAttribute('theme'); }";- } else {
script = "if ($0) { document.documentElement.setAttribute('theme', $0); } "+ "else { document.documentElement.removeAttribute('theme'); }";- }
2. **Avoid `Optional.get()` on `element.getComponent()`** If someone ever calls this with an `Element` not backed by a `Component`, `element.getComponent().get()` will throw. A defensive check would make the API safer: ```diff - Component c = element.getComponent().get(); - collectThemeChangeObservers(c).forEach(observer -> observer.onThemeChange(theme)); + element.getComponent() + .ifPresent(c -> collectThemeChangeObservers(c) + .forEach(observer -> observer.onThemeChange(theme)));Please confirm that
ThemeChangeObserverimplementations expect to receive an empty string when no theme is active, or adjust the above suggestion accordingly.
253-262: GitHub branch lookup helper still uses"master"default
lookupGithubBranchcontinues to default to"master", which is consistent with the Javadoc inDefaultSourceUrlResolver. If you ever switch repositories to"main", this helper will be the place to adjust.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/demo/DefaultSourceUrlResolver.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/demo/DemoSource.java(1 hunks)src/main/java/com/flowingcode/vaadin/addons/demo/RouteTabs.java(0 hunks)src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java(4 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/flowingcode/vaadin/addons/demo/RouteTabs.java
🔇 Additional comments (4)
src/main/java/com/flowingcode/vaadin/addons/demo/DemoSource.java (1)
5-5: Metadata update and cleanup of deprecated constantCopyright year update is fine, and removing the deprecated
GITHUB_SOURCEconstant (per PR description) keeps the annotation lean and avoids supporting legacy configuration paths.src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java (2)
5-5: Header metadata change looks goodCopyright update is consistent with other files in this PR.
203-204: Reapplying the stored theme on content change is a good additionCalling
applyThemeAttribute(getElement(), getThemeAttribute())after inserting the new content ensures the theme is consistently re-applied when the router layout content changes, which is exactly what you want for a session-scoped dark/light toggle.It’s worth smoke-testing navigation between different demos to ensure theme observers are triggered as expected when the content element is replaced.
src/main/java/com/flowingcode/vaadin/addons/demo/DefaultSourceUrlResolver.java (1)
45-57: Verification confirms GITHUB_SOURCE constant successfully removed with no remaining usagesThe DemoSource annotation definition shows only the DEFAULT_VALUE constant remains. All existing @demosource annotations in the codebase use compatible arguments—either default, explicit paths, clazz, or sourcePosition—and no references to the removed GITHUB_SOURCE constant exist anywhere. The resolver implementation correctly handles DEFAULT_VALUE and /src/test/... URLs as intended for this major release.
| themeCB.setValue(false); | ||
| themeCB.addClassName("smallcheckbox"); | ||
| themeCB.addValueChangeListener(cb -> { | ||
| applyTheme(getElement(), themeCB.getValue()); | ||
| boolean useDarkTheme = themeCB.getValue(); | ||
| String theme = useDarkTheme ? "dark" : ""; | ||
| applyThemeAttribute(getElement(), theme); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Theme toggle handler correctly delegates to the new theme API
Using the checkbox value to compute "dark" vs "" and calling applyThemeAttribute(getElement(), theme) keeps the UI behavior clear and ties directly into the new session-backed theme attribute.
Minor suggestion: you can use the event’s value (cb.getValue()) instead of closing over themeCB, but the current implementation is perfectly fine.
If you want to double-check expected Vaadin theming behavior for the "dark" theme attribute, please confirm against the Vaadin 24/25 theming docs.
🌐 Web query:
Vaadin Flow theming: how is the `"theme"` attribute on `documentElement` interpreted, and is an empty string equivalent to no theme?
💡 Result:
Short answer: The document's theme attribute is treated as a space‑separated token list (a ThemeList) and is used with attribute selectors like [theme~="dark"]. An empty string is not the same as the attribute being absent — Flow distinguishes "attribute not set" (getAttribute() -> null) from "attribute set but no tokens" (getThemeName() -> empty string). Use the ThemeList / HasTheme API to add/remove tokens rather than manipulating raw strings. [1][2][3]
Sources:
- HasTheme API: getThemeName/getThemeNames behavior (empty string vs null). [1]
- Element API: attributes and getThemeList / attribute semantics (empty value rendering, getAttribute() -> null if not set). [2]
- Vaadin blog on dynamic theming and use of [theme~="..."] selectors. [3]
I'll verify the implementation of applyThemeAttribute to confirm whether the current code correctly handles the theme attribute behavior described in the web search results.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find applyThemeAttribute method definition
rg -nP 'applyThemeAttribute' --type=java -B2 -A10Length of output: 4163
🏁 Script executed:
#!/bin/bash
# Search for any theme attribute handling or Element.setAttribute calls in the codebase
rg -nP 'setAttribute.*theme|getTheme|setTheme' --type=java -C3Length of output: 2688
Critical bug: setting theme attribute to empty string instead of removing it
The applyThemeAttribute method (lines 324–334) always calls setAttribute('theme', $0), which means when theme="" is passed (line 97), the DOM receives setAttribute('theme', ""). Per Vaadin theming semantics, an empty string is not the same as the attribute being absent — Flow distinguishes "attribute not set" (getAttribute() → null) from "attribute set but no tokens" (getThemeName() → empty string).
Fix: Update applyThemeAttribute to call removeAttribute('theme') when the theme parameter is empty:
public static void applyThemeAttribute(Element element, String theme) {
VaadinSession.getCurrent().setAttribute(THEME_NAME, theme);
String script;
if (theme.isEmpty()) {
if (element.getTag().equalsIgnoreCase("iframe")) {
script = "this.contentWindow.document.documentElement.removeAttribute('theme')";
} else {
script = "document.documentElement.removeAttribute('theme');";
}
} else {
if (element.getTag().equalsIgnoreCase("iframe")) {
script = "this.contentWindow.document.documentElement.setAttribute('theme', $0)";
} else {
script = "document.documentElement.setAttribute('theme', $0);";
}
}
if (theme.isEmpty()) {
element.executeJs(script);
} else {
element.executeJs(script, theme);
}
}🤖 Prompt for AI Agents
In src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java around lines
93–99 (and update applyThemeAttribute implementation at lines ~324–334), the
current logic calls setAttribute('theme', "") when theme is empty which leaves
an empty attribute instead of removing it; change applyThemeAttribute so it
stores the theme in VaadinSession as before but builds two branches: if theme is
empty, generate JS that calls removeAttribute('theme') (use
this.contentWindow.document.documentElement for iframe tags, otherwise
document.documentElement), and call element.executeJs(script) with no
parameters; if theme is non-empty, generate JS that calls setAttribute('theme',
$0) (same iframe vs document handling) and call element.executeJs(script,
theme).
There was a problem hiding this comment.
@coderabbitai Create an issue about it. The scope of this PR is only removing the deprecated methods, not fixing behavior that was already there.
CommonsDemo/src/main/java/com/flowingcode/vaadin/addons/demo/TabbedDemo.java
Lines 353 to 357 in 9e9f1e8
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the theme attribute handling bug
gh issue create \
--repo FlowingCode/CommonsDemo \
--title "Theme attribute handling: empty string should remove attribute instead of setting it to empty" \
--assignee javier-godoy \
--body "## Description
The \`applyThemeAttribute\` method in \`TabbedDemo.java\` currently calls \`setAttribute('theme', '')\` when the theme parameter is an empty string. According to Vaadin theming semantics, an empty string attribute is not the same as the attribute being absent — Flow distinguishes \"attribute not set\" (getAttribute() → null) from \"attribute set but no tokens\" (getThemeName() → empty string).
## Current Behavior
When the theme is toggled off (dark mode unchecked), the code sets:
\`\`\`javascript
document.documentElement.setAttribute('theme', '')
\`\`\`
This leaves an empty \`theme\` attribute on the document element.
## Expected Behavior
When the theme parameter is empty, the attribute should be removed entirely:
\`\`\`javascript
document.documentElement.removeAttribute('theme')
\`\`\`
## Suggested Fix
Update \`applyThemeAttribute\` to check if the theme is empty and call \`removeAttribute('theme')\` instead of \`setAttribute('theme', '')\`.
## Context
- Flagged during review of PR #127: https://github.com/FlowingCode/CommonsDemo/pull/127#discussion_r2539569353
- Requested by: @javier-godoy
- This behavior existed in the previous implementation and is out of scope for PR #127, which focuses on removing deprecated methods"
Length of output: 53
@javier-godoy I've created issue #128 to track the theme attribute handling bug: #128
I've assigned it to you and included technical details along with references to this PR and comment thread. You're absolutely right to keep PR #127 focused on removing the deprecated methods—the theme attribute behavior can be addressed separately. 👍
🧬 Code Graph Analysis Results
[src/test/java/com/flowingcode/vaadin/addons/demo/AdHocDemo.java, lines 27-36]
@Route(value = "demo/demo-with-adhoc-source", layout = Demo.class)
@PageTitle("Demo with source in a different file")
@DemoSource("/src/test/java/com/flowingcode/vaadin/addons/demo/AdHocSource.java")
public class AdHocDemo extends Div {
public AdHocDemo() {
add(new Span("This span is added to the demo view"));
}
}
[src/main/java/com/flowingcode/vaadin/addons/demo/SplitLayoutDemo.java, lines 32-114]
@SuppressWarnings("serial")
class SplitLayoutDemo extends Composite<SplitLayout> {
private MultiSourceCodeViewer code;
private Component demo;
private SourcePosition sourcePosition;
public SplitLayoutDemo(Component demo, String sourceUrl, SourcePosition sourcePosition) {
this(demo, Arrays.asList(new SourceCodeTab(sourceUrl, sourcePosition)));
}
public SplitLayoutDemo(Component demo, List<SourceCodeTab> tabs) {
getContent().setOrientation(Orientation.HORIZONTAL);
Map<String, String> properties = new HashMap<>();
properties.put("vaadin", VaadinVersion.getVaadinVersion());
properties.put("flow", Version.getFullVersion());
code = new MultiSourceCodeViewer(tabs, properties);
this.demo = demo;
setSourcePosition(code.getSourcePosition());
getContent().setSizeFull();
}
private void setSourcePosition(SourcePosition position) {
if (!position.equals(sourcePosition)) {
getContent().removeAll();
switch (position) {
case PRIMARY:
getContent().addToPrimary(code);
getContent().addToSecondary(demo);
break;
case SECONDARY:
default:
getContent().addToPrimary(demo);
getContent().addToSecondary(code);
}
sourcePosition = position;
}
}
public void toggleSourcePosition() {
setSourcePosition(sourcePosition.toggle());
}
public void setOrientation(Orientation o) {
getContent().setOrientation(o);
getContent()
.getPrimaryComponent()
.getElement()
.setAttribute("style", "width: 100%; height: 100%");
code.setSizeFull();
}
public Orientation getOrientation() {
return getContent().getOrientation();
}
public void setSplitterPosition(int pos) {
getContent().setSplitterPosition(pos);
}
public void setSizeFull() {
getContent().setSizeFull();
}
public void showSourceCode() {
getContent().setSplitterPosition(50);
}
public void hideSourceCode() {
switch (sourcePosition) {
case PRIMARY:
getContent().setSplitterPosition(0);
break;
case SECONDARY:
getContent().setSplitterPosition(100);
break;
}
}
}
[src/test/java/com/flowingcode/vaadin/addons/demo/MultiSourceDemo.java, lines 27-51]
@Route(value = "demo/multisource", layout = Demo.class)
@PageTitle("Demo with multiple sources")
// show-source @DemoSource
// show-source @DemoSource(clazz = AdditionalSources.class)
// show-source @DemoSource("/src/test/resources/META-INF/resources/frontend/multi-source-demo.css")
@DemoSource
@DemoSource(clazz = AdditionalSources.class)
@DemoSource("/src/test/resources/META-INF/resources/frontend/multi-source-demo.css")
@StyleSheet("./multi-source-demo.css")
public class MultiSourceDemo extends Div {
public MultiSourceDemo() {
// begin-block main
Div div = new Div("This is the main source");
div.addClassName("custom-style");
SourceCodeViewer.highlightOnHover(div, "main");
add(div);
// end-block
Button button1 = new Button("Highlight code in AdditionalSources");
SourceCodeViewer.highlightOnClick(button1, "AdditionalSources.java#fragment");
add(button1);
}
}
[src/test/java/com/flowingcode/vaadin/addons/demo/SampleDemoDefault.java, lines 27-42]
@Route(value = "demo/demo-with-source", layout = Demo.class)
@PageTitle("Demo with source")
@DemoSource
public class SampleDemoDefault extends Div {
public SampleDemoDefault() {
add(new Span("Demo component with defaulted @DemoSource annotation"));
// show-source System.out.println("this line will be displayed in the code snippet");
this.getClass(); // hide-source (this line will not be displayed in the code snippet)
// #if vaadin ge 23
// show-source System.out.println("conditional code for Vaadin 23+");
// #elif vaadin ge 14
// show-source System.out.println("conditional code for Vaadin 14-22");
// #endif
}
}
[src/test/java/com/flowingcode/vaadin/addons/demo/Demo.java, lines 26-40]
@Route("demo")
@RouteAlias("")
GithubLink("https://github.com/FlowingCode/CommonsDemo")
public class Demo extends TabbedDemo {
public Demo() {
addDemo(new LegacyDemo());
addDemo(SampleDemo.class);
addDemo(SampleDemoDefault.class);
addDemo(SampleDemoHighlight.class);
addDemo(AdHocDemo.class);
addDemo(MultiSourceDemo.class);
addDemo(SourcePositionDemo.class);
}
}
[src/test/java/com/flowingcode/vaadin/addons/demo/DialogDemoHelperViewer.java, lines 30-38]
@SuppressWarnings("serial")
public class DialogDemoHelperViewer implements DemoHelperViewer {
@Override
public void show(Component content) {
new Dialog(content).open();
}
}
[src/test/java/com/flowingcode/vaadin/addons/demo/SourceCodeViewerElement.java, lines 25-28]
@Element("code-viewer")
public class SourceCodeViewerElement extends TestBenchElement {
}
github-project-automation
bot
moved this from To Do
to Pending release
in Flowing Code Addons
Close #126
Summary by CodeRabbit
Breaking Changes
Chores