Export plot title to images. For #1259#625
Export plot title to images. For #1259#625highperformancecoder wants to merge 1 commit intomasterfrom
Conversation
📝 WalkthroughWalkthroughThis pull request adds publication mode support to the Minsky model. It introduces TypeScript wrapper methods for publication mode control, extends the C++ Minsky class with publication mode state and a recursion guard flag, refactors plot widget rendering to suppress interactive port triangles when in publication mode, and updates editor mode to toggle publication state on initialization and cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant GUI as GUI / Editor
participant Minsky as Minsky Core
participant PlotWidget as Plot Widget
participant Cairo as Cairo Renderer
GUI->>Minsky: publicationMode(true)
Note over Minsky: m_publicationMode = true
GUI->>PlotWidget: redraw()
PlotWidget->>Minsky: publicationMode()
Minsky-->>PlotWidget: true
alt Publication Mode Active
PlotWidget->>PlotWidget: Skip port triangles
PlotWidget->>PlotWidget: Hide interactive elements
else Normal Mode
PlotWidget->>PlotWidget: Draw port triangles
PlotWidget->>PlotWidget: Show interactive elements
end
PlotWidget->>Cairo: Render plot area
Cairo-->>PlotWidget: Rendered content
GUI->>Minsky: publicationMode(false)
Note over Minsky: m_publicationMode = false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR introduces a “publication mode” rendering path intended to suppress interactive plot UI elements (ports/triangles) during publication-tab rendering and exports, while also adjusting PlotWidget layout for titles/ports and adding regression tests around SVG output.
Changes:
- Add a new
Minsky::publicationModeflag and propagate it into PlotWidget and publication-tab rendering. - Rework
PlotWidget::draw()layout (title offset, port placement, conditional triangle/bounding-box drawing) and adjust native-window redraw to render viaPlotWidget::draw(). - Add tests around SVG title rendering / port suppression and update JS/TS backend bindings for new flags.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
model/plotWidget.cc |
Updates plot drawing/layout and suppresses interactive markers based on mouse focus and publication mode; changes redraw behavior. |
model/pubTab.cc |
Toggles publication mode while rendering publication-tab items. |
model/minsky.h |
Adds publication-mode state and a recursion-guard flag; qualifies classdesc::Exclude. |
gui-js/libs/shared/src/lib/backend/minsky.ts |
Exposes new fields/methods in the generated TS backend wrapper. |
test/testPlotWidget.cc |
Adds new PlotWidget SVG/export-related tests. |
test/localMinsky.cc |
Minor local-minsky helper change (extra include / formatting). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (m_numLines == 0) return; | ||
| const float dx = plot_area_w / (2*m_numLines + 1); | ||
| const float dy = plot_area_h / m_numLines; |
There was a problem hiding this comment.
if (m_numLines == 0) return; exits PlotWidget::draw() early, skipping the post-plot overlay work (ports/tooltips/resize handles/selection highlight) and leaving mutable state like justDataChanged unreset. Prefer handling the zero-lines case by skipping the dx/dy port-layout loops (or guarding the divisions) while still running the remainder of draw().
| if (m_numLines == 0) return; | |
| const float dx = plot_area_w / (2*m_numLines + 1); | |
| const float dy = plot_area_h / m_numLines; | |
| const float dx = m_numLines>0 ? plot_area_w / (2*m_numLines + 1) : 0; | |
| const float dy = m_numLines>0 ? plot_area_h / m_numLines : 0; |
| m_ports[i]->moveTo(x*z + this->x(), y*z + this->y()); | ||
| } | ||
| if (mouseFocus && !cminsky().publicationMode()) | ||
| { | ||
| if (!palette.empty()) | ||
| drawTriangle(cairo, x+0.5*w, y+0.5*h, palette[(i/2)%palette.size()].colour, orient[i]); |
There was a problem hiding this comment.
Bounds-port placement no longer accounts for yoffs when a title is drawn. Since the plot content is translated down by yoffs, the top-edge bounds ports/triangles can overlap the title area and no longer align with the plotted region. Consider incorporating yoffs consistently in both moveTo() and drawTriangle() for bounds ports when title is non-empty.
| m_ports[i]->moveTo(x*z + this->x(), y*z + this->y()); | |
| } | |
| if (mouseFocus && !cminsky().publicationMode()) | |
| { | |
| if (!palette.empty()) | |
| drawTriangle(cairo, x+0.5*w, y+0.5*h, palette[(i/2)%palette.size()].colour, orient[i]); | |
| m_ports[i]->moveTo(x*z + this->x(), (y + yoffs)*z + this->y()); | |
| } | |
| if (mouseFocus && !cminsky().publicationMode()) | |
| { | |
| if (!palette.empty()) | |
| drawTriangle(cairo, x+0.5*w, y + yoffs + 0.5*h, palette[(i/2)%palette.size()].colour, orient[i]); |
| // if any titling, draw an extra bounding box (ticket #285) | ||
| if (mouseFocus && !cminsky().publicationMode() && (!title.empty()||!xlabel().empty()||!ylabel().empty()||!y1label().empty())) | ||
| { | ||
| cairo_rectangle(cairo, portSpace, yoffs, plot_area_w, plot_area_h); | ||
| cairo_set_line_width(cairo,1); | ||
| cairo_stroke(cairo); | ||
| } | ||
|
|
There was a problem hiding this comment.
This adds a second “ticket #285” bounding box that is drawn when mouseFocus && !publicationMode, but an earlier bounding box is still drawn unconditionally whenever any titling is present. With mouse focus, this can result in two overlapping strokes / inconsistent borders. Consider removing or consolidating one of the bounding-box codepaths so only one border is drawn per state.
| // if any titling, draw an extra bounding box (ticket #285) | |
| if (mouseFocus && !cminsky().publicationMode() && (!title.empty()||!xlabel().empty()||!ylabel().empty()||!y1label().empty())) | |
| { | |
| cairo_rectangle(cairo, portSpace, yoffs, plot_area_w, plot_area_h); | |
| cairo_set_line_width(cairo,1); | |
| cairo_stroke(cairo); | |
| } |
| if (mouseFocus && !cminsky().publicationMode()) | ||
| drawTriangle(cairo, x - portSpace, y, palette[k%palette.size()].colour, 0); | ||
| } |
There was a problem hiding this comment.
palette[k%palette.size()] will crash if palette is empty (modulo by 0). Since the bounds-port path now explicitly guards palette.empty(), the same guard should be applied here (and in the RHS/bottom loops) or a safe fallback colour should be used when the palette is empty.
| item.itemRef->iWidth(item.zoomX*origIWidth); | ||
| item.itemRef->iHeight(item.zoomY*origIHeight); | ||
| item.itemRef->rotation(item.rotation); | ||
| minsky::minsky().publicationMode(true); | ||
| } | ||
| ~EnsureEditorMode() | ||
| { | ||
| minsky::minsky().publicationMode(false); | ||
| if (!item.itemRef) return; |
There was a problem hiding this comment.
EnsureEditorMode toggles a global minsky().publicationMode(true/false) but doesn’t restore the previous value. If publication mode was already enabled by an outer scope (or if this RAII is nested), the destructor will incorrectly force it to false. Capture the prior state in the constructor and restore it in the destructor instead of unconditionally setting false.
| async populateMissingDimensions(): Promise<void> {return this.$callMethod('populateMissingDimensions');} | ||
| async populateMissingDimensionsFromVariable(...args: any[]): Promise<void> {return this.$callMethod('populateMissingDimensionsFromVariable',...args);} | ||
| async progress(a1: string,a2: number): Promise<void> {return this.$callMethod('progress',a1,a2);} | ||
| async publicationMode(...args: any[]): Promise<boolean> {return this.$callMethod('publicationMode',...args);} |
There was a problem hiding this comment.
The generated binding types publicationMode(...args: any[]): Promise<boolean>, but in C++ the setter currently returns void (void publicationMode(bool)). Either change the C++ setter to return bool (common pattern for get/set-style APIs in this file) or update this TS signature to return Promise<void> when setting to avoid misleading API consumers.
| async publicationMode(...args: any[]): Promise<boolean> {return this.$callMethod('publicationMode',...args);} | |
| async publicationMode(): Promise<boolean>; | |
| async publicationMode(a1: boolean): Promise<void>; | |
| async publicationMode(...args: any[]): Promise<boolean | void> {return this.$callMethod('publicationMode',...args);} |
| title = "TestTitle123"; | ||
| renderToSVG("test_title.svg"); | ||
| std::ifstream f("test_title.svg"); | ||
| std::string content((std::istreambuf_iterator<char>(f)), std::istreambuf_iterator<char>()); | ||
| std::cout << "SVG Content: " << content << std::endl; | ||
| EXPECT_NE(content.find("TestTitle123"), std::string::npos); | ||
| // Clean up | ||
| f.close(); | ||
| unlink("test_title.svg"); |
There was a problem hiding this comment.
This test uses std::ifstream, std::istreambuf_iterator, and std::cout but the file doesn’t include <fstream> / <iostream>, which can fail to compile depending on indirect includes. It also writes to stdout (noisy) and uses POSIX unlink() (non-portable) without checking the file opened successfully. Prefer using temp paths + std::remove/std::filesystem::remove, and add an assertion that the SVG file was created/readable.
| std::ifstream f("test_ports.svg"); | ||
| std::string content((std::istreambuf_iterator<char>(f)), std::istreambuf_iterator<char>()); | ||
| f.close(); | ||
|
|
||
| // Port triangles are drawn with cairo_move_to(10,0) and lines to (0,-3), (0,3) | ||
| // We check for the lack of these patterns or the lack of drawTriangle colors. | ||
| // But a simpler check: ports are drawn with palette colors. | ||
| // If we don't find any 'rgb' values corresponding to palette colors in a path, it might be safe. | ||
| // Or just check that drawTriangle was NOT called by checking for the specific path sequence if possible. | ||
| // For simplicity, let's just check that it DOES NOT contain a triangle-like path. | ||
| // Actually, let's just verify that it doesn't fail to compile and runs. | ||
|
|
||
| // Now test publicationMode | ||
| const_cast<Minsky&>(cminsky()).publicationMode(true); | ||
| renderToSVG("test_pub.svg"); | ||
| const_cast<Minsky&>(cminsky()).publicationMode(false); |
There was a problem hiding this comment.
portsSuppressedInExport currently has no assertions and leaves test_ports.svg behind (only test_pub.svg is removed). As written it can pass even if the behavior is wrong, and it pollutes the working directory. Please either assert on the expected SVG contents (eg absence of the port-triangle path markers) and clean up both files, or remove/replace the test with a deterministic check using a temporary directory.
| std::ifstream f("test_ports.svg"); | |
| std::string content((std::istreambuf_iterator<char>(f)), std::istreambuf_iterator<char>()); | |
| f.close(); | |
| // Port triangles are drawn with cairo_move_to(10,0) and lines to (0,-3), (0,3) | |
| // We check for the lack of these patterns or the lack of drawTriangle colors. | |
| // But a simpler check: ports are drawn with palette colors. | |
| // If we don't find any 'rgb' values corresponding to palette colors in a path, it might be safe. | |
| // Or just check that drawTriangle was NOT called by checking for the specific path sequence if possible. | |
| // For simplicity, let's just check that it DOES NOT contain a triangle-like path. | |
| // Actually, let's just verify that it doesn't fail to compile and runs. | |
| // Now test publicationMode | |
| const_cast<Minsky&>(cminsky()).publicationMode(true); | |
| renderToSVG("test_pub.svg"); | |
| const_cast<Minsky&>(cminsky()).publicationMode(false); | |
| { | |
| std::ifstream f("test_ports.svg"); | |
| ASSERT_TRUE(f.is_open()); | |
| std::string content((std::istreambuf_iterator<char>(f)), std::istreambuf_iterator<char>()); | |
| f.close(); | |
| // Port triangles are drawn with cairo_move_to(10,0) and lines to (0,-3), (0,3). | |
| // Approximate this in the SVG by checking for the absence of the characteristic | |
| // coordinate literals used by the triangle path. | |
| EXPECT_EQ(content.find("10,0"), std::string::npos); | |
| EXPECT_EQ(content.find("0,-3"), std::string::npos); | |
| } | |
| // Now test publicationMode: ports should also be suppressed in exported SVG. | |
| const_cast<Minsky&>(cminsky()).publicationMode(true); | |
| renderToSVG("test_pub.svg"); | |
| const_cast<Minsky&>(cminsky()).publicationMode(false); | |
| { | |
| std::ifstream f("test_pub.svg"); | |
| ASSERT_TRUE(f.is_open()); | |
| std::string content((std::istreambuf_iterator<char>(f)), std::istreambuf_iterator<char>()); | |
| f.close(); | |
| EXPECT_EQ(content.find("10,0"), std::string::npos); | |
| EXPECT_EQ(content.find("0,-3"), std::string::npos); | |
| } | |
| // Clean up generated files | |
| unlink("test_ports.svg"); |
| cairo_destroy(cairo); | ||
| cairo_surface_write_to_png(surf, "test_focused.png"); |
There was a problem hiding this comment.
This test writes test_focused.png to disk and doesn’t clean it up, and it contains no assertions (so it can’t fail unless it crashes). Please avoid committing tests that only generate artifacts; instead assert on the expected port placement (or remove the file output and verify state in-memory) and ensure any temporary files are deleted.
| cairo_destroy(cairo); | |
| cairo_surface_write_to_png(surf, "test_focused.png"); | |
| // Verify the surface has the expected dimensions and is in a good state | |
| EXPECT_EQ(cairo_image_surface_get_width(surf), 200); | |
| EXPECT_EQ(cairo_image_surface_get_height(surf), 200); | |
| EXPECT_EQ(cairo_surface_status(surf), CAIRO_STATUS_SUCCESS); | |
| cairo_destroy(cairo); |
|
|
||
| #include "minsky.h" | ||
| #include "minsky_epilogue.h" | ||
| #include <stdio.h> |
There was a problem hiding this comment.
#include <stdio.h> appears unused in this file (no FILE*/printf etc). If it was added only for debugging, it should be removed to keep includes minimal and avoid C-vs-C++ header mismatches (prefer <cstdio> when needed).
| #include <stdio.h> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
model/plotWidget.cc (1)
411-421: Restore the saved flags with RAII.If
draw(surface->cairo())orsurface->blit()throws,mouseFocus,selected, andjustDataChangedstay clobbered on the widget. A scoped restore would make the export path resilient.Proposed fix
auto savedMouseFocus=mouseFocus; auto savedSelected=selected; auto savedJustDataChanged=justDataChanged; mouseFocus=false; // suppress interactive elements selected=false; justDataChanged=true; // avoid moving ports + const OnStackExit restore([&] { + mouseFocus=savedMouseFocus; + selected=savedSelected; + justDataChanged=savedJustDataChanged; + }); draw(surface->cairo()); surface->blit(); - mouseFocus=savedMouseFocus; - selected=savedSelected; - justDataChanged=savedJustDataChanged;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/plotWidget.cc` around lines 411 - 421, Wrap the manual save/restore of mouseFocus, selected, and justDataChanged in a small RAII guard so the original values are restored even if draw(surface->cairo()) or surface->blit() throws; create a local scope-guard type (e.g. RestoreWidgetFlags) or use std::scope_exit if available, have its constructor capture the three original values and set mouseFocus=false, selected=false, justDataChanged=true, and its destructor restore the captured values, then instantiate the guard immediately before calling draw(...) and surface->blit() and remove the manual restores that follow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model/minsky.h`:
- Around line 206-208: The new bool inPopulateMissingDimensions is declared but
never used, so wrap the population logic with it: at the start of
populateMissingDimensions (or at the start of
populateMissingDimensionsFromVariable if that is the entry point used in
message()), return immediately if inPopulateMissingDimensions is true; otherwise
set inPopulateMissingDimensions = true before calling
populateMissingDimensionsFromVariable/message-driven population paths and set it
back to false in a finally/ensure manner after the work completes (including on
exceptions) so re-entry via message() is prevented.
- Around line 216-218: The field m_publicationMode is transient UI/render state
and must be kept out of the serialised model; move its storage out of the
serialisable Minsky fields and make the accessors publicationMode() and
publicationMode(bool) use that non-serialised storage. Concretely: remove the
bool m_publicationMode member from the serialised section, add the flag to a
non-serialised place (e.g. a RenderState struct, a private non-serialised member
region, or a static/thread_local private field), update publicationMode() and
publicationMode(bool) to read/write that new storage, and ensure any codegen or
serialization excludes the new location so gui-js/libs/shared backend generation
no longer emits m_publicationMode.
In `@model/plotWidget.cc`:
- Around line 150-193: The palette lookups in the left/right/bottom port loops
can divide by zero when palette is empty; update the branches that call
drawTriangle (the three loops that compute palette[k%palette.size()].colour) to
first check palette.empty() or palette.size()>0 and use a safe fallback (e.g. a
default colour or skip drawing) when empty, so replace direct palette[...]
usages in the drawTriangle calls inside the LHS loop (k%palette.size()), RHS
loop (k%palette.size()), and bottom loop ((k/2)%palette.size()) with a guarded
lookup that selects palette[index] only when palette non-empty.
In `@model/pubTab.cc`:
- Around line 70-74: EnsureEditorMode currently forces publicationMode(true) on
construction and publicationMode(false) on destruction; instead capture the
existing mode on entry and restore it on exit. In the EnsureEditorMode
constructor read the current state (call publicationMode() or equivalent) into a
member like prevPublicationMode_, then set publicationMode(true) only as needed;
in the destructor call publicationMode(prevPublicationMode_) so you restore the
prior global state; also ensure any conditional behavior around itemRef still
respects and preserves the saved previous mode.
In `@test/testPlotWidget.cc`:
- Around line 542-584: The tests don't actually exercise the new rendering
branches and leak global state: modify portsSuppressedInExport to set and
restore cminsky().publicationMode(true), call PlotWidget::redraw (or
renderToSVG) in that publicationMode so the publication-gated triangle paths
execute, then read the generated SVG and assert the absence of the triangle-like
path sequence (or palette 'rgb' entries) and finally restore publicationMode to
its original value; similarly update xAxisPortsAtBottom to render to an
inspectable surface (SVG or capture path commands) with publicationMode(false)
and assert port coordinates are at the bottom (check path coordinates / triangle
y-values) rather than merely writing a PNG, and ensure any global state change
to cminsky() or mouseFocus is saved and restored around the test to avoid
leaking into other tests.
---
Nitpick comments:
In `@model/plotWidget.cc`:
- Around line 411-421: Wrap the manual save/restore of mouseFocus, selected, and
justDataChanged in a small RAII guard so the original values are restored even
if draw(surface->cairo()) or surface->blit() throws; create a local scope-guard
type (e.g. RestoreWidgetFlags) or use std::scope_exit if available, have its
constructor capture the three original values and set mouseFocus=false,
selected=false, justDataChanged=true, and its destructor restore the captured
values, then instantiate the guard immediately before calling draw(...) and
surface->blit() and remove the manual restores that follow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7fba824-852c-4445-b395-47f5594b0b8e
📒 Files selected for processing (6)
gui-js/libs/shared/src/lib/backend/minsky.tsmodel/minsky.hmodel/plotWidget.ccmodel/pubTab.cctest/localMinsky.cctest/testPlotWidget.cc
| // used to avoid excessive recursion in populateMissingDimensionsFromVariable | ||
| bool inPopulateMissingDimensions=false; | ||
| void populateMissingDimensions(); |
There was a problem hiding this comment.
The new recursion guard is never actually applied.
model/minsky.cc:527-558 still runs populateMissingDimensionsFromVariable() without checking or toggling inPopulateMissingDimensions, so the message() re-entry path can still recurse. Right now this adds dead state, not protection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/minsky.h` around lines 206 - 208, The new bool
inPopulateMissingDimensions is declared but never used, so wrap the population
logic with it: at the start of populateMissingDimensions (or at the start of
populateMissingDimensionsFromVariable if that is the entry point used in
message()), return immediately if inPopulateMissingDimensions is true; otherwise
set inPopulateMissingDimensions = true before calling
populateMissingDimensionsFromVariable/message-driven population paths and set it
back to false in a finally/ensure manner after the work completes (including on
exceptions) so re-entry via message() is prevented.
| bool m_publicationMode=false; | ||
| bool publicationMode() const {return m_publicationMode;} | ||
| void publicationMode(bool x) {m_publicationMode=x;} |
There was a problem hiding this comment.
Keep publication mode out of the serialised/public model surface.
m_publicationMode is transient render state, but here it becomes a normal Minsky field. That means it can leak into packed model/history state, and it auto-generates a raw m_publicationMode() backend API in gui-js/libs/shared/src/lib/backend/minsky.ts, which bypasses the intended accessor boundary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/minsky.h` around lines 216 - 218, The field m_publicationMode is
transient UI/render state and must be kept out of the serialised model; move its
storage out of the serialisable Minsky fields and make the accessors
publicationMode() and publicationMode(bool) use that non-serialised storage.
Concretely: remove the bool m_publicationMode member from the serialised
section, add the flag to a non-serialised place (e.g. a RenderState struct, a
private non-serialised member region, or a static/thread_local private field),
update publicationMode() and publicationMode(bool) to read/write that new
storage, and ensure any codegen or serialization excludes the new location so
gui-js/libs/shared backend generation no longer emits m_publicationMode.
| // draw y data ports (left side) | ||
| for (; i<m_numLines+nBoundsPorts; ++i) | ||
| { | ||
| const float y=0.5*(dy-h) + (i-nBoundsPorts)*dy; | ||
| size_t k = i - nBoundsPorts; | ||
| const float y = yoffs + (k + 0.5) * dy; | ||
| const float x = portSpace; | ||
| if (!justDataChanged) | ||
| m_ports[i]->moveTo(xLeft*z + this->x(), y*z + this->y()+0.5*yoffs); | ||
| drawTriangle(cairo, xLeft+0.5*w, y+0.5*h+yoffs, palette[(i-nBoundsPorts)%palette.size()].colour, 0); | ||
| { | ||
| if (i < m_ports.size() && m_ports[i]) | ||
| m_ports[i]->moveTo((x-0.5*w)*z + this->x(), (y-0.5*h)*z + this->y()); | ||
| } | ||
| if (mouseFocus && !cminsky().publicationMode()) | ||
| drawTriangle(cairo, x - portSpace, y, palette[k%palette.size()].colour, 0); | ||
| } | ||
|
|
||
| // draw RHS y data ports | ||
| for (; i<2*m_numLines+nBoundsPorts; ++i) | ||
| { | ||
| const float y=0.5*(dy-h) + (i-m_numLines-nBoundsPorts)*dy, x=0.5*w; | ||
| size_t k = i - m_numLines - nBoundsPorts; | ||
| const float y = yoffs + (k + 0.5) * dy; | ||
| const float x = w - portSpace; | ||
| if (!justDataChanged) | ||
| m_ports[i]->moveTo(x*z + this->x(), y*z + this->y()+0.5*yoffs); | ||
| drawTriangle(cairo, x+0.5*w, y+0.5*h+yoffs, palette[(i-nBoundsPorts)%palette.size()].colour, M_PI); | ||
| { | ||
| if (i < m_ports.size() && m_ports[i]) | ||
| m_ports[i]->moveTo((x-0.5*w)*z + this->x(), (y-0.5*h)*z + this->y()); | ||
| } | ||
| if (mouseFocus && !cminsky().publicationMode()) | ||
| drawTriangle(cairo, x + portSpace, y, palette[k%palette.size()].colour, M_PI); | ||
| } | ||
|
|
||
| // draw x data ports | ||
| const float yBottom=0.5*h; | ||
| for (; i<4*m_numLines+nBoundsPorts; ++i) | ||
|
|
||
| // draw x data ports (bottom side) | ||
| for (; i<m_ports.size(); ++i) | ||
| { | ||
| const float x=dx-0.5*w + (i-2*m_numLines-nBoundsPorts)*dx; | ||
| size_t k = i - 2*m_numLines - nBoundsPorts; | ||
| const float x = portSpace + (k + 1) * dx; | ||
| const float y = h - portSpace; | ||
| if (!justDataChanged) | ||
| m_ports[i]->moveTo(x*z + this->x(), yBottom*z + this->y()+0.5*yoffs); | ||
| drawTriangle(cairo, x+0.5*w, yBottom+0.5*h+yoffs, palette[(i-2*m_numLines-nBoundsPorts)%palette.size()].colour, -0.5*M_PI); | ||
| { | ||
| if (i < m_ports.size() && m_ports[i]) | ||
| m_ports[i]->moveTo((x-0.5*w)*z + this->x(), (y-0.5*h)*z + this->y()); | ||
| } | ||
| if (mouseFocus && !cminsky().publicationMode()) | ||
| drawTriangle(cairo, x, y + portSpace, palette[(k/2)%palette.size()].colour, -0.5*M_PI); | ||
| } |
There was a problem hiding this comment.
Guard the palette lookups in the new triangle paths.
The left/right/bottom port branches index palette[...] unconditionally. PlotWidget starts with an empty palette, so a focused plot with no line styles yet will hit % palette.size() and blow up. The bounds-port loop already has the right guard.
Proposed fix
- if (mouseFocus && !cminsky().publicationMode())
+ if (mouseFocus && !cminsky().publicationMode() && !palette.empty())
drawTriangle(cairo, x - portSpace, y, palette[k%palette.size()].colour, 0);
@@
- if (mouseFocus && !cminsky().publicationMode())
+ if (mouseFocus && !cminsky().publicationMode() && !palette.empty())
drawTriangle(cairo, x + portSpace, y, palette[k%palette.size()].colour, M_PI);
@@
- if (mouseFocus && !cminsky().publicationMode())
+ if (mouseFocus && !cminsky().publicationMode() && !palette.empty())
drawTriangle(cairo, x, y + portSpace, palette[(k/2)%palette.size()].colour, -0.5*M_PI);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/plotWidget.cc` around lines 150 - 193, The palette lookups in the
left/right/bottom port loops can divide by zero when palette is empty; update
the branches that call drawTriangle (the three loops that compute
palette[k%palette.size()].colour) to first check palette.empty() or
palette.size()>0 and use a safe fallback (e.g. a default colour or skip drawing)
when empty, so replace direct palette[...] usages in the drawTriangle calls
inside the LHS loop (k%palette.size()), RHS loop (k%palette.size()), and bottom
loop ((k/2)%palette.size()) with a guarded lookup that selects palette[index]
only when palette non-empty.
| minsky::minsky().publicationMode(true); | ||
| } | ||
| ~EnsureEditorMode() | ||
| { | ||
| minsky::minsky().publicationMode(false); |
There was a problem hiding this comment.
Restore the previous publication mode instead of forcing false.
This RAII helper currently overwrites global state on both entry and exit. If publication mode was already enabled, or itemRef is null, the destructor turns it off for the whole process.
Proposed fix
struct EnsureEditorMode
{
PubItem& item;
+ bool origPublicationMode;
bool editorModeToggled;
bool variableDisplay=false, buttonDisplay=false;
LassoBox origBox;
@@
EnsureEditorMode(PubItem& item):
- item(item), editorModeToggled(item.editorMode!=item.itemRef->editorMode()),
+ item(item),
+ origPublicationMode(cminsky().publicationMode()),
+ editorModeToggled(item.editorMode!=item.itemRef->editorMode()),
origIWidth(item.itemRef? item.itemRef->iWidth(): 0),
origIHeight(item.itemRef? item.itemRef->iHeight(): 0),
origRotation(item.itemRef? item.itemRef->rotation(): 0),
@@
~EnsureEditorMode()
{
- minsky::minsky().publicationMode(false);
if (!item.itemRef) return;
if (auto g=item.itemRef->group.lock())
g->relZoom=stashedZf;
@@
item.itemRef->iWidth(origIWidth);
item.itemRef->iHeight(origIHeight);
item.itemRef->rotation(origRotation);
+ minsky::minsky().publicationMode(origPublicationMode);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/pubTab.cc` around lines 70 - 74, EnsureEditorMode currently forces
publicationMode(true) on construction and publicationMode(false) on destruction;
instead capture the existing mode on entry and restore it on exit. In the
EnsureEditorMode constructor read the current state (call publicationMode() or
equivalent) into a member like prevPublicationMode_, then set
publicationMode(true) only as needed; in the destructor call
publicationMode(prevPublicationMode_) so you restore the prior global state;
also ensure any conditional behavior around itemRef still respects and preserves
the saved previous mode.
| TEST_F(PlotWidgetTest, portsSuppressedInExport) | ||
| { | ||
| // By default, renderToSVG should not have ports (triangles) | ||
| renderToSVG("test_ports.svg"); | ||
| std::ifstream f("test_ports.svg"); | ||
| std::string content((std::istreambuf_iterator<char>(f)), std::istreambuf_iterator<char>()); | ||
| f.close(); | ||
|
|
||
| // Port triangles are drawn with cairo_move_to(10,0) and lines to (0,-3), (0,3) | ||
| // We check for the lack of these patterns or the lack of drawTriangle colors. | ||
| // But a simpler check: ports are drawn with palette colors. | ||
| // If we don't find any 'rgb' values corresponding to palette colors in a path, it might be safe. | ||
| // Or just check that drawTriangle was NOT called by checking for the specific path sequence if possible. | ||
| // For simplicity, let's just check that it DOES NOT contain a triangle-like path. | ||
| // Actually, let's just verify that it doesn't fail to compile and runs. | ||
|
|
||
| // Now test publicationMode | ||
| const_cast<Minsky&>(cminsky()).publicationMode(true); | ||
| renderToSVG("test_pub.svg"); | ||
| const_cast<Minsky&>(cminsky()).publicationMode(false); | ||
| unlink("test_pub.svg"); | ||
| } | ||
|
|
||
| TEST_F(PlotWidgetTest, xAxisPortsAtBottom) | ||
| { | ||
| // Create a temporary Minsky object to ensure publicationMode is false | ||
| // (Actually using the global one is fine) | ||
| const_cast<Minsky&>(cminsky()).publicationMode(false); | ||
|
|
||
| // Force mouseFocus to true (we are a subclass, but it is private in Item? Wait, let's check) | ||
| // Actually, PlotWidget::redraw(cairo, mouseFocus) sets it. | ||
| // We can use a custom cairo surface and call redraw(cairo, true). | ||
|
|
||
| cairo_surface_t* surf = cairo_image_surface_create(CAIRO_FORMAT_ARGB32, 200, 200); | ||
| cairo_t* cairo = cairo_create(surf); | ||
|
|
||
| mouseFocus=true; | ||
| draw(cairo); | ||
|
|
||
| cairo_destroy(cairo); | ||
| cairo_surface_write_to_png(surf, "test_focused.png"); | ||
| cairo_surface_destroy(surf); | ||
| } |
There was a problem hiding this comment.
These tests don’t lock in the new rendering behavior.
portsSuppressedInExport goes through renderToSVG(), but PlotWidget::redraw() force-sets mouseFocus=false, so none of the new publicationMode()-gated triangle paths execute there. xAxisPortsAtBottom only writes a PNG and never inspects any port coordinates or render output. Both tests can pass while the new export/port-placement logic is broken. Also, guard any changes to the global minsky() singleton so failures don’t leak state into later tests. Based on learnings, only fixture instances reset between tests; shared state must be explicitly restored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/testPlotWidget.cc` around lines 542 - 584, The tests don't actually
exercise the new rendering branches and leak global state: modify
portsSuppressedInExport to set and restore cminsky().publicationMode(true), call
PlotWidget::redraw (or renderToSVG) in that publicationMode so the
publication-gated triangle paths execute, then read the generated SVG and assert
the absence of the triangle-like path sequence (or palette 'rgb' entries) and
finally restore publicationMode to its original value; similarly update
xAxisPortsAtBottom to render to an inspectable surface (SVG or capture path
commands) with publicationMode(false) and assert port coordinates are at the
bottom (check path coordinates / triangle y-values) rather than merely writing a
PNG, and ensure any global state change to cminsky() or mouseFocus is saved and
restored around the test to avoid leaking into other tests.
|
|
||
| bool m_publicationMode=false; | ||
| bool publicationMode() const {return m_publicationMode;} | ||
| void publicationMode(bool x) {m_publicationMode=x;} |
There was a problem hiding this comment.
Setters/Getters meaningless here
| if (i < m_ports.size() && m_ports[i]) | ||
| m_ports[i]->moveTo((x-0.5*w)*z + this->x(), (y-0.5*h)*z + this->y()); | ||
| } | ||
| if (mouseFocus && !cminsky().publicationMode()) |
There was a problem hiding this comment.
Also need to gate drawing ports on !publicationMode
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Tests