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

Separate PluginLoggerImpl from LoggerImpl #2075

Merged
merged 2 commits into from
Feb 10, 2014

Conversation

joshmoore
Copy link
Member

Since all methods in LoggerImpl were checking
if (runAsPlugin < 0) {...} separating out the logic
into a separate class is straight-forward. The reason
for doing this, however, is that in the case that the
IJ plugin is used with the loci_tools.jar jar, which
contains log4j, then the client hangs at

"Loading Logger Service"

--depends-on ome/bioformats#889
--depends-on #2074

Since all methods in LoggerImpl were checking
`if (runAsPlugin < 0) {...}` separating out the logic
into a separate class is straight-forward. The reason
for doing this, however, is that in the case that the
IJ plugin is used with the `loci_tools.jar` jar, which
contains log4j, then the client hangs at

    "Loading Logger Service"
@joshmoore joshmoore mentioned this pull request Feb 6, 2014
@jburel
Copy link
Member

jburel commented Feb 6, 2014

@joshmoore: good idea

@jburel
Copy link
Member

jburel commented Feb 6, 2014

@joshmoore: only concern now (I did not think about it at first) is that when insight is run as a knime node, there will be no logging at all.
Knime uses log4j+ will also need to adjust way to insight log file location. (known limitation)

@jburel jburel added the dev_5_0 label Feb 6, 2014
@joshmoore
Copy link
Member Author

@jburel : is there anything you want me to try to change for 5.0.0? Do we need a ticket for 5.0.1?

@jburel
Copy link
Member

jburel commented Feb 7, 2014

@joshmoore: I will probably leave it as it is for 5.0.0. I think we have a ticket for passing "external" log. I will check.

@jburel
Copy link
Member

jburel commented Feb 7, 2014

Copyright (C) 2006-14 University of Dundee. All rights reserved.
Remove 2006

* This program is free software; you can redistribute it and/or modify
Tab to be replaced by space. (It is my fault!!!)

@joshmoore
Copy link
Member Author

Pushed.

@mtbc
Copy link
Member

mtbc commented Feb 10, 2014

The code looks fine and the latest commit is inoffensive. Does that suffice? Unfortunately I don't seem to be able to actually run Insight from ImageJ at the moment -- see #2074 (comment)

@joshmoore
Copy link
Member Author

@mtbc: yup, think that's fine. Thanks.

joshmoore added a commit that referenced this pull request Feb 10, 2014
Separate PluginLoggerImpl from LoggerImpl
@joshmoore joshmoore merged commit 24d1283 into ome:dev_5_0 Feb 10, 2014
@joshmoore joshmoore deleted the plugin-logger-impl branch February 10, 2014 16:51
@mtbc
Copy link
Member

mtbc commented Feb 11, 2014

Great. (-: I did finally get Insight running in ImageJ and it wasn't obviously horribly broken!

@jburel
Copy link
Member

jburel commented Feb 26, 2014

--no-rebase
Commits cherry-picked see gh-2122

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

Successfully merging this pull request may close these issues.

3 participants