feat: move display Godley table variable values as an attribute of Go…#662
Conversation
…dleyIcon rather than system preference. For feature #194.
|
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 (4)
💤 Files with no reviewable changes (3)
📝 WalkthroughWalkthroughThis PR refactors the Godley table display-values setting from a global ChangesGodley Table Display Values Refactoring
sequenceDiagram
participant ContextMenu
participant GodleyIcon
participant Canvas
ContextMenu->>GodleyIcon: toggleDisplayValues()
GodleyIcon-->>ContextMenu: done
ContextMenu->>Canvas: requestRedraw()
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
Moves the “show values” toggle for Godley tables from a global Minsky preference into a per-GodleyIcon attribute, persists it via schema 3, and surfaces it as a per-icon context menu checkbox while removing the global preferences UI/store entry. The Godley table output style (DRCR vs sign) remains a global setting.
Changes:
- Add
GodleyIcon::displayValues+toggleDisplayValues(), persist viaschema3::Item, update popup rendering to read from the icon. - Remove global
Minsky::displayValuesandsetGodleyDisplayValue, plus the GUI preferences form/HTML/store entry forgodleyTableShowValues; rewireGodleyMenuManager/CommandsManagerto useminsky.displayStyle(). - Expose
displayValues/toggleDisplayValuesin the TS backend wrapper and add a per-icon “Display values” item to the Godley context menu.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| model/godleyIcon.h | Adds per-icon displayValues field and toggle. |
| model/godleyTableWindow.cc | Reads display flag from owning GodleyIcon instead of global. |
| model/godleyTableWindow.h | Adds explanatory comment for godleyIcon() accessor. |
| model/minsky.h | Removes global displayValues member; comments out setGodleyDisplayValue declaration. |
| model/minsky.cc | Comments out old setGodleyDisplayValue implementation. |
| schema/schema3.h | Adds displayValues to schema and pack constructor. |
| schema/schema3.cc | Unpacks displayValues via toggleDisplayValues. |
| gui-js/libs/shared/src/lib/backend/minsky.ts | Moves displayValues/adds toggleDisplayValues from Minsky to GodleyIcon; drops setGodleyDisplayValue. |
| gui-js/libs/menu/.../preferences.component.ts/.html | Removes global godleyTableShowValues form control & UI. |
| gui-js/apps/minsky-electron/.../StoreManager.ts | Comments out godleyTableShowValues from persisted preferences. |
| gui-js/apps/minsky-electron/.../GodleyMenuManager.ts | Drops godleyTableShowValues branch; switches output-style to minsky.displayStyle(). |
| gui-js/apps/minsky-electron/.../CommandsManager.ts | Drops godleyTableShowValues from applyPreferences. |
| gui-js/apps/minsky-electron/.../ContextMenuManager.ts | Adds per-icon “Display values” menu item; redraws canvas on toggle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| click: async () => { | ||
| await godley.toggleDisplayValues(); | ||
| await minsky.canvas.requestRedraw(); | ||
| }, |
There was a problem hiding this comment.
Popups no longer use this code, but rather HTML table rendering.
There was a problem hiding this comment.
I should remove the no longer used popup member, but that is for a later refactor.
![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
🤖 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/godleyIcon.h`:
- Around line 86-88: toggleDisplayValues currently only flips the displayValues
flag and doesn't trigger a visual update; update toggleDisplayValues() in
GodleyIcon to call the popup redraw path after flipping the flag (e.g., if the
associated popup/window for this Godley table exists/is open, call
popup.requestRedraw() or the equivalent method used elsewhere to force a
repaint) so open popups immediately reflect the change.
🪄 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: 5ab76cab-6ee1-4eb9-9761-e1d02f499a8a
📒 Files selected for processing (14)
gui-js/apps/minsky-electron/src/app/managers/CommandsManager.tsgui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.tsgui-js/apps/minsky-electron/src/app/managers/GodleyMenuManager.tsgui-js/apps/minsky-electron/src/app/managers/StoreManager.tsgui-js/libs/menu/src/lib/options/preferences/preferences.component.htmlgui-js/libs/menu/src/lib/options/preferences/preferences.component.tsgui-js/libs/shared/src/lib/backend/minsky.tsmodel/godleyIcon.hmodel/godleyTableWindow.ccmodel/godleyTableWindow.hmodel/minsky.ccmodel/minsky.hschema/schema3.ccschema/schema3.h
💤 Files with no reviewable changes (2)
- gui-js/libs/menu/src/lib/options/preferences/preferences.component.ts
- gui-js/libs/menu/src/lib/options/preferences/preferences.component.html
| /// whether to display values in this table | ||
| bool displayValues=false; | ||
| void toggleDisplayValues() {displayValues=!displayValues;} |
There was a problem hiding this comment.
Redraw the popup when this flag changes.
toggleDisplayValues() only flips the bit. If this Godley table is already open in its popup window, it will keep showing the old rendering until some unrelated event triggers popup.requestRedraw(). The previous global path handled that redraw, so this refactor regresses live updates for open tables unless the per-icon toggle path does it too.
Suggested fix
- void toggleDisplayValues() {displayValues=!displayValues;}
+ void toggleDisplayValues()
+ {
+ displayValues=!displayValues;
+ popup.requestRedraw();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// whether to display values in this table | |
| bool displayValues=false; | |
| void toggleDisplayValues() {displayValues=!displayValues;} | |
| /// whether to display values in this table | |
| bool displayValues=false; | |
| void toggleDisplayValues() | |
| { | |
| displayValues=!displayValues; | |
| popup.requestRedraw(); | |
| } |
🤖 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/godleyIcon.h` around lines 86 - 88, toggleDisplayValues currently only
flips the displayValues flag and doesn't trigger a visual update; update
toggleDisplayValues() in GodleyIcon to call the popup redraw path after flipping
the flag (e.g., if the associated popup/window for this Godley table exists/is
open, call popup.requestRedraw() or the equivalent method used elsewhere to
force a repaint) so open popups immediately reflect the change.
There was a problem hiding this comment.
Popups no longer use this code. I'll add a ticket to refactor this member away.
There was a problem hiding this comment.
@highperformancecoder, thanks for the clarification — that makes sense. Would you like me to open a GitHub issue to track the refactoring of the popup member away from this code path?
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…rformancecoder/minsky into ture-194-show-values-as-intrinsic
…dleyIcon rather than system preference. For feature #194.
This change is
Summary by CodeRabbit
Chores
New Features
Bug Fixes