Skip to content
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.

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

Menu#add_submenu failure #777

Open
Fredosixx opened this issue Mar 15, 2022 · 8 comments
Open

Menu#add_submenu failure #777

Fredosixx opened this issue Mar 15, 2022 · 8 comments

Comments

@Fredosixx
Copy link

Fredosixx commented Mar 15, 2022

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
Copy link
Member

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
Copy link
Author

On Windows only.

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

But this seems to happen randomly.

@thomthom
Copy link
Member

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
Copy link
Author

Fredosixx commented Mar 16, 2022

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
Copy link
Member

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
Copy link

DanRathbun commented Mar 17, 2022

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
Copy link

DanRathbun commented Mar 17, 2022

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
Copy link

sketchup bot commented Nov 20, 2023

Logged as: SKEXT-3903

@sketchup sketchup bot added the logged label Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants