Skip to content

Initial implementation of a painter that serializes chart to JSON - #2812

Open
dbarashev wants to merge 8 commits into
masterfrom
dbarashev/json-chart-painter
Open

Initial implementation of a painter that serializes chart to JSON#2812
dbarashev wants to merge 8 commits into
masterfrom
dbarashev/json-chart-painter

Conversation

@dbarashev

@dbarashev dbarashev commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

What is inside:

  • Painter capable of serializing chart to JSON, similar to the existing painter that serializes to mxGraph XML
  • Kotlin/JS library that draws the JSON chart on canvas
  • Demo projects that use the library

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

Claude finished @dbarashev's task in 6m 12s —— View job


Review: JSON chart painter

  • Gather context (repo guidelines, PR diff)
  • Review JVM-side painter changes (biz.ganttproject.mxgraph)
  • Review Kotlin/JS chart module (cloud.ganttproject.chart)
  • Review build configuration & generated artifacts
  • Post review findings

The refactoring is clean: extracting PainterImpl out of MxPainterImpl and threading it through the style-resolving painters is exactly the right seam, and both sides come with real tests (the pixel-readback tests in DrawChartTest are a nice touch). The ColortoHexString() fix in ColouredRectanglePainter.kt:33 and hexBackgroundColor in SummaryTaskPainter.kt:35 are genuine bug fixes for the mxGraph backend too.

Below are the things I'd address before this becomes the basis for the client-side renderer.


1. Z-order is lost, and the renderer inverts it further

JsonPainterImpl buckets primitives by kind, and drawChart replays them as rectangles → lines → rhombuses (ChartRenderer.kt:53-55). Two problems:

  • Within a single canvas, Canvas.paint() (biz.ganttproject.core/.../canvas/Canvas.java:592-617) emits rectangles → rhombuseslines → texts. The renderer paints rhombuses last, so a milestone diamond will now cover a dependency line that used to be drawn on top of it.
  • Across canvases the problem is bigger. A chart is painted by several renderers/layers in sequence (DayGridSceneBuilder, TimelineSceneBuilder produce grid lines; TaskRendererImpl-family produces the bars; DependencySceneBuilder produces connectors). MxPainterImpl preserved global insertion order in the mxGraph child list, but flattening into per-kind arrays does not: all grid lines end up after all task rectangles, so the day grid and timeline separators will paint over the task bars.

This is inherent to the model shape, not to the renderer, so it needs a model change. Cheapest fix: give every primitive a monotonically increasing sequence number in JsonPainterImpl and sort by it in drawChart. Cleaner: emit one flat primitives array with a type discriminator, which matches the display-list semantics of Painter exactly.

Fix this →

2. version = "2026-dev-json" is hardcoded in the published module

biz.ganttproject.mxgraph/build.gradle:61. addPublishing(project) wires up the GCS internal repo and GitHub Packages, so this string is what publish uploads — not just publishToMavenLocal (which CI runs in gradle.yml:58). Looks like a leftover from the local-publishing commit; if it must stay for now, at least gate it behind a property.

3. texts is emitted but exists in neither client model

JsonPainterImpl.paintText (JsonPainterImpl.kt:70-78) fills a texts collection and JsonPainterTest.kt:124-136 asserts on it, but:

  • chart_model.ts:15 states "Text and text groups are not implemented yet" — inaccurate, they are serialized;
  • neither ChartModel in chart_model.ts:25-29 nor in ChartModel.kt:40-44 declares texts, and drawChart ignores it.

Net effect: a client rendering a real server model gets bars with no labels, and a TS consumer typed against chart_model.ts cannot even see the field. Please add texts to both models (renderer support can stay a follow-up) and fix the header comment.

4. strokeWidth is consumed but never produced

ChartRenderer.kt:93 and :135 apply strokeWidth, and ChartModel.kt:55,69 declare it — but nothing on the JVM side ever emits mxConstants.STYLE_STROKEWIDTH, and chart_model.ts doesn't declare it either (drift between the two hand-written models). MxGraphPainter.paint(line) already reads the stroke at MxGraphPainter.kt:100, so stroke?.lineWidth is right there. Same story for the dash pattern: stroke.dashArray is reduced to a 1/0 flag and the renderer hardcodes [3, 3] (ChartRenderer.kt:35), so custom dashes (e.g. the "today" line / baseline styles) won't be reproduced.

5. Jackson: version drift + not reachable from the published artifact

  • jackson-databind:2.22.1 (build.gradle:24) vs 2.19.1 used in biz.ganttproject.core and exported by biz.ganttproject.app.libs. Please align — otherwise you compile against one version and run against another wherever both are on the classpath.
  • Jackson is in implementation but not in the direct configuration, so it lands neither in the fat jar assembled by jar { from configurations.direct } (build.gradle:45-52) nor in the Class-Path manifest entry. And the publication is artifact jar only — no from components.java — so the POM carries no dependencies. A consumer of biz.ganttproject.mxgraph:2026-dev-json that calls JsonPainterImpl.toJson() will hit NoClassDefFoundError: com/fasterxml/jackson/... unless it happens to provide Jackson itself. Worth deciding explicitly: bundle it, or publish real dependency metadata.

(I could not run ./gradlew in this environment — the command needs approval — so I have not verified that 2.22.1 resolves or that the new tests pass. Please make sure CI is green.)

6. cloud.ganttproject.chart is not built anywhere

It's a standalone build (own wrapper, own settings.gradle.kts) like cloud.ganttproject.colloboque, which is fine and consistent — but colloboque has an explicit CI step (.github/workflows/gradle.yml:64) and the new module has none, and it isn't in the root settings.gradle either. So ChartRenderer.kt and its 8 tests are never compiled or run in CI. Note the test task needs headless Chrome (build.gradle.kts:12-16) when you wire it up.


Design-level suggestions

  • Three hand-maintained copies of one model. The shape lives in JsonPainterImpl's string literals, in chart_model.ts, and in ChartModel.kt — and items 3 and 4 above are already the result of drift between them. Since -Xgenerate-dts is enabled anyway, consider defining the model once as @Serializable data classes in a multiplatform module (kotlinx.serialization works on both JVM and JS) and deleting the hand-written declarations. That also removes the untyped Map<String, Any?> from the JVM side.
  • The wire format speaks mxGraph. opacity as 0–100, dashed as 1/0, the "none" colour sentinel, mxConstants keys — these are mxGraph implementation details leaking into a public client-facing JSON contract, and ChartRenderer has to translate them back (MX_NONE, /100.0, == 1.0). Normalising at the JsonPainterImpl boundary (0–1 opacity, boolean dashed, absent instead of "none") would make the format self-explanatory and the renderer simpler.
  • Naming. An interface called PainterImpl reads oddly; PainterBackend / ChartPainterBackend would say what it is. Relatedly, MxGraphPainter and MxTextPainter are no longer mxGraph-specific — the KDoc now says so explicitly, so the names invite confusion.
  • Breaking API change. MxGraphPainter(uiConfig)MxGraphPainter(uiConfig, painter) plus the removal of getGraphXml() breaks the cloud consumer. A default argument (painter: PainterImpl = MxPainterImpl()) would keep source compatibility for the mxGraph path; either way it's worth calling out in the PR description since the artifact is published.
  • Nulls in the JSON. MxGraphPainter.paint(rhombus) (:118-119) can put null into the style map, so the output contains "fillColor": null. drawChart copes, but that violates the declared TS index signature StyleValue | undefined and would be rejected under strictNullChecks. mapper.setSerializationInclusion(NON_NULL) fixes it at the source.
  • JsonPainterImpl allocates its own ObjectMapper per instance (:33) — a companion object constant is enough. And paintText stores the label twice, at "text" and inside attributes (:74,76); one of them is redundant.

Nits

  • ChartRenderer.kt:139-144withOpacity calls save()/restore() without try/finally, so an exception mid-primitive leaves globalAlpha/lineWidth/lineDash applied to everything after it.
  • ChartRenderer.kt:127-130 — the KDoc says "opacity, stroke width and dash style", but withShapeStyle never touches the dash style.
  • ChartRenderer.kt:53-55rectangles/lines/rhombuses are typed non-null, so a JS caller that omits an empty collection gets a TypeError rather than an empty chart. Cheap to make tolerant given drawChart is the exported entry point.
  • ChartRenderer.kt:38-39 — arrow-head geometry is fixed at 10×8px while Canvas.Line.arrow carries real dimensions that MxGraphPainter.kt:102-103 only uses as a boolean.
  • ChartModel.kt:19-20@file:JsExport over a file containing only external interface declarations doesn't do anything (externals aren't emitted); worth removing so it doesn't read as "these types are exported". Relatedly Attributes is an empty interface, so the binding data (taskId, …) is unreachable from Kotlin — an operator fun Attributes.get(key: String): String? over asDynamic() would make it usable for hit-testing later.
  • drawChart ignores devicePixelRatio, so output is blurry on HiDPI displays. Fine for now, but the canvas sizing will need to move somewhere.
  • MxGraphPainter.kt:73-79catch (exception: Exception) { throw exception } is a no-op (pre-existing); could go away with the finally kept.
    · branch dbarashev/json-chart-painter

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

Test Results

360 tests  +9   360 ✅ +9   15s ⏱️ -7s
 97 suites +1     0 💤 ±0 
 97 files   +1     0 ❌ ±0 

Results for commit c960690. ± Comparison against base commit e523bed.

♻️ This comment has been updated with latest results.

Bugfix: the rendering order is not preserved.
Enhancement: texts are now recognized
Bugfix: stroke width styles
@dbarashev
dbarashev marked this pull request as draft August 10, 2026 11:12
@dbarashev
dbarashev marked this pull request as ready for review August 10, 2026 11:12
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @dbarashev's task in 5m 19s —— View job


Review: JSON chart painter (round 2)

  • Gather context (repo guidelines, PR diff)
  • Review JVM-side painter changes (biz.ganttproject.mxgraph)
  • Review Kotlin/JS chart module (cloud.ganttproject.chart)
  • Review build configuration & CI wiring
  • Post review findings

Good progress since the last round. Three of the five earlier blockers are genuinely fixed:

  • Z-order — the flat primitives list with a type discriminator is the right model, and JsonPainterTest.kt:186-201 / DrawChartTest.kt:226-246 pin the ordering from both ends. This also fixes the cross-canvas problem, since insertion order is now the only thing that matters.
  • Texts — emitted, declared in all three models, rendered, and the placement formulas in ChartRenderer.kt:166-186 are a faithful transcription of TextPainter.paint() (I checked left/center/right and top/middle/bottom against TextPainter.java:78-87 — they agree, including the (left-right)/2 centering).
  • strokeWidth / dashesPainterUtils.kt:46-61 now emits STYLE_STROKEWIDTH and a real STYLE_DASH_PATTERN, the renderer parses it, and there are tests for the solid, dashed, custom-pattern and malformed-pattern cases.

Two are still open (items 1 and 2 below), and reading the text/border code more closely surfaced a few fidelity gaps that will show up as soon as a real chart model is fed to the renderer.


1. version = "2026-dev-json" is still hardcoded

biz.ganttproject.mxgraph/build.gradle:61. addPublishing(project) wires the GCS repo and GitHub Packages, so this is the version publish uploads. Still looks like a leftover from the local-publishing commit.

Fix this →

2. Jackson: version drift, and unreachable from the published artifact

  • build.gradle:24 pins jackson-databind:2.22.1; biz.ganttproject.app.libs:44-46 and biz.ganttproject.core:21 all use 2.19.1. Compile against one, run against the other wherever both are on the classpath.
  • Jackson is in implementation but not in direct, so it is in neither the fat jar (jar { from configurations.direct }, :45-52) nor the Class-Path manifest entry; and the publication is artifact jar without from components.java, so the POM has no dependencies at all. A consumer calling JsonPainterImpl.toJson() gets NoClassDefFoundError: com/fasterxml/jackson/... unless it happens to provide Jackson itself.

3. cloud.ganttproject.chart still isn't built or tested anywhere

Not in the root settings.gradle, and no CI step (unlike cloud.ganttproject.colloboque, .github/workflows/gradle.yml:64). The CI run for this PR reports +9 tests — exactly the 9 in JsonPainterTest; the 16 tests in DrawChartTest never compile or run. That's the entire client-side renderer unverified, and the tests need headless Chrome (build.gradle.kts:12-16). I can't edit .github/workflows/** myself (GitHub App permission), so this one needs a human commit.

4. kotlinx-browser should be api, not implementation

cloud.ganttproject.chart/build.gradle.kts:28. drawChart(model: ChartModel, canvas: HTMLCanvasElement) has HTMLCanvasElement in its public signature, and in Kotlin 2.2 the org.w3c.dom declarations come from kotlinx-browser. Kotlin consumers only compile because they redeclare the dependency themselves — which is exactly what demo/build.gradle.kts:20 has to do.


Fidelity gaps in the model (these will bite on a real chart)

5. Per-side borders collapse into a full strokeRect

chart.properties defines borders edge by edge: task.border-top + task.border-bottom only, task.start adds border-left, task.end adds border-right. RectangleRenderer.renderBorders (RectangleRenderer.java:78-89) honours that — when the borders aren't homogeneous it draws only the defined edges. MxGraphPainter.paint(rectangle) reduces all of it to a single strokeColor/strokeWidth taken from border.top (PainterUtils.kt:33,40), and ChartRenderer.kt:96-99 strokes the whole rectangle. Net effect: every activity boundary inside a task bar gets a black vertical line that the desktop chart doesn't draw. The wire format has no way to express this today, so it's a contract decision (e.g. strokeTop/strokeLeft/… or a borders sub-object). Shared with the mxGraph backend, but this PR is where the JSON contract is being fixed.

Fix this →

6. Standalone texts carry no font

MxTextPainter.paint(text) (:37-39) passes emptyMap() as styles, so a Canvas.Text created via createText never gets STYLE_FONTSIZE / STYLE_FONTFAMILY — only text groups do, via AbstractTextPainter.paint(TextGroup)getFontStyles. The renderer then falls back to DEFAULT_FONT_SIZE = 10.0 / sans-serif (ChartRenderer.kt:152-153). Canvas.Text.font (the FontSpec that TextPainter.java:76 honours) is dropped too. Every task label and timeline label rendered from a real server model comes out at 10px regardless of the chart's font settings — and since the vertical placement is computed from fontSize, the baselines move as well.

7. Text background and padding box are never emitted

text.timeline.label.background-color = #FFFFFF with padding = 2 2 2 2 (chart.properties:51-52) — the white backdrop that makes TimelineLabelRendererImpl's labels readable over the grid and the "today" line. TextPainter.java:88-101 fills that rect and renders its borders; MxTextPainter emits only the spacings, so the client draws the label straight onto whatever is underneath.

8. STYLE_FONTCOLOR overrides the text-group line colour

MxTextPainter.kt:46-55 computes styles + style where style re-declares STYLE_FONTCOLOR to text.hexForegroundColor(). For a TextGroup, getFontStyles has already put the per-line colour there (Canvas.java:506FontChooser.getColor(lineStyle)), and TextPainter.paint(Text, Label, …) uses exactly that. The override replaces it with text.foregroundColor ?: BLACK, which for group lines is normally black. Pre-existing, and harmless with today's properties, but it silently discards the colour the desktop renderer would use.

9. 1px strokes land on integer coordinates

drawLine / strokeRect with lineWidth = 1 at integer coordinates straddles the pixel boundary, so a crisp desktop grid line becomes two half-covered columns in the browser. Your own test documents it: DrawChartTest.kt:206 asserts pixel[3] > 80 for a solid black line instead of == 255. The usual fix is a +0.5 offset for odd stroke widths (or translating the context by 0.5).


Smaller things

  • Nulls in the JSON. MxGraphPainter.paint(rhombus) (:116-117) still puts null into the style map when the style defines no background/border, so the output contains "fillColor": null. ChartRenderer copes, but it violates the declared fillColor?: string in chart_model.ts:43 and is rejected under strictNullChecks. mapper.setSerializationInclusion(NON_NULL) fixes it at the source. Same for "text": null, which chart_model.ts:145 declares as required text: string while ChartModel.kt:159 declares it nullable — pick one.
  • model.primitives is typed non-null (ChartModel.kt:44), so a JS caller that omits it gets a TypeError from forEach rather than an empty chart. drawChart is the exported entry point; cheap to be tolerant.
  • Arrow geometry is still hardcoded at 10×8 px (ChartRenderer.kt:62-63) while Canvas.Line.arrow carries real dimensions that MxGraphPainter.kt:101-102 reduces to a boolean.
  • withOpacity has no try/finally (ChartRenderer.kt:230-235): an exception mid-primitive leaves globalAlpha/lineWidth/lineDash applied to everything after it. Same file, :218-221: the KDoc of withShapeStyle still mentions "dash style", which it doesn't touch.
  • fontDescent measures on every text (:194-195) — measureText("Hg") per primitive is not free for a chart with hundreds of labels; it only depends on ctx.font, so it's cacheable.
  • JsonPainterImpl allocates an ObjectMapper per instance (:34); a companion object constant is enough.
  • ChartModel.kt:19@file:JsExport over a file that is almost entirely external interface declarations still doesn't export anything except PrimitiveType; and Attributes is an empty interface, so taskId & co. are unreachable from Kotlin. An operator fun Attributes.get(key: String): String? over asDynamic() would make it usable for hit-testing.
  • JsonPainterImpl.kt:64-67 — the KDoc says MxTextPainter "has already chosen the label which fits into the available space", but for standalone texts it takes getLabels(TextMetricsStub).firstOrNull() with stubbed metrics (7px per char), i.e. the longest candidate rather than a fitting one.
  • MxGraphPainter.kt:73-79catch (exception: Exception) { throw exception } is still a no-op; the finally alone does the job.

Design-level, carried over

  • The model is still hand-maintained in three placesJsonPainterImpl's string literals, chart_model.ts, ChartModel.kt — and items 5/6/7 plus the text: string vs String? drift above are all consequences of that. @Serializable data classes in a multiplatform module would collapse all three into one and remove the untyped Map<String, Any?>.
  • The wire format still speaks mxGraph: opacity 0–100, dashed as 1/0, the "none" sentinel, mxConstants keys — and ChartRenderer translates them back (MX_NONE, /100.0, == 1.0). Normalising at the JsonPainterImpl boundary would make the contract self-explanatory.
  • MxGraphPainter(uiConfig)MxGraphPainter(uiConfig, painter) plus the removal of getGraphXml() is a source-breaking change in a published artifact. painter: PainterImpl = MxPainterImpl() as a default would keep the mxGraph path source-compatible; either way worth calling out in the PR description now that MxPainterImpl is public and toXml() is the replacement.
  • Naming: PainterImpl for an interface still reads oddly (PainterBackend?), and MxGraphPainter/MxTextPainter are no longer mxGraph-specific.
  • drawChart still ignores devicePixelRatio — blurry on HiDPI. Fine for now, but canvas sizing will have to live somewhere.

I did not run Gradle in this environment (./gradlew needs approval, and the JS module would need to download the Kotlin/JS toolchain plus Chrome), so items 2, 4 and the binaries.library() UMD layout the vanilla demo relies on are unverified by execution. The JVM side is green in CI (+9 tests).
· branch dbarashev/json-chart-painter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant