Skip to content

Commit c4de618

Browse files
committed
MenuService: do not initialize menus so eagerly
When a ModulesAddedEvent, ModulesRemovedEvent or ModulesUpdatedEvent is published, we only care if we have already initialized the menu structure, and that structure would thus need to be updated. In the case where no menu structure has yet been generated, we can ignore the module events. We just need to be careful if module events come in while we are generating the menu structure. If that happens, we block (thanks to the event handling methods being synchronized now) until menus are created, and then update them in response to the event. Among other benefits, this commit fixes a bug in ImageJ on macOS whereby scripts in legacy directories (i.e.: in plugins and plugins/Scripts) were no longer appearing in the menu structure. It was happening because the following order of events occurred: 1. The DefaultPlatformService initialized the MacOSPlatform. 2. The MacOSPlatform edited app commands (About, Quit and Preferences) to remove their menu paths, since they are specially handled. 3. The MacOSPlatform fired a ModulesUpdatedEvent to notify others of it. 4. The DefaultMenuService subscribed to the ModulesUpdatedEvent; in response, its event handler method eagerly initialized the menus. 5. The menu initialization code looped over ModuleService.getModules(). 6. The call to getModules() triggered the deferred addition of modules. 7. The deferred addition of modules triggered the DefaultScriptService to discover scripts in the currently registered script directories. All of the above happened before the LegacyService initialized, and therefore the DefaultScriptService did not yet know about the legacy script directories to include in its script discovery process. Once the scripts have been discovered, adding the additional script directories has no effect (i.e., no additional script discovery is triggered). This commit addresses step (4), changing the DefaultMenuService to do nothing when modules change in the case of menus still uninitialized. After this change, the order of events now becomes: 1. The DefaultPlatformService initializes the MacOSPlatform. 2. The MacOSPlatform edits the app command menu paths. 3. The MacOSPlatform fires a ModulesUpdatedEvent to notify others. 4. The DefaultMenuService still receives the ModulesUpdatedEvent, but does nothing in response. 5. Later, the LegacyService loops over ModuleService.getModules(). 6. The call to getModules() triggers the deferred addition of modules. 7. The deferred addition of modules triggers the DefaultScriptService to discover scripts in the currently registered script directories, which include the legacy directories added by the LegacyService. So the problem is solved, at least for now. There are still questions and potential issues with the design: - Why does the DefaultPlatformService (at priority normal) initialize before the LegacyService (at priority normal+1)? - What if some other service triggers script discovery before the additional script directories can be registered? There are many code paths and service calls, such as MenuService and ModuleService methods, that would do so. Should we make the ScriptService smarter such that scripts are rescanned when additional script directories are registered after initial script discovery has taken place? Doing so would be more robust, but substantially complicate things.
1 parent e7d8bd3 commit c4de618

File tree

1 file changed

+6
-9
lines changed

1 file changed

+6
-9
lines changed

src/main/java/org/scijava/menu/DefaultMenuService.java

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -79,25 +79,22 @@ public ShadowMenu getMenu(final String menuRoot) {
7979
// -- Event handlers --
8080

8181
@EventHandler
82-
protected void onEvent(final ModulesAddedEvent event) {
83-
if (rootMenus == null) {
84-
// add *all* known modules, which includes the ones given here
85-
rootMenus();
86-
return;
87-
}
88-
// data structure already exists; add *these* modules only
82+
protected synchronized void onEvent(final ModulesAddedEvent event) {
83+
if (rootMenus == null) return; // menus not yet initialized
8984
addModules(event.getItems());
9085
}
9186

9287
@EventHandler
93-
protected void onEvent(final ModulesRemovedEvent event) {
88+
protected synchronized void onEvent(final ModulesRemovedEvent event) {
89+
if (rootMenus == null) return; // menus not yet initialized
9490
for (final ShadowMenu menu : rootMenus().values()) {
9591
menu.removeAll(event.getItems());
9692
}
9793
}
9894

9995
@EventHandler
100-
protected void onEvent(final ModulesUpdatedEvent event) {
96+
protected synchronized void onEvent(final ModulesUpdatedEvent event) {
97+
if (rootMenus == null) return; // menus not yet initialized
10198
for (final ShadowMenu menu : rootMenus().values()) {
10299
menu.updateAll(event.getItems());
103100
}

0 commit comments

Comments
 (0)