SketchUp / api-issue-tracker

Public issue tracker for the SketchUp and LayOut's APIs
https://developer.sketchup.com/
38 stars 10 forks source link

Menu#add_submenu failure #777

Open Fredosixx opened 2 years ago

Fredosixx commented 2 years ago

Putting traces in my code, I found occurrences where Menu#add_submenu fails (i.e. return nil), although the argument are valid.

The traces shows that @su_menu.add_submenu(@menu_name) returns nil with

@menu_name = TopoShaper
@su_menu = #Sketchup::Menu:0x000001f46ca9a580

This happens rarely, a few users, and randomly, that is, not necessarily TopoShaper. Furthermore, sers getting the problem may have it working fine when reinstalling the plugins or Sketchup.

Could someone in the Trimble Extensibility team have a look at the Menu#add_submenu method and check under which conditions the method could return nil.

thomthom commented 2 years ago

Is this happening only on Windows by any chance?

And is it always in scenarios where you have an instance variable? Are you sharing the menu reference cross extensions?

Fredosixx commented 2 years ago

On Windows only.

And yes, the menu object is in an instance variable @su_menu.

But this seems to happen randomly.

thomthom commented 2 years ago

I've seem something similar when creating menus from the Ruby Console:

# Enter both lines at the same time, works.
menu  = UI.menu('Extensions')
menu.add_item('Hello'} { puts 'World' }

But separately:

menu  = UI.menu('Extensions')
# This would now fail.
menu.add_item('Hello'} { puts 'World' }

Are you sharing the menu references across multiple extensions?

Maybe... if we see the same scenario as in the Ruby Console if; an extension is installed? (The menu ref might no longer work until you restart?) Or if, something delays some code execution to take it out of the startup loop, a timer for instance...?

Fredosixx commented 2 years ago

It looks like it is the issue with the persistence of menu handles in Windows, already reported.

All I try to achieve is to group the menu entries of all my plugins under Tools / Fredo6 Collection. Normally it works, but apparently there may be some cases now where it does not due to this issue of menu handle persistence.

I would suggest that the menu handling is reviewed to provide a working API to do this grouping by author that many users prefer, in order not to have the Extensions menus and other SketchUp top menus crowded with plugin menu entries when they have many installed.

We need a solid framework, and also a way to test whether a menu has already been created. Today there is no method, since UI.menu() only works for the Sketchup root menus. Currently, if you call UI.menu('foobar'), it returns a menu handle, not nil. We also need to test and get the handle to submenu, with a notation like "Tools/Fredo6 Collection"

thomthom commented 2 years ago

We are in the process of replacing the tech for the UI. So lots of UI/UX related items on our plate coming up. I've tagged this issue ui so we easily find it when revewing out UI related issues.

DanRathbun commented 2 years ago

We also need to test and get the handle to submenu, with a notation like "Tools/Fredo6 Collection"

Ie .. we've brought this up in the past (about using slash ['/'] characters) that are already used on Mac in the menus. It already causes issues with confused shortcuts, etc. There is likely no check to prevent slashes in menu name (submenus or items) so trying to use menu path strings may be problematic going forward. (Unless some new rule is implemented and embedded slashes are replaced with some other character by the API methods that create submenus, menu items and UI::Command menu text.)

It would be easier and more robust as:

tools = UI.menu('Tools')
submenu = tools.get_submenu('Fredo6 Collection')
submenu = tools.add_submenu('Fredo6 Collection') unless submenu

... or ...

submenu = UI.menu('Tools').get_submenu('Fredo6 Collection', create: true)

The :create option should default to false.

Also a boolean query method could be useful ...

UI.menu('Tools').has_submenu?('Fredo6 Collection')

Rounding out the features, a method to test whether an item or command has been added ...

if UI.menu('Tools')&.get_submenu('Fredo6 Collection')&.has_item?('Round Corner')

I'm not sure if a #[] item/command getter is "safe". Malicious code could change another extension's command properties

ADD: Also it would be nice to test if the menu currently has a separator at the bottom so as not to create double separators.

Currently, if you call UI.menu('foobar'), it returns a menu handle, not nil.

I agree, this should return nil.

Also, the same Ruby object (and ID) should be retrieved each time a menu handle is requested by the API method call(s). (Currently we get a different object each time on Windows.)


REF:

https://docs.microsoft.com/en-us/windows/win32/menurc/menus

DanRathbun commented 2 years ago

Oh, I thought of another method and that is a test what type of object is at a particular index in a menu (ie, submenu, separator or menu command.)

if UI.menu('Tools').type_at(12) == MENU_SUBMENU

Could also use MENU_COMMAND or MENU_SEPARATOR.

A negative index counts from the bottom, ie -1 would be the last member in the menu's list.

sketchup[bot] commented 1 year ago

Logged as: SKEXT-3903