Skip to content

Add "Import stock variables" context menu item for Godley icons #575

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
highperformancecoder merged 8 commits into from
Oct 27, 2025
Merged

Add "Import stock variables" context menu item for Godley icons #575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
16dfc5c
Initial plan
Copilot Oct 27, 2025
68fa3ae
Add Import stock variables context menu item for Godley icons
Copilot Oct 27, 2025
68f8b1b
Remove redundant importEnabled check in click handler
Copilot Oct 27, 2025
666fe8a
Restore defensive check in click handler for clarity
Copilot Oct 27, 2025
3fe0c9a
Fix up Copilot's code.
highperformancecoder Oct 27, 2025
d669455
Update ContextMenuManager.ts
highperformancecoder Oct 27, 2025
adf7d8a
Remove dead code
highperformancecoder Oct 27, 2025
8fb0036
Merge branch 'copilot/add-import-stock-variables-menu-item' of github…
highperformancecoder Oct 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions gui-js/apps/minsky-electron/src/app/managers/ContextMenuManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,22 @@ export class ContextMenuManager {
const rowColButtonsChecked = await godley.buttonDisplay();
const editorModeChecked = await godley.editorMode();

// build stock import menu for this column

const stockImportMenuItems=[];
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space after '=' operator. Should be 'const stockImportMenuItems = [];' to follow consistent spacing conventions.

Suggested change
const stockImportMenuItems=[];
const stockImportMenuItems = [];

Copilot uses AI. Check for mistakes.
{
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
{
{
// 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.

Copilot uses AI. Check for mistakes.
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);
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing spaces around '=' operators. Should be 'let x = ...', 'let col = ...', and 'let importOptions = ...' for consistent code formatting.

Suggested change
let importOptions=await godley.editor.matchingTableColumnsByCol(col);
let importOptions = await godley.editor.matchingTableColumnsByCol(col);

Copilot uses AI. Check for mistakes.
for (let v of importOptions)
stockImportMenuItems.push({
label: v,
click: (item) => {
godley.editor.importStockVarByCol(item.label, col);
},
});
}
Comment on lines +654 to +668
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Copy link
Owner

@highperformancecoder highperformancecoder Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


const menuItems = [
new MenuItem({
label: 'Open Godley Table',
Expand Down Expand Up @@ -696,6 +712,11 @@ export class ContextMenuManager {
label: 'Copy stock variables',
click: async () => {minsky.canvas.copyAllStockVars();}
}),
new MenuItem({
label: 'Import stock variables',
enabled: stockImportMenuItems.length>0,
submenu: Menu.buildFromTemplate(stockImportMenuItems),
}),
new MenuItem({
label: 'Export as',
submenu: [
Expand Down
16 changes: 1 addition & 15 deletions model/godleyTableWindow.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,13 +203,6 @@ namespace minsky
colWidgets[col].draw(cairo);
}

if (col>1)
{
cairo_move_to(cairo,x-pulldownHot,topTableOffset);
pango.setMarkup("▼");
pango.show();
}

double y=topTableOffset;
double colWidth=minColumnWidth;
for (unsigned row=0; row<m_godleyIcon.table.rows(); ++row)
Expand Down Expand Up @@ -284,8 +277,7 @@ namespace minsky
}
pango.setMarkup(text);
}
// allow extra space for the ▼ in row 0
colWidth=max(colWidth,pango.width() + (row==0? pulldownHot:0));
colWidth=max(colWidth,pango.width());
cairo_move_to(cairo,x+3,y);
pango.show();
y+=rowHeight;
Expand All @@ -301,12 +293,6 @@ namespace minsky
x=colLeftMargin[col+1];
}

// display pulldown for last column
cairo_move_to(cairo,x-pulldownHot,topTableOffset);
pango.setMarkup("▼");
pango.show();


pango.setMarkup
(capitalise(enumKey<GodleyAssetClass::AssetClass>(assetClass)));
// increase column by enough to fit asset class label
Expand Down