Check that a MENU has ITEMS that is LISTP before trying to display/use it.

[MattHeffron]

This resolves #2306

Checking added to FNS: ADDMENU, CHECK/MENU/IMAGE, UPDATE/MENU/IMAGE, and MENU.

[rmkaplan]

I think the menu arguments in the calls to ERROR shouldn't be quoted

[nbriggs]

Even prior to this change, the way it presents a problem with the ITEMS is:

and it's a "TYPE-MISMATCH" exception that is being reported, not an "INTERLISP-ERROR" that a call to ERROR will give you.

[pamoroso]

I successfully built this PR on Linux Mint 22.1 Cinnamon and the new MENU behavior seems to work with no issues. When the menu is not supplied or there are no items I now get warnings instead of break windows with division by 0 errors.

[pamoroso]

I successfully built this PR on Linux Mint 22.1 Cinnamon and the new MENU behavior seems to work with no issues. When the menu is not supplied or there are no items I now get warnings instead of break windows with division by 0 errors.

[masinter]

It is a matter of taste, I suppose, but I would hate to see this style of error checking pervade the sources. We can't afford to add this kind of debugging tool support to prevent and diagnose errors that are rare and easily diagnosed when they do occur.

[masinter]

lets discuss ...

[rmkaplan]

In this case we are talking about a user interface feature, with a generic divide-by-zero that is happening somewhere far down. I think it is appropriate to give a more useful diagnosis.

…

On Oct 3, 2025, at 8:26 AM, Larry Masinter ***@***.***> wrote: @masinter requested changes on this pull request. It is a matter of taste, I suppose, but I would hate to see this style of error checking pervade the sources. We can't afford to add this kind of debugging tool support to prevent and diagnose errors that are rare and easily diagnosed when they do occur. — Reply to this email directly, view it on GitHub <#2309 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQSTUJPVRJC5NUDK6NWW6O33V2IR7AVCNFSM6AAAAACIFDJYS2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTEOJZGU4TQNBRGM>. You are receiving this because your review was requested.

[nbriggs]

I agree with Larry on this one. The UI code could have avoided creating an empty menu.

[rmkaplan]

A point earlier in this discussion is that it should be OK to *create* a menu with no items, because later code can fill in the items. But, what should happen when an attempt is made to display such a menu before any items have been added. It is an invalid menu with respect to display, and divide by 0 is not a useful description of what went wrong. An alternative approach would be to avoid the error, and instead put up a menu with a single dummy item ("No items").

…

On Oct 3, 2025, at 10:16 AM, Nick Briggs ***@***.***> wrote: nbriggs left a comment (Interlisp/medley#2309) <#2309 (comment)> I agree with Larry on this one. The UI code could have avoided creating an empty menu. — Reply to this email directly, view it on GitHub <#2309 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQSTUJO4EBENFRXMQZ2E3YL3V2VNHAVCNFSM6AAAAACIFDJYS2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTGNRWGUZTCNBVGI>. You are receiving this because your review was requested.

[MattHeffron]

An alternative approach would be to avoid the error, and instead put up a menu with a single dummy item ("No items").

My initial attempt, which I replaced before pushing it to GitHub, was to initialize the ITEMS field (in the record definition) with (LIST "*** Empty MENU!! ***"). I mentioned this in PR #2307.

This would break any code that built the list incrementally, and that assumed that ITEMS could be empty, and if so, created the LIST for the first item added, and used, e.g. NCONC1 to add to the existing list.
If the list was never really empty, then the "*** Empty MENU!! ***" entry would always be there.

The question is, when should the check for the empty ITEMS be made?
Since MENU, CHECK/MENU/IMAGE, and ADDMENU check the type of the MENU / ADDEDMENU argument at the beginning of the function body, it doesn't seem unreasonable to check there for conditions that will cause failure during/beneath these functions.

To modify the MENU ITEMS, at the time of the call, to note that it is empty, should also restore it after the call. This would be especially problematic with ADDMENU which leaves the menu visible beyond the temporal scope of the call.

[nbriggs]

I think we should not change it. It has worked this way since the MENU code was written. We're not seeing lots of undebuggable code as a result of this, and it should be the responsibility of the code that's constructing the menu to ensure that the items are properly set up before having the menu displayed. I think if we get to the point of being able to properly update the IRM (and thus DInfo) we could note that it is an error to cause a menu with no items to be displayed.

[masinter]

not sure what the problem was, the changes here look fine today.