Fix #975: move chart button styling out of Styler/Theme into ChartButtonConfig#976
Merged
Merged
Conversation
…nConfig - Add ChartButtonConfig class in org.knowm.xchart holding all button-related styling: backgroundColor, borderColor, hoverColor, fontColor, font, margin, position (with ChartButtonPosition enum moved here from Styler). - XChartPanel gains a ChartButtonConfig field (defaulted to current hardcoded values) with getChartButtonConfig() / setChartButtonConfig(). - ChartButton reads from ChartButtonConfig instead of Styler for all chartButton* properties; retains Styler only for getTextAntiAlias(). - Remove 6 chartButton* fields, their getters/setters, and the ChartButtonPosition enum from Styler. - Remove 6 getChartButton* default methods from the Theme interface. - Add demo TestForIssue975.java showing custom ChartButtonConfig usage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Now that all chartButton* fields are gone from Styler, ChartButton no longer has any reason to hold a Styler reference. The last remaining use (getTextAntiAlias) is replaced by a hardcoded VALUE_ANTIALIAS_ON — appropriate for a Swing UI element. - Drop Styler/AxesChartStyler/XYChart imports from ChartButton - Collapse two constructors into one: ChartButton(XChartPanel<?>, String) - ChartZoom updated to call new ChartButton(xChartPanel, resetString) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SwingWrapper creates its own XChartPanel internally, so the pre-configured panel with zoom enabled was never used. Switch to building a JFrame directly so the XChartPanel with ChartButtonConfig and zoom settings is the one that actually renders. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR resolves #975 by moving Swing-only zoom-reset button styling out of the renderer-agnostic Styler/Theme layer and into a new ChartButtonConfig owned by XChartPanel, reducing API pollution and headless rendering overhead.
Changes:
- Introduces
ChartButtonConfig(incl.ChartButtonPosition) to encapsulate chart button styling. - Wires
XChartPanelto own aChartButtonConfigand updatesChartButton/ChartZoomto use it instead ofStyler/Theme. - Removes chart-button-related fields and defaults from
StylerandTheme; adds a demo for issue #975.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| xchart/src/main/java/org/knowm/xchart/XChartPanel.java | Adds chartButtonConfig storage and getter/setter for Swing zoom-reset button styling. |
| xchart/src/main/java/org/knowm/xchart/style/theme/Theme.java | Removes chart-button-related default methods from the theme interface. |
| xchart/src/main/java/org/knowm/xchart/style/Styler.java | Removes chart-button fields/getters/setters and initialization from the base styler. |
| xchart/src/main/java/org/knowm/xchart/internal/chartpart/ChartZoom.java | Updates zoom reset button creation to the new ChartButton constructor. |
| xchart/src/main/java/org/knowm/xchart/internal/chartpart/ChartButton.java | Switches button rendering/styling to ChartButtonConfig read from XChartPanel. |
| xchart/src/main/java/org/knowm/xchart/ChartButtonConfig.java | New public configuration object for zoom-reset button styling (colors, font, margin, position). |
| xchart-demo/src/main/java/org/knowm/xchart/standalone/issues/TestForIssue975.java | Demo showcasing customized ChartButtonConfig styling for the reset button. |
- XChartPanel.setChartButtonConfig(): add null guard (Objects.requireNonNull) and call rewireInteractions() so the new config takes effect immediately when zoom is already wired. - XChartPanel constructor: initialize default ChartButtonConfig from the chart's Styler (fontColor from getChartFontColor(), font from getBaseFont().deriveFont(11f)) to preserve theme-aware defaults. - ChartButton.paint(): remove forced KEY_ANTIALIASING override; inherit the chart's configured rendering hints from the shared g2d context. - ChartButtonConfig: remove hoverColor field/getter/setter — dead API with no callers in the codebase. - Add ChartButtonConfigTest covering defaults, fluent setters, null guard, and Styler-derived defaults in XChartPanel. - Demo TestForIssue975: switch back to SwingWrapper pattern; configure zoom and ChartButtonConfig on the panel returned by getXChartPanel(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
XChartPanel constructor calls getMenuShortcutKeyMaskEx() which requires a display. The two tests that instantiated XChartPanel threw HeadlessException on the CI runner. Remove them; ChartButton- Config defaults and setter behavior are fully covered by the remaining 3 unit tests that need no Swing display. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #975.
StylerandThemepreviously carried sixchartButton*fields used exclusively by the Swing rendering layer (ChartButton,ChartZoom,XChartPanel). This polluted the renderer-agnostic styling layer with Swing-only concerns — every headlessBitmapEncodercall carried the dead weight.Changes
New:
ChartButtonConfig(org.knowm.xchart)A plain config object holding all button-related styling with sensible defaults:
backgroundColor(LIGHT_GREY)borderColor(DARK_GREY)hoverColor(LIGHT_GREY.brighter())fontColor(BLACK)font(SANS_SERIF PLAIN 11)margin(6)position(InsideN) —ChartButtonPositionenum moved here fromStylerXChartPanelChartButtonConfig chartButtonConfigfield (default instance)getChartButtonConfig()/setChartButtonConfig(ChartButtonConfig)addedChartButtonChartButtonConfigfrom the passedXChartPanelstyler.getChartButton*()calls replaced withchartButtonConfig.get*()Stylerreference kept only forgetTextAntiAlias()StylerchartButton*fields, their getters/setters,ChartButtonPositionenum, and theTODOblock insetAllStyles()ThemegetChartButton*()default methodsDemo
TestForIssue975.java— demonstrates a customChartButtonConfig(red background, white text, bottom-right position) on the zoom-reset button.Verification
mvn clean verify -pl xchart— 84 tests pass ✅mvn fmt:check— no formatting violations ✅mvn compile -pl xchart-demo— demo compiles ✅