imagej / imagej-legacy

ImageJ+ImageJ2 compatibility layer
https://imagej.net/libs/imagej-legacy
BSD 2-Clause "Simplified" License
17 stars 25 forks source link

Remove SciJava Script and Class search actions #283

Closed gselzer closed 1 year ago

gselzer commented 2 years ago

The Source search action is super cool. It's so cool that I wanna move it upstream.

The interesting thing, though, is that it is really 3 actions in 1. Each action has the same result, but operates on different types.

My remaining question: What does the remaining functionality in SourceSearchActionFactory get us? The only difference between this ActionFactory and the work added to scijava/scijava-search#24 is that this ActionFactory assumes that the ModuleInfo is a CommandInfo and loads the class from the Presets of that CommandInfo via IJ1Helper.getClassLoader.

Do we really need to do all of that? I'll have to understand it better...

ctrueden commented 1 year ago

@gselzer wrote:

it is really 3 actions in 1. Each action has the same result, but operates on different types. ... What does the remaining functionality in SourceSearchActionFactory get us?

The three actions you're talking about are:

  1. SciJava scripts:

    if (info instanceof ScriptInfo) sourceForScript((ScriptInfo) info);
  2. Legacy (original ImageJ) commands:

    if (id.startsWith("legacy:")) sourceForLegacyCommand(info);
  3. Other modules, notably SciJava/ImageJ2 Command plugins:

    sourceForClass(info, info.loadDelegateClass());

So by "remaining functionality" you mean (3), right? What it gets us is a Source button for SciJava commands, like everything in imagej-plugins-commands for example.

Do I understand your question correctly?

gselzer commented 1 year ago

Do I understand your question correctly?

@ctrueden hard to say as I am having trouble recalling, but I think by "remaining functionality" I was describing (2), as (1) and (3) encapsulated the functionality I moved out.

I think my point was just to make sure that we actually need (2) here, if we have (3) already. But again, I don't really remember since this was months ago.