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

Fix Logger Parameter with Ops #319

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maarzt
Copy link
Contributor

@maarzt maarzt commented Apr 27, 2018

Previously the LoggerPreprocessor automatically injected Logger Paramters for CommandService.
But LoggerPreprocessor didn't work for Ops.
This PR fixes this issue:

  1. Context injects Logger Parameter
  2. Logger extend Optional interface, this make Logger optional during ops matching.

The issue is described in http://forum.imagej.net/t/using-the-new-scijava-logging-and-status-service-system-in-an-application/9964/10

maarzt added 3 commits April 27, 2018 16:00
LoggerPreprecessor is no longer needed, because Context does the
Logger injection. But it's still important to test, if the Logger
gets confortable injected when using CommandService.
@ctrueden
Copy link
Member

I think this will wreak havoc with the ops matcher, unfortunately. Even though the Logger is optional, it is still a parameter, unlike LogService which is ignored by the ops matching framework. The Logger should be completely invisible to ops, and never appear in any op signature.

@ctrueden
Copy link
Member

Also, we do still need the LoggerPreprocessor, for scripts, since a script could have a Logger parameter, and the context injection will not handle it. This is the same reason we have the ServicePreprocessor still.

@maarzt
Copy link
Contributor Author

maarzt commented Apr 27, 2018

Even though the Logger is optional, it is still a parameter, unlike LogService which is ignored by the ops matching framework. The Logger should be completely invisible to ops, and never appear in any op signature.

I have a different opinion, I want the Logger to be an optional parameter. Let's assume: The caller of an op has a sophisticated UI with an integrated logging panel. The logging panel should show the log messages of the op. This can be achieved by providing a prepared Logger as parameter to the op.

Also, we do still need the LoggerPreprocessor, for scripts, since a script could have a Logger parameter, and the context injection will not handle it. This is the same reason we have the ServicePreprocessor still.
Merge state

Ok, I will restore the LoggerPreprocessor

@ctrueden
Copy link
Member

I want the Logger to be an optional parameter.

OK. There will be consequences though, is all I am saying. I assume you want existing ops that use LogService parameter to instead use Logger:

  • src/main/java/net/imagej/ops/commands/filter/FrangiVesselness.java
  • src/main/java/net/imagej/ops/help/HelpCandidates.java
  • src/main/java/net/imagej/ops/image/equation/DefaultEquation.java
  • src/main/java/net/imagej/ops/thread/chunker/ChunkerInterleaved.java
  • src/main/java/net/imagej/ops/thread/chunker/DefaultChunker.java
  • src/main/java/net/imagej/ops/threshold/rosin/ComputeRosinThreshold.java

These ops will all need their namespace methods updated to include the Logger. All existing code that relied on a certain number and order of parameters for them will break, because there is now a new Logger parameter, which must either be passed, or set to null. (Only if there are no other optional parameters "to the right" will existing code still work as expected, with null logger.) Furthermore, all of these ops assume the LogService (now Logger) is non-null, which it may not necessarily be now, so they all need to wrap all log statements with if (log != null).

@maarzt
Copy link
Contributor Author

maarzt commented Apr 28, 2018

Ok, the consequences your are describing clearly call for a better solution, than what I suggested.

@ctrueden
Copy link
Member

CC @gselzer: next year when we take stock of the current Ops progress, let's discuss this Logger issue. Now that the Ops framework is fundamentally not built on Command/Module anymore, this PR may be easier to integrate into the SciJava framework in a backwards-compatible way. Ideally, op implementations should be able to declare a @Parameter Logger for logging, without it affecting the function signature, in the same way they can declare service parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants