Copilot/refactor item draw method#656
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughText rendering throughout the codebase has been refactored from direct Pango manipulation to an abstraction layer using ChangesText rendering abstraction and integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors item/text rendering to route more drawing through ICairoShim (including a new TextProperties/ICacheRender abstraction), updates Cairo shim implementations accordingly, and refreshes SVG golden test outputs to match the new rendering behavior.
Changes:
- Introduce
TextProperties,TextExtents, andICacheRenderto replace directPangousage in shim-facing APIs. - Refactor variable/operation/item/intOp drawing code to use
ICairoShim::showText()/textExtents()and cached text rendering. - Update stored SVG outputs under
test/oldSchema/to reflect the refactored rendering pipeline.
Reviewed changes
Copilot reviewed 15 out of 76 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/oldSchema/schema2/UWS05.mky.svg | Updated golden SVG output after rendering refactor. |
| test/oldSchema/schema2/math-examples.mky.svg | Updated golden SVG output after rendering refactor. |
| test/oldSchema/schema2/dense.mky.svg | Updated golden SVG output after rendering refactor. |
| test/oldSchema/schema2/data-example.mky.svg | Updated golden SVG output after rendering refactor. |
| test/oldSchema/schema2/1Free.mky.svg | Updated golden SVG output after rendering refactor. |
| test/oldSchema/schema1/math-examples.mky.svg | Updated golden SVG output after rendering refactor. |
| test/oldSchema/schema1/data-example.mky.svg | Updated golden SVG output after rendering refactor. |
| test/oldSchema/schema0/UWS05.mky.svg | Updated golden SVG output after rendering refactor. |
| test/oldSchema/schema0/1Free.mky.svg | Updated golden SVG output after rendering refactor. |
| test/Makefile | Make dependency includes optional and add localMinsky.d. |
| model/variablePane.cc | Adapt variable pane rendering to use CairoShimCairo + shim-based RenderVariable. |
| model/variable.h | Switch cached mantissa/exponent render caches to ICacheRender. |
| model/variable.cc | Refactor variable drawing to use shim text rendering/caching and shim-based RenderVariable. |
| model/operation.h | Move DrawBinOpShim::drawSymbol out-of-line. |
| model/operation.cc | Replace many icon text draws with shim text APIs and updated positioning. |
| model/item.cc | Replace direct Pango usage with shim text APIs for detailed text and tooltips. |
| model/intOp.cc | Refactor integral operator drawing to use shim-based RenderVariable and shim transforms. |
| model/ICairoShim.h | Add text/caching abstractions and remove direct Pango exposure from the interface. |
| model/group.cc | Update group title measurement to use shim text extents. |
| model/godleyIcon.cc | Update title drawing to use shim TextProperties. |
| model/cairoItems.h | Refactor RenderVariable away from inheriting Pango; introduce cached renderer. |
| model/cairoItems.cc | Implement new shim-based RenderVariable construction and measurement. |
| engine/cairoShimCairo.h | Update shim interface to implement new text API; add context(). |
| engine/cairoShimCairo.cc | Implement TextProperties rendering/extents using ecolab::Pango internally. |
Comments suppressed due to low confidence (1)
model/group.cc:1040
tp(font size, markup) is used to compute the text extents, but the actual draw call usesshowText(title), which drops the configured font size and can make the label positioning/background wrong. Render using the sameTextPropertiesinstance used for measurement (and optionally settp.fontFamilyto preserve the previous sans-serif intent).
TextProperties tp;
tp.fontSize=12;
// extract the bounding box of the text
tp.markup=title;
auto bbox=cairoShim.textExtents(tp);
const double w=0.5*bbox.width+2;
const double h=0.5*bbox.height+5;
// if rotation is in 1st or 3rd quadrant, rotate as
// normal, otherwise flip the text so it reads L->R
if (flipped)
cairoShim.rotate(M_PI);
// prepare a background for the text, partially obscuring graphic
const double transparency=displayContents()? 0.25: 1;
// display text
cairoShim.moveTo(-w+1, h-12-0.5*(height)/z);
cairoShim.setSourceRGBA(0,0,0,transparency);
cairoShim.showText(title);
cairoShim.restore();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
model/ICairoShim.h (1)
50-73: 💤 Low value
NonCachedRendererstores a reference toICairoShim- ensure lifetime safety.The
shimreference must remain valid for the lifetime ofNonCachedRenderer. This is typically fine when used transiently, but could cause dangling references if the renderer outlives its shim. Given the usage pattern (factory returnsunique_ptrfrom the shim itself), callers should be careful not to store these renderers beyond the shim's lifetime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/ICairoShim.h` around lines 50 - 73, NonCachedRenderer currently stores a const ICairoShim& (shim) which can dangle if the shim outlives its owner; change the member to an owning or at least nullable pointer type and update the constructor and methods to accept and store that pointer (e.g., replace "const ICairoShim& shim" with "const ICairoShim* shim" or "std::shared_ptr<const ICairoShim> shim"), validate the pointer in the constructor (or require shared_ptr to extend lifetime), and update references to shim in show(), extents(), and context() accordingly so the renderer no longer holds a dangling reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model/cairoItems.cc`:
- Around line 54-55: The constructor RenderVariable::RenderVariable(const
VariableBase& var, const ICairoShim& shim) initializes members in the wrong
order; update the initializer list to match the declaration order in
cairoItems.h by initializing var first and then cairoShim (i.e., use var(var),
cairoShim(shim)) so member initialization order matches the class declaration
and removes the warning/error.
In `@model/group.cc`:
- Around line 1019-1025: The title measurement and rendering use different
payloads: you set TextProperties tp (tp.markup=title) and measure with
cairoShim.textExtents(tp) but later render the raw title string; change the
render call(s) to use the same TextProperties instance (tp) so measurement and
drawing use identical markup/plain settings—update any draw/textExtents usage
around the title handling to pass tp (e.g., to the cairo draw/text routine)
instead of the plain title string.
In `@model/variable.cc`:
- Around line 730-731: Replace the unsafe reinterpret_cast usage when obtaining
the Cairo context by performing a runtime-checked cast to the concrete type: use
dynamic_cast<const CairoShimCairo&>(cairoShim) (or dynamic_cast<const
CairoShimCairo*>(...) with a null check to avoid exceptions) before calling
_internalGetCairoContext(), and handle the failure (throw/catch std::bad_cast or
return/log an error) so an invalid cairoShim does not invoke undefined behavior;
apply this change for the occurrence using
cairoShim/CairoShimCairo/_internalGetCairoContext() and the second identical
occurrence referenced in the review.
- Around line 796-799: The exponent TextProperties isn't rotated like the
mantissa because exponentText.angle is not set; update the code that builds
exponentText (the call to expMultiplier / where exponentText is assigned) to set
exponentText.angle = angle + (flipped ? M_PI : 0) so it uses the same rotation
as mantissaText (which currently uses mantissaText.angle = angle + (flipped ?
M_PI : 0)); keep the existing fontSize assignment for exponentText.fontSize.
---
Nitpick comments:
In `@model/ICairoShim.h`:
- Around line 50-73: NonCachedRenderer currently stores a const ICairoShim&
(shim) which can dangle if the shim outlives its owner; change the member to an
owning or at least nullable pointer type and update the constructor and methods
to accept and store that pointer (e.g., replace "const ICairoShim& shim" with
"const ICairoShim* shim" or "std::shared_ptr<const ICairoShim> shim"), validate
the pointer in the constructor (or require shared_ptr to extend lifetime), and
update references to shim in show(), extents(), and context() accordingly so the
renderer no longer holds a dangling reference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebb06140-55a6-4342-afa7-63aa2a6cfbbd
⛔ Files ignored due to path filters (61)
test/oldSchema/schema0/1Free.mky.svgis excluded by!**/*.svgtest/oldSchema/schema0/4MonetaryMinskyModelLessUnstableStart.mky.svgis excluded by!**/*.svgtest/oldSchema/schema0/GoodwinLinear.mky.svgis excluded by!**/*.svgtest/oldSchema/schema0/GoodwinLinear02.mky.svgis excluded by!**/*.svgtest/oldSchema/schema0/Steve.mky.svgis excluded by!**/*.svgtest/oldSchema/schema0/SteveTest028.mky.svgis excluded by!**/*.svgtest/oldSchema/schema0/UWS05.mky.svgis excluded by!**/*.svgtest/oldSchema/schema0/exponentialGrowth.mky.svgis excluded by!**/*.svgtest/oldSchema/schema0/exponentialGrowthWithExtraLabel.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/1Free.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/4MonetaryMinskyModelLessUnstableStart.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/BasicGrowthModel.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/GoodwinLinear.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/GoodwinLinear02.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/LoanableFunds.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/MinskyGovernmentNonlinear.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/MinskyNonLinear.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/MinskyPricesFinal.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/MonetaryMinskyNeoPrices.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/PredatorPrey.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/Solow.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/Steve.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/SteveTest028.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/UWS05.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/data-example.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/exponentialGrowth.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/exponentialGrowthWithExtraLabel.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/math-examples.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/switchBlock.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/1Free.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/4MonetaryMinskyModelLessUnstableStart.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/BasicGrowthModel.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/EndogenousMoney.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/GoodwinLinear.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/GoodwinLinear02.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/LoanableFunds.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/MinskyGovernmentNonlinear.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/MinskyNonLinear.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/MinskyPricesFinal.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/MonetaryMinskyNeoPrices.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/PredatorPrey.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/Solow.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/Steve.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/SteveTest028.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/UWS05.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/data-example.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/dense.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/exponentialGrowth.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/exponentialGrowthWithExtraLabel.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/indexing.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/math-examples.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/reductionExample.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/switchBlock.mky.svgis excluded by!**/*.svgtest/renderedImages/allItemsBare.svgis excluded by!**/*.svgtest/renderedImages/allItemsColHeadAndTail.svgis excluded by!**/*.svgtest/renderedImages/allItemsColTail.svgis excluded by!**/*.svgtest/renderedImages/allItemsHeadAndTail.svgis excluded by!**/*.svgtest/renderedImages/allItemsMouseOver.svgis excluded by!**/*.svgtest/renderedImages/allItemsOnBorder.svgis excluded by!**/*.svgtest/renderedImages/allItemsSelected.svgis excluded by!**/*.svgtest/renderedImages/allItemsTail.svgis excluded by!**/*.svg
📒 Files selected for processing (15)
engine/cairoShimCairo.ccengine/cairoShimCairo.hmodel/ICairoShim.hmodel/cairoItems.ccmodel/cairoItems.hmodel/godleyIcon.ccmodel/group.ccmodel/intOp.ccmodel/item.ccmodel/operation.ccmodel/operation.hmodel/variable.ccmodel/variable.hmodel/variablePane.cctest/Makefile
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model/variable.cc (2)
772-803:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalidate the numeric text cache when rotation changes.
Lines 777-798 bake
angleintomantissaTextandexponentText, but the rebuild guard only keys off context andcachedValue. Rotating a variable without changing its value will reuse stale glyph orientation/extents, so the numeric overlay can be misplaced or drawn with the previous rotation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/variable.cc` around lines 772 - 803, The cache guard for numeric rendering only checks cairo context and cachedValue, but mantissaText.angle uses angle and flipped so rotating reuses stale cached glyphs; modify the invalidation check (the if before rebuilding mantissa/exponent) to also compare the current angle and flipped with stored cachedAngle/cachedFlipped (or include them in a cache key), and when rebuilding set cachedAngle=angle and cachedFlipped=flipped alongside cachedValue and cachedMantissa/cachedExponent so the cachedRender(cairoShim.cachedRender(...)) is regenerated when rotation changes.
717-719:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the local cache snapshot in the invalidation check.
Line 718 copies
cachedNameRenderto avoid cross-thread invalidation, but then dereferencescachedNameRenderagain instead ofl_cachedNameRender. That can observe a different pointer than the snapshot and undermines the race-avoidance here.Suggested fix
- if (!l_cachedNameRender || cairoShim.context()!=cachedNameRender->context()) + if (!l_cachedNameRender || cairoShim.context()!=l_cachedNameRender->context())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/variable.cc` around lines 717 - 719, The local snapshot l_cachedNameRender is created to avoid races but the invalidation check still dereferences cachedNameRender; change the check to use the snapshot instead (i.e. test !l_cachedNameRender or cairoShim.context() != l_cachedNameRender->context()) and when recreating assign the new shared_ptr to both cachedNameRender and l_cachedNameRender so subsequent use of l_cachedNameRender is consistent; update the condition around RenderVariable construction (symbols: l_cachedNameRender, cachedNameRender, cairoShim, RenderVariable, context()) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@model/variable.cc`:
- Around line 772-803: The cache guard for numeric rendering only checks cairo
context and cachedValue, but mantissaText.angle uses angle and flipped so
rotating reuses stale cached glyphs; modify the invalidation check (the if
before rebuilding mantissa/exponent) to also compare the current angle and
flipped with stored cachedAngle/cachedFlipped (or include them in a cache key),
and when rebuilding set cachedAngle=angle and cachedFlipped=flipped alongside
cachedValue and cachedMantissa/cachedExponent so the
cachedRender(cairoShim.cachedRender(...)) is regenerated when rotation changes.
- Around line 717-719: The local snapshot l_cachedNameRender is created to avoid
races but the invalidation check still dereferences cachedNameRender; change the
check to use the snapshot instead (i.e. test !l_cachedNameRender or
cairoShim.context() != l_cachedNameRender->context()) and when recreating assign
the new shared_ptr to both cachedNameRender and l_cachedNameRender so subsequent
use of l_cachedNameRender is consistent; update the condition around
RenderVariable construction (symbols: l_cachedNameRender, cachedNameRender,
cairoShim, RenderVariable, context()) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d45e8240-23f4-4142-bd69-40cfd420e089
📒 Files selected for processing (3)
model/cairoItems.ccmodel/operation.ccmodel/variable.cc
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model/variable.cc (1)
771-799:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInvalidate the value cache when slider formatting changes.
mantissaText.markupnow depends onsliderVisible(),vv->sliderStepRel, andvv->maxSliderSteps(), but the rebuild guard only checks the cairo context andvalue()!=cachedValue. If the slider mode/bounds/step change without changing the numeric value, the displayed mantissa/exponent stays stale until the value changes. Please include those formatting inputs in the cache key or resetcachedMantissa/cachedExponentfrom the slider setters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/variable.cc` around lines 771 - 799, The current rebuild guard only compares cairo context and value()!=cachedValue, so mantissa/exponent stay stale when slider formatting changes; update the cache check to also consider sliderVisible(), vv->sliderStepRel and vv->maxSliderSteps() (or compute and compare a small formatting-key derived from them) before skipping the cached render, or alternatively clear cachedMantissa/cachedExponent when slider setters change (invalidate in the setter code that updates vv->sliderStepRel, vv->maxSliderSteps or sliderVisible state). Ensure you reference and update l_cachedMantissa/cachedMantissa, l_cachedExponent/cachedExponent and the condition that uses value()!=cachedValue so formatting changes trigger a rebuild.model/intOp.cc (1)
53-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply
coupledIntTranslationbefore the restore or remove it.Line 170 translates the shim and Line 171 immediately restores the saved state, so this offset never affects the draw path or the port-coordinate transform. As written, the coupled-integrator width adjustment computed above is effectively inert, so the icon/port layout still uses the unshifted origin.
Also applies to: 154-171
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/intOp.cc` around lines 53 - 63, The computed coupledIntTranslation is never applied because cairoShim.restore() is called immediately after setting it; update the code in the coupled() && intVar block (the section using cairoShim.save(), cairoShim.rotate(angle), RenderVariable rv, and the coupledIntTranslation variable) so that the translation is applied to cairoShim before calling cairoShim.restore() (e.g. call the equivalent translate/apply method on cairoShim using coupledIntTranslation) or remove the coupledIntTranslation calculation entirely if you don't intend to translate; ensure the translation occurs while the saved state is active so it affects subsequent drawing and port-coordinate transforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@model/intOp.cc`:
- Around line 53-63: The computed coupledIntTranslation is never applied because
cairoShim.restore() is called immediately after setting it; update the code in
the coupled() && intVar block (the section using cairoShim.save(),
cairoShim.rotate(angle), RenderVariable rv, and the coupledIntTranslation
variable) so that the translation is applied to cairoShim before calling
cairoShim.restore() (e.g. call the equivalent translate/apply method on
cairoShim using coupledIntTranslation) or remove the coupledIntTranslation
calculation entirely if you don't intend to translate; ensure the translation
occurs while the saved state is active so it affects subsequent drawing and
port-coordinate transforms.
In `@model/variable.cc`:
- Around line 771-799: The current rebuild guard only compares cairo context and
value()!=cachedValue, so mantissa/exponent stay stale when slider formatting
changes; update the cache check to also consider sliderVisible(),
vv->sliderStepRel and vv->maxSliderSteps() (or compute and compare a small
formatting-key derived from them) before skipping the cached render, or
alternatively clear cachedMantissa/cachedExponent when slider setters change
(invalidate in the setter code that updates vv->sliderStepRel,
vv->maxSliderSteps or sliderVisible state). Ensure you reference and update
l_cachedMantissa/cachedMantissa, l_cachedExponent/cachedExponent and the
condition that uses value()!=cachedValue so formatting changes trigger a
rebuild.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 594e32c9-33f3-4e6f-9e1b-273f2b756210
⛔ Files ignored due to path filters (15)
test/oldSchema/schema1/1Free.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/GoodwinLinear02.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/MinskyGovernmentNonlinear.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/MinskyNonLinear.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/MinskyPricesFinal.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/MonetaryMinskyNeoPrices.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/exponentialGrowth.mky.svgis excluded by!**/*.svgtest/oldSchema/schema1/exponentialGrowthWithExtraLabel.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/1Free.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/GoodwinLinear02.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/MinskyGovernmentNonlinear.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/MinskyNonLinear.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/MinskyPricesFinal.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/MonetaryMinskyNeoPrices.mky.svgis excluded by!**/*.svgtest/oldSchema/schema2/exponentialGrowthWithExtraLabel.mky.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
model/ICairoShim.hmodel/cairoItems.ccmodel/cairoItems.hmodel/group.ccmodel/intOp.ccmodel/variable.cc
Owner
Author
@copilot - first issue is a non issue. The point is to display the value in a way that is useful to the user. I don't think the user expects the value display to change due to changes to the slider step, slider bounds. If the slider moves, then the value changes anyway, causing a recompute. Good catch on the second issue. I did a deep dive, and this inert line of code has been present since 2020, and looks like the result of a merge gone wrong, rather than a deliberate action by a porgrammer. |
Contributor
Addressed in commit |
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.
This change is
Summary by CodeRabbit
Refactor
New Features