ImageJ2Options: add log level selection #137

stelfrich · 2016-03-23T18:41:06Z

Add selection of the SciJava log level to the ImageJ2 options dialog.

Known issues:

Doesn't set the drop down's default value to the currently active log level. Is there a possibility to access the generated widget from an initialize() method?
Could LogService.WARN, etc, be implemented as an enum?

ctrueden · 2016-03-23T18:42:36Z

src/main/java/net/imagej/legacy/ImageJ2Options.java

@@ -80,10 +80,20 @@
 		description = "<html>Whether to use ImageJ2's file I/O mechanism when "
 			+ "opening files.<br>Image files will be opened using the SCIFIO library "
 			+ "(SCientific Image<br>Format Input and Output), which provides truly "
-			+ "extensible support for<br>reading and writing image file formats.",
+			+ "extensible support for<br>reading and writing image file formats.</html>",


Is that closing tag really necessary?

With HTML5 it is optional. Shall I remove it?

Well, that HTML is the bastardized Java version, which Swing uses for rich text formatting. AFAIK, you absolutely do not need the closing tag. And in any case: adding it here is unrelated to the stated purpose of your commit.

ctrueden · 2016-03-23T18:56:00Z

Is there a possibility to access the generated widget from an initialize() method?

Yes, although you shouldn't need to access the widget. Just give the field an initializer method in its @Parameter declaration, then change the value of that field to match what the LogService reports.

Could LogService.WARN, etc, be implemented as an enum?

Not without breaking backwards API compatibility in SciJava Common. I guess we could do this for SJC3. But what would be the advantage? I guess as things stand, when you call LogService.getLevel() you'll have to convert the int back into the level using case logic, which is unfortunate...

stelfrich · 2016-03-23T19:22:41Z

Just give the field an initializer method in its @Parameter declaration, then change the value of that field to match what the LogService reports.

That does not set the selection of the combo box. That's why I was asking about the widget. Should the selection of the combo box be set according to the value of the annotated field?

when you call LogService.getLevel() you'll have to convert the int back into the level using case logic, which is unfortunate...

Exactly...

ctrueden · 2016-03-23T19:25:05Z

Should the selection of the combo box be set according to the value of the annotated field?

Yes, that is what is supposed to happen. The initial value of the field should determine the initially selected item of the combo box, according to the equals method.

ctrueden · 2016-03-23T19:27:42Z

src/main/java/net/imagej/legacy/ImageJ2Options.java

+	/**
+	 * Parses a log level from a {@code int}
+	 * 
+	 * FIXME Could be removed if log level is defined as enumeration


Please mark this as TODO rather than FIXME. From the ImageJ coding style page:

For code (or lack thereof) that is considered wrong or broken, and in need of repair, we add a "FIXME" comment with the relevant developer's initials, to serve as a reminder to address it as soon as time allows.

When additional work is needed somewhere, but not urgently, we add a "TODO" comment marking it.

stelfrich · 2016-03-23T19:34:17Z

The initial value of the field should determine the initially selected item of the combo box.

That does not work for me.

Some additional insights, @ctrueden

initialize() sets logLevel field properly; when SwingChoiceWidget.set(WidgetModel) is called, the module of WidgetModel has logLevel == ERROR no matter what initialize() has done beforehand

Shall I open a new issue?

ctrueden · 2016-03-23T20:40:18Z

I just tried to debug into this and could not reproduce. It then occurred to me what the problem is: the parameter value was persisted by the SaveInputsPreprocessor and is being restored the same every time by the LoadInputsPreprocessor (which runs after the InitPreprocessor). Please add persist = false to the logLevel parameter, and I bet you the problem will disappear for you.

ctrueden · 2016-03-23T20:47:44Z

We could add a hack that the SaveInputsPreprocessor ignores inputs that have an initializer callback. I'm not sure if that would break any code anywhere, but it would prevent others from stumbling over this in the future (I think I have stumbled over it at least twice before, in my dim memory).

stelfrich · 2016-03-23T20:58:50Z

and I bet you the problem will disappear for you.

It did 👏 Thanks, @ctrueden!

We could add a hack that the SaveInputsPreprocessor ignores inputs that have an initializer callback.

I would appreciate that hack since debugging such issues repeatedly is a pain (I thought the issue was somewhere with the DefaultValuePreprocessor because ERROR is the first choice and might be the fallback). This doesn't seem to be an urgent issue, so I could give it a shot next week?

ctrueden · 2016-03-23T21:07:51Z

I could give it a shot next week?

Sure. Here is where you would add the following code:

// NB: Do not persist values which are computed via an initializer.
if (item.getInitializer() != null && !item.getInitializer().isEmpty()) return;

Again: this breaks the (theoretical) use case where a command uses the initializer to specify the initial initial value (i.e.: the initial value the first time the command ever runs), but then wants to prefer the last-chosen value of the user for the initial value upon subsequent executions. I am not sure whether that is a big deal. But without this hack, you can have the behavior work either way, by setting persist=false or not.

Disable the persisting of a ModuleItems if an initialize() method is set and is not empty. This way, initialize() can overwrite the value of a ModuleItem even if persist=true is assumed implicitly. See: * imagej/imagej-legacy#137 (comment)

stelfrich · 2016-03-24T15:13:14Z

Sure. Here is where you would add the following code

Feel free to do it yourself the next time (if it is just one line). I think writing that comment has taken you longer than actually integrating that line 😄

ctrueden · 2016-03-24T16:03:01Z

Feel free to do it yourself the next time (if it is just one line). I think writing that comment has taken you longer than actually integrating that line 😄

Nope, I didn't test it. And untested code is nonworking code. Besides: I am trying to train you. Training always takes longer than doing it yourself. But hopefully not globally over all time.

ctrueden · 2016-07-01T17:51:17Z

src/main/java/net/imagej/legacy/ImageJ2Options.java

@@ -90,9 +90,10 @@
 	@Parameter(
 		label = "SciJava log level",
 		description = "<html>Log level for SciJava</html",
+		initializer = "initializeLogLevel",


Please add persist = false just for clarity here (even though scijava/scijava-common#236 will probably merge soon).

ctrueden · 2016-07-01T17:52:13Z

@stelfrich Can you take a couple of minutes to clean up these commits as mentioned in my earlier comments? Then we can merge it!

stelfrich · 2016-07-02T08:30:41Z

Thanks for the comments @ctrueden. I took the liberty to reformat both @Parameter annotations after realizing that they first one was not formatted according to the code style (or was it?). I have reformatted with Eclipse Mars and the current version of the ImageJ preferences.

Disable the persisting of a ModuleItems if an initialize() method is set and is not empty. This way, initialize() can overwrite the value of a ModuleItem even if persist=true is assumed implicitly. See: * imagej/imagej-legacy#137 (comment)

ctrueden · 2017-01-01T16:24:35Z

I finally found time to rewrite/clean these commits. Merging. Thanks again @stelfrich!

ctrueden reviewed Mar 23, 2016
View reviewed changes

stelfrich force-pushed the log-level-in-ij2-options branch from de4895c to 243739c Compare March 23, 2016 19:19

ctrueden reviewed Mar 23, 2016
View reviewed changes

stelfrich force-pushed the log-level-in-ij2-options branch from 4dcc348 to d96d028 Compare March 23, 2016 20:36

stelfrich mentioned this pull request Mar 24, 2016

DefaultModuleService: change persisting of items scijava/scijava-common#236

Open

ctrueden reviewed Jul 1, 2016
View reviewed changes

stelfrich force-pushed the log-level-in-ij2-options branch from f252ed6 to 7cd2af8 Compare July 2, 2016 16:29

stelfrich added 4 commits January 1, 2017 10:22

ImageJ2Options: add log level selection

af0fd0e

ImageJ2Options: use initializer for log level

313a747

ImageJ2Options: fix persistence issue

f28b2cb

ImageJ2Options: clean up and reformat code

be07ac0

ctrueden force-pushed the log-level-in-ij2-options branch from 7cd2af8 to be07ac0 Compare January 1, 2017 16:23

ctrueden merged commit 3fbc4ad into master Jan 1, 2017

ctrueden deleted the log-level-in-ij2-options branch January 1, 2017 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ImageJ2Options: add log level selection #137

ImageJ2Options: add log level selection #137

stelfrich commented Mar 23, 2016

ctrueden Mar 23, 2016

stelfrich Mar 23, 2016

ctrueden Mar 23, 2016

ctrueden commented Mar 23, 2016

stelfrich commented Mar 23, 2016

ctrueden commented Mar 23, 2016

ctrueden Mar 23, 2016

stelfrich commented Mar 23, 2016

ctrueden commented Mar 23, 2016

ctrueden commented Mar 23, 2016

stelfrich commented Mar 23, 2016

ctrueden commented Mar 23, 2016

stelfrich commented Mar 24, 2016

ctrueden commented Mar 24, 2016

ctrueden Jul 1, 2016

ctrueden commented Jul 1, 2016

stelfrich commented Jul 2, 2016

ctrueden commented Jan 1, 2017

ImageJ2Options: add log level selection #137

ImageJ2Options: add log level selection #137

Conversation

stelfrich commented Mar 23, 2016

ctrueden Mar 23, 2016

Choose a reason for hiding this comment

stelfrich Mar 23, 2016

Choose a reason for hiding this comment

ctrueden Mar 23, 2016

Choose a reason for hiding this comment

ctrueden commented Mar 23, 2016

stelfrich commented Mar 23, 2016

ctrueden commented Mar 23, 2016

ctrueden Mar 23, 2016

Choose a reason for hiding this comment

stelfrich commented Mar 23, 2016

ctrueden commented Mar 23, 2016

ctrueden commented Mar 23, 2016

stelfrich commented Mar 23, 2016

ctrueden commented Mar 23, 2016

stelfrich commented Mar 24, 2016

ctrueden commented Mar 24, 2016

ctrueden Jul 1, 2016

Choose a reason for hiding this comment

ctrueden commented Jul 1, 2016

stelfrich commented Jul 2, 2016

ctrueden commented Jan 1, 2017