Add "Import stock variables" context menu item for Godley icons #575
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughTwo distinct enhancements across the application stack: the UI layer now provides a dynamic stock variable import submenu in Godley table context menus based on column calculations, while the rendering layer disables pulldown indicator symbols and associated width accounting in table headers. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as ContextMenuManager
participant Calc as Column Calculator
participant Menu as Menu Builder
participant Handler as Stock Import Handler
User->>UI: Right-click on Godley table
UI->>Calc: Compute columns from node position & width
Calc->>Calc: Filter matching columns
Calc-->>UI: Return column list
UI->>Menu: Build submenu items
Note over Menu: Create item per column<br/>if list non-empty
Menu-->>UI: Stock import submenu
UI->>UI: Enable submenu if items exist
UI-->>User: Display context menu
User->>Handler: Click submenu item
Handler->>Handler: importStockVarByCol(columnIndex)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
- Added import for GodleyMenuManager - Detect if mouse is over stock column header (row0) using godley.popup.clickTypeZoomed - Get column index using godley.popup.colXZoomed - Added "Import stock variables" menu item enabled only when over row0 - Click handler calls GodleyMenuManager.importStock with godley.popup and column - Added defensive error logging for column detection Reference: https://sourceforge.net/p/minsky/tickets/1890/ Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
The menu item is already disabled when importEnabled is false, so checking it again in the click handler is redundant. Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
Keep both importEnabled and importCol checks for logical consistency and defensive programming, even though disabled menu items shouldn't be clickable. Co-authored-by: highperformancecoder <3075825+highperformancecoder@users.noreply.github.com>
|
@CodeRabbit review |
|
@copilot review |
✅ Actions performedReview triggered.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
model/godleyTableWindow.cc (2)
288-288: Commented code: remove or restore the pulldownHot width accounting.The commented portion suggests the pulldown indicator width is no longer factored into column width calculations. If the indicator is permanently removed, delete the commented code. If it may be restored, document why it's temporarily disabled.
Apply this diff to clean up:
- colWidth=max(colWidth,pango.width() /* + (row==0? pulldownHot:0)*/); + colWidth=max(colWidth,pango.width());
305-307: Remove commented code for the last column's pulldown rendering.If the pulldown indicator is permanently removed, delete these commented lines to keep the codebase clean.
Apply this diff:
-// cairo_move_to(cairo,x-pulldownHot,topTableOffset); -// pango.setMarkup("▼"); -// pango.show();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts(3 hunks)model/godleyTableWindow.cc(3 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts
[notice] 9-9: Unused variable, import, function or class
Unused import GodleyMenuManager.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: copilot
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
🔇 Additional comments (2)
gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts (1)
716-720: LGTM with minor suggestion.The "Import stock variables" menu item is correctly positioned between "Copy stock variables" and "Export as" as specified in the PR objectives. However, the enabled state depends only on
stockImportMenuItems.length>0. Once the row0 check is added (see previous comment), this will work correctly.model/godleyTableWindow.cc (1)
206-211: Remove commented code and clean up line 326; verify colWidgets provides adequate visual feedback for column import feature.The pulldown indicator (▼) rendering was removed, but this appears to be part of a refactoring where column header interactions now use
colWidgets(ButtonWidget objects) instead. These button widgets are actively drawn (line 203) and handle column interactions. However, line 326 contains orphaned cursor positioning code (cairo_move_to(cairo,x-pulldownHot,topTableOffset);) with no rendering—this should be removed as part of the cleanup. The import feature itself remains functional throughimportStockVar()andimportStockVarByCol()functions. Verify that the colWidgets button UI provides sufficient visual indication to users about where to interact for importing stock variables.
| import { BrowserWindow, Menu, MenuItem, IpcMainEvent } from 'electron'; | ||
| import { BookmarkManager } from './BookmarkManager'; | ||
| import { CommandsManager } from './CommandsManager'; | ||
| import { GodleyMenuManager } from './GodleyMenuManager'; |
There was a problem hiding this comment.
Remove unused import.
GodleyMenuManager is imported but never used. The PR description mentions using GodleyMenuManager.importStock(), but the implementation directly calls godley.editor.importStockVarByCol().
Apply this diff if GodleyMenuManager won't be used:
-import { GodleyMenuManager } from './GodleyMenuManager';Alternatively, if the PR intended to use GodleyMenuManager.importStock(), update the click handlers at lines 666 and 1269 accordingly.
Based on learnings
📝 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.
| import { GodleyMenuManager } from './GodleyMenuManager'; |
🧰 Tools
🪛 GitHub Check: CodeQL
[notice] 9-9: Unused variable, import, function or class
Unused import GodleyMenuManager.
🤖 Prompt for AI Agents
In gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts around
line 9, the GodleyMenuManager import is unused; remove the import line to clean
up unused code (or if you intended to use GodleyMenuManager, replace the direct
calls to godley.editor.importStockVarByCol() at the click handlers around lines
666 and 1269 with calls to GodleyMenuManager.importStock(...) and adjust
parameters accordingly so the manager is actually used).
| // build stock import menu for this column | ||
|
|
||
| const stockImportMenuItems=[]; | ||
| { | ||
| let x=this.x-await godley.x()+0.5*await godley.width(); | ||
| let col=await godley.editor.colXZoomed(x); | ||
| let importOptions=await godley.editor.matchingTableColumnsByCol(col); | ||
| for (let v of importOptions) | ||
| stockImportMenuItems.push({ | ||
| label: v, | ||
| click: (item) => { | ||
| godley.editor.importStockVarByCol(item.label, col); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing row0 check: menu should only be enabled when clicking on stock column headers.
According to the PR objectives, the "Import stock variables" menu should be "enabled only when right-clicking over a Godley icon's stock column header." The current code builds the import menu for any click location on the GodleyIcon without verifying the user clicked on row0 (the stock variable header row).
Consider checking the click type before building the menu:
const editorModeChecked = await godley.editorMode();
- // build stock import menu for this column
-
const stockImportMenuItems=[];
- {
+ // Only build import menu if clicking on row0 (stock column header)
+ const clickType = await godley.editor.clickTypeZoomed(this.x - await godley.x() + 0.5 * await godley.width(),
+ this.y - await godley.y() + 0.5 * await godley.height());
+ if (clickType === "row0") {
let x=this.x-await godley.x()+0.5*await godley.width();
let col=await godley.editor.colXZoomed(x);
let importOptions=await godley.editor.matchingTableColumnsByCol(col);
for (let v of importOptions)
stockImportMenuItems.push({
label: v,
click: (item) => {
godley.editor.importStockVarByCol(item.label, col);
},
});
}This ensures the menu is context-sensitive as intended.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
In practical testing, so long as the import menu item was restricted to columns rather than just row 0, it was fine, since it is a context menu, rather than a right mouse click action.
There was a problem hiding this comment.
@highperformancecoder, thank you for the clarification and for testing this! I understand now—since this is a context menu for the entire Godley icon (not a row0-specific right-click), restricting the import functionality by column is sufficient and provides the appropriate context-sensitive behavior. Your practical testing confirms this works well.
I appreciate you explaining the distinction between a context menu approach and a right-click action. This makes sense given the overall UI design.
✏️ Learnings added
Learnt from: highperformancecoder
PR: highperformancecoder/minsky#575
File: gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts:654-668
Timestamp: 2025-10-27T05:57:05.870Z
Learning: For Godley icon context menus in `gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts`, the "Import stock variables" menu item is restricted by column (using the clicked x-coordinate to determine the column) rather than being limited to row0 clicks only. This column-based restriction is sufficient for the context menu approach, as confirmed by practical testing.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
The implementation looks good! The coordinate transformation and direct submenu building are improvements over my initial approach. A few observations: Positives:
Suggestions for robustness:
The core functionality is solid and the build passes successfully. |
|
@copilot - the calls to colX etc are guaranteed not to throw, which is why I removed the try/catch statements. |
….com:highperformancecoder/minsky into copilot/add-import-stock-variables-menu-item
highperformancecoder
deleted the
copilot/add-import-stock-variables-menu-item
branch
There was a problem hiding this comment.
Pull Request Overview
Adds a context menu item "Import stock variables" to Godley icons, enabling users to import matching stock variable columns from other Godley tables. The implementation builds a dynamic submenu based on available import options for the column at the click position.
- Added "Import stock variables" menu item with dynamic submenu to Godley icon context menu
- Implemented coordinate transformation to map canvas coordinates to Godley table columns
- Removed pulldown indicator symbols (▼) from Godley table headers in the C++ rendering code
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts | Adds "Import stock variables" menu item with coordinate-to-column mapping and dynamic submenu generation |
| model/godleyTableWindow.cc | Removes pulldown indicator symbols from Godley table header rendering |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // build stock import menu for this column | ||
|
|
||
| const stockImportMenuItems=[]; | ||
| { |
There was a problem hiding this comment.
The coordinate transformation lacks explanation. Add a comment describing why 0.5*width is added (presumably to adjust from icon center to left edge) to clarify the mathematical relationship between canvas and Godley-local coordinates.
| { | |
| { | |
| // Convert from canvas coordinates (where this.x is relative to the icon center) | |
| // to Godley-local coordinates (where 0 is the left edge of the icon). | |
| // Adding 0.5*width shifts the origin from the center to the left edge. |
|
|
||
| // build stock import menu for this column | ||
|
|
||
| const stockImportMenuItems=[]; |
There was a problem hiding this comment.
Missing space after '=' operator. Should be 'const stockImportMenuItems = [];' to follow consistent spacing conventions.
| const stockImportMenuItems=[]; | |
| const stockImportMenuItems = []; |
| { | ||
| let x=this.x-await godley.x()+0.5*await godley.width(); | ||
| let col=await godley.editor.colXZoomed(x); | ||
| let importOptions=await godley.editor.matchingTableColumnsByCol(col); |
There was a problem hiding this comment.
Missing spaces around '=' operators. Should be 'let x = ...', 'let col = ...', and 'let importOptions = ...' for consistent code formatting.
| let importOptions=await godley.editor.matchingTableColumnsByCol(col); | |
| let importOptions = await godley.editor.matchingTableColumnsByCol(col); |
Adds a context-sensitive menu item to import stock variables when right-clicking on a Godley icon, enabling users to quickly import matching columns from other Godley tables.
Changes
godley.editor.matchingTableColumnsByCol()with items that callgodley.editor.importStockVarByCol()Implementation
Notes
Uses
godley.editor(GodleyTableEditor) for coordinate calculations and import operations. The coordinate transformation accounts for the Godley icon's position and width to correctly map canvas coordinates to table columns.Reference: https://sourceforge.net/p/minsky/tickets/1890/
Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
This change is
Summary by CodeRabbit
New Features
Style