← 返回首页
Fix Logger Parameter with Ops by maarzt · Pull Request #319 · scijava/scijava-common · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
Resetting focus

Fix Logger Parameter with Ops#319

Open
maarzt wants to merge 3 commits into
scijava:masterfrom
maarzt:add-logger-injection
Open

Fix Logger Parameter with Ops#319
maarzt wants to merge 3 commits into
scijava:masterfrom
maarzt:add-logger-injection

Conversation

Copy link
Copy Markdown
Contributor

maarzt commented Apr 27, 2018
edited
Loading

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.
Copy link
Copy Markdown
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.

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Contributor Author

maarzt commented Apr 27, 2018
edited
Loading

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

Copy link
Copy Markdown
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).

Copy link
Copy Markdown
Contributor Author

maarzt commented Apr 28, 2018

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

Copy link
Copy Markdown
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.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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

Footer

© 2026 GitHub, Inc.