Handle Plugins*Event in AbstractSingletonService #271
Conversation
For most SingletonServices the instance field was only set during initialization making them inflexible during runtime. Registering to PluginAddedEvents and PluginRemovedEvents allows for more dynamic behavior, e.g. adding Converters in a SJJK-backed notebook.
| @SuppressWarnings("unchecked") | ||
| final Class<? extends PT> ptClass = (Class<? extends PT>) instance | ||
| .getClass(); | ||
| instanceMap.putIfAbsent(ptClass, instance); |
There was a problem hiding this comment.
If someone tries to add a plugin which already exists, should we overwrite it? Naively, I would vote yes. (Though I suppose you could remove it and readd it, otherwise—but then there are two separate events.) And actually it might cause problems/skew if the PluginIndex was overwritten with a new version of a class, but the SingletonServices ignored that new version and kept the old one, no?
| * {@link PluginsAddedEvent}s originating from the {@link PluginService}. | ||
| */ | ||
| @Test | ||
| public void testSingletonServicePluginsAddedHandling() { |
| @SuppressWarnings("rawtypes") | ||
| PluginInfo<Converter> converterInfo = new PluginInfo<>( | ||
| DummyStringConverter.class, Converter.class); | ||
| converterInfo.setPluginClass(DummyStringConverter.class); |
There was a problem hiding this comment.
Is it actually necessary to call setPluginClass here? The constructor should do it?
| @SuppressWarnings("rawtypes") | ||
| PluginInfo<Converter> converterInfo = new PluginInfo<>( | ||
| DummyStringConverter.class, Converter.class); | ||
| converterInfo.setPluginClass(DummyStringConverter.class); |
There was a problem hiding this comment.
Same here: is this call necessary?
| pluginService.addPlugin(converterInfo); | ||
|
|
||
| // De-register DummyStringConverter | ||
| pluginService.removePlugin(converterInfo); |
There was a problem hiding this comment.
Instead of adding then immediately removing, a cleaner approach might be to simply remove one of the built-in discovered plugins. If you don't like that external dependency for some reason (and I wouldn't blame you), you could create a second static inner class here annotated with @Plugin so that it is automatically discovered and added initially, and then remove it here. The advantage is that we ensure it works to remove plugins where were present when the context was first initialized. I think this is a more robust test—or at least a needed additional test.
| pluginService.removePlugin(converterInfo); | ||
|
|
||
| assertNull(pluginService.getPlugin(DummyStringConverter.class)); | ||
| assertFalse(convertService.supports("StringInput", Number.class)); |
There was a problem hiding this comment.
Wait... doesn't the convert service support conversion of string to number? Or only if you give an explicit Number subtype otherwise?
I would prefer, instead of String and Number, that you use two new interfaces defined here. That way we can be certain that future converters relating to basic types like strings and numbers do not inadvertently cause this test to fail in the future.
|
Thank you very much, @stelfrich, this is looking really good. I'm glad it turned out to be straightforward and clean to implement. I made some comments—could you please address them if you have time? Should be relatively minor and quick I hope. |
I have addressed your comments @ctrueden. |
|
Meanwhile, @gab1one did this work again in ecf6cbb. 😓 @gab1one: will you please take a look at this PR and see if it accomplished anything that your changes did not? @stelfrich I am very sorry that I not only dropped the ball on merging this, but then forgot it even existed. I am now making it a priority to go through the substantial backlog of very worthy PRs across the various SciJava and ImageJ components. |
Don't worry! I know how hard it is to keep track of all the PRs...
Just go ahead and close this if there's nothing of additional value here (which is likely).. |
|
I have extracted the tests to a new PR #338, everything else is pretty much identical. |
SingletonServiceTest: extend with tests from #271.
3 participants
For most
SingletonServicestheinstancesandinstanceMapfields were only set during initialization making them inflexible during runtime. Registering toPluginAddedEventsandPluginRemovedEventsallows for more dynamic behavior, e.g. addingConverters in notebooks./cc @ctrueden