Addon's custom context menu entries get overridden by other addons #100423

New Issue

Luca Rood · 2022-08-15T21:42:17+02:00

Luca Rood commented

2022-08-15 21:42:17 +02:00

The ability to add custom entries to context menus was implemented in D2612. However, the current implementation is not reliable, as only the last addon to register custom menu entries will actually have the entries show up, leaving it purely up to chance.

The feature works by registering a Menu subclass named WM_MT_button_context, which is then called by Blender in source/blender/editors/interface/interface_context_menu.c to draw the additional menu items. The issue occurs when multiple addons register this class, thus overriding each other's implementation.

This issue was raised by @dr.sybren in the discussion of the original implementation, but didn't end up getting resolved.

The simple solution is for Blender to always register a dummy WM_MT_button_context class on startup, allowing addons to simply append to it, rather than having to register a whole class. Such a solution was implemented by @JulianEisel for UI_MT_list_item_context_menu (another similar class for extending context menus) in 87c1c8112f. In this instance, the dummy class gets registered in release/scripts/startup/bl_ui/__init__.py.

Unfortunately, this is now no longer a complete solution. If Blender now registers the class, and the documentation is updated, it still does not solve the issue of existing addons that use the existing registration routine, as they will still override the class.

My proposed solution is to mark the current class as deprecated, and make another class with a different name for this purpose (the old class can be kept around for a while alongside the new one, or support for it can be removed at once).

I'm creating this issue to see whether this proposal is deemed sound, in which case I can submit a patch for this change. I'd also like to query what a new such class should be called under current naming conventions.

An alternative might be to keep the current class name, get Blender to register it on startup, and implement a mechanism to block the re-registration of this particular class. But I don't know if RNA has such a mechanism already, so this solution could require deeper changes, and might therefore not be preferable.

Also note that even though addons can check if the class is already registered (as mentioned by @mont29), this does not solve the issue. Even if an addon does this check, it can still have its menu entries overridden by another addon that does not do the check (which is very plausible, given that the official documented example does not perform this check). Additionally, even in an ideal scenario where every addon does the check, there is still the issue of the class getting unregistered in the middle of a session, if the user disables the first addon to register the class. The only robust approach I can see is for Blender itself to register the class, as in @JulianEisel's implementation.

Also tagging @plasmasolutions and @ideasman42, who made the original implementation.

The ability to add custom entries to context menus was implemented in [D2612](https://archive.blender.org/developer/D2612). However, the current implementation is not reliable, as only the last addon to register custom menu entries will actually have the entries show up, leaving it purely up to chance. The feature works by registering a `Menu` subclass named `WM_MT_button_context`, which is then called by Blender in `source/blender/editors/interface/interface_context_menu.c` to draw the additional menu items. The issue occurs when multiple addons register this class, thus overriding each other's implementation. This issue was raised by @dr.sybren in the discussion of the original implementation, but didn't end up getting resolved. The simple solution is for Blender to always register a dummy `WM_MT_button_context` class on startup, allowing addons to simply append to it, rather than having to register a whole class. Such a solution was implemented by @JulianEisel for `UI_MT_list_item_context_menu` (another similar class for extending context menus) in 87c1c8112f. In this instance, the dummy class gets registered in `release/scripts/startup/bl_ui/__init__.py`. Unfortunately, this is now no longer a complete solution. If Blender now registers the class, and the documentation is updated, it still does not solve the issue of existing addons that use the existing registration routine, as they will still override the class. My proposed solution is to mark the current class as deprecated, and make another class with a different name for this purpose (the old class can be kept around for a while alongside the new one, or support for it can be removed at once). I'm creating this issue to see whether this proposal is deemed sound, in which case I can submit a patch for this change. I'd also like to query what a new such class should be called under current naming conventions. An alternative might be to keep the current class name, get Blender to register it on startup, and implement a mechanism to block the re-registration of this particular class. But I don't know if RNA has such a mechanism already, so this solution could require deeper changes, and might therefore not be preferable. Also note that even though addons can check if the class is already registered (as mentioned by @mont29), this does not solve the issue. Even if an addon does this check, it can still have its menu entries overridden by another addon that does not do the check (which is very plausible, given that the official documented example does not perform this check). Additionally, even in an ideal scenario where every addon does the check, there is still the issue of the class getting unregistered in the middle of a session, if the user disables the first addon to register the class. The only robust approach I can see is for Blender itself to register the class, as in @JulianEisel's implementation. Also tagging @plasmasolutions and @ideasman42, who made the original implementation.

Luca Rood commented

2022-08-15 21:42:17 +02:00

Added subscribers: @JulianEisel, @plasmasolutions, @mont29, @ideasman42, @dr.sybren, @LucaRood-3

Campbell Barton commented

2022-08-16 03:47:00 +02:00

This proposal for a new class seems reasonable, while it's possible to do tricks with registration that collect sub-classes of Menu and combine all sub-classes called WM_MT_button_context, I'd rather avoid overly clever/complicated code here as this will likely add fairly involved logic into the registration path code-path for all classes and even if that can be avoided, any bugs we encounter are likely to be quite involved to troubleshoot.

The new menu can use layout.menu_contents("WM_MT_button_context") to ensure it's content is also included and there is no reason we can't keep the old old menu around until 3.4x (for e.g.).

Note that re-reading D2612 I think it was a mistake to accept the patch, especially as Sybren noted this limitation.

This proposal for a new class seems reasonable, while it's possible to do tricks with registration that collect sub-classes of `Menu` and combine all sub-classes called `WM_MT_button_context`, I'd rather avoid overly clever/complicated code here as this will likely add fairly involved logic into the registration path code-path for all classes and even if that can be avoided, any bugs we encounter are likely to be quite involved to troubleshoot. The new menu can use `layout.menu_contents("WM_MT_button_context")` to ensure it's content is also included and there is no reason we can't keep the old old menu around until 3.4x (for e.g.). ---- Note that re-reading [D2612](https://archive.blender.org/developer/D2612) I think it was a mistake to accept the patch, especially as Sybren noted this limitation.

Luca Rood commented

2022-08-16 18:20:11 +02:00

Thank you for the quick response, Campbell. I agree that changes to the registration system are probably not warranted for such a specific case. I've implemented my proposed fix in D15702.

Thank you for the tip about menu_contents, I actually didn't know about that API. Using this method to draw the legacy menu did have a small issue though, so check that out in the patch to see if my fix is acceptable, or if it should be handled differently.

Indeed it was somewhat surprising that the original implementation was accepted without addressing this, but luckily it doesn't seem like the feature was used much, as far as I can tell.
In fact, that's probably in part due to it not really being advertised much. I bet many addon developers don't even realise this feature exists. Perhaps the availability of this API could be highlighted more somehow, as I can imagine it being of use for many addons!

Thank you for the quick response, Campbell. I agree that changes to the registration system are probably not warranted for such a specific case. I've implemented my proposed fix in [D15702](https://archive.blender.org/developer/D15702). Thank you for the tip about `menu_contents`, I actually didn't know about that API. Using this method to draw the legacy menu did have a small issue though, so check that out in the patch to see if my fix is acceptable, or if it should be handled differently. Indeed it was somewhat surprising that the original implementation was accepted without addressing this, but luckily it doesn't seem like the feature was used much, as far as I can tell. In fact, that's probably in part due to it not really being advertised much. I bet many addon developers don't even realise this feature exists. Perhaps the availability of this API could be highlighted more somehow, as I can imagine it being of use for many addons!

Philipp Oeser commented

2022-08-17 10:36:32 +02:00

Added subscriber: @lichtwerk