-
Notifications
You must be signed in to change notification settings - Fork 11
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
Enhance console window with regard to log messages #25
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for filing this. Will you be around during the Learnathon? If so, that might be a great time to get this and scijava/scijava-common#255 finally merged.
pom.xml
Outdated
@@ -106,6 +107,7 @@ Wisconsin-Madison.</license.copyrightOwners> | |||
<dependency> | |||
<groupId>org.scijava</groupId> | |||
<artifactId>scijava-common</artifactId> | |||
<version>2.64.1-SNAPSHOT</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit should be labeled WIP since it adds a SNAPSHOT dependency. All such dependencies must be purged from history before they merge to master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it is better to use <scijava-common.version>2.64.1-SNAPSHOT</scijava-common.version>
in the <properties>
instead of hardcoding the <version>
in the <dependencies>
section. The former allows overriding the version in various circumstances such as the SciJava melting pot script, whereas the latter bakes in the version into the dependency hierarchy in an immutable way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Next time I will do it that way. Thanks for the advices!
Yes, I will be at the Learnathon too. Would be great if we get this done there. |
4d4ac55
to
450d7be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some miscellaneous comments!
@@ -59,8 +59,7 @@ public static void main(final String[] args) throws Exception { | |||
final Context context = new Context(); | |||
context.service(UIService.class).showUI(); | |||
|
|||
System.out.print("Hello "); | |||
System.err.println("world!"); | |||
System.out.println("Hello world!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you combine these? It does not seem related to the stated purpose of this commit. It also reduces the effectiveness of the manual test, since the console pane is supposed to handle line fragments gracefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the test is as it was before. I changed it in the past, because I handled line fragments differently.
* @author Matthias Arzt | ||
*/ | ||
@IgnoreAsCallingClass | ||
public class ConsolePanel extends JPanel implements OutputListener |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this naming very confusing. Now we have a ConsolePanel
class which is actually Swing-specific (but does not have Swing in its class name) which is a field of SwingConsolePane
.
import org.scijava.ui.swing.StaticSwingUtils; | ||
|
||
/** | ||
* LoggingPanel can display log message and console output as a list, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is not called LoggingPanel
, but ConsolePanel
, so I guess the javadoc should be adjusted... assuming we keep things structured this way.
* LoggingPanel implements {@link LogListener} and {@link OutputListener}, that | ||
* way it can receive log message and console output from {@link LogService}, | ||
* {@link Logger} and {@link org.scijava.console.ConsoleService} | ||
* {@link LoggingPanel} can displays log messages, and provides convenient ways |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can displays
should be can display
import org.scijava.ui.swing.StaticSwingUtils; | ||
|
||
/** | ||
* LoggingPanel can display log message and console output as a list, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this was added as a copy...?
final PrintWriter printer = new PrintWriter(sw); | ||
|
||
printWithBrackets(printer, message.time().toString()); | ||
printWithBrackets(printer, LogLevel.prefix(message.level())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to discuss this LogLevel.prefix
method. The idea of "prefix" previously included the brackets and trailing space, but now no longer does, and this discrepancy concerns me.
* | ||
* @author Matthias Arzt | ||
*/ | ||
class LogSourcesPanel extends JPanel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be OK instead as a private inner class of LoggingPanel? I like to avoid non-public classes as outer classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make ItemTextPane, TextFilterField and LogSourcesPanel inner classes of LoggingPanel, but this would result in files with more that 1k lines. For me such file is a pain to read and a barrier to fully understand the code. Package-private classes are not nice, but if they can be tested separately and code becomes more readable, than I prefer them. And I don't want them to be public, so that changes can be made without breaking code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, no problem, you're right. Let's stick with package-private outer classes then.
@@ -0,0 +1,99 @@ | |||
package org.scijava.ui.swing.console; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing license header.
@@ -0,0 +1,79 @@ | |||
package org.scijava.ui.swing.console; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing license header.
SwingConsolePane is responsible for lazy initialization and showing the console window at correct time. ConsolePanel is responsible for holding a text area to show console output. ConsolePanel will become a starting point for implementing a LoggingPanel.
At this point LoggingPanel is basically a copy of ConsolPanel. But it will envole to something very different. Features like filtering messages by log source, level and text, and options for message formatting will be added.
This requiers to actually store and log messages that are received by LoggingPanel, in correct order and with meta data. (LogRecorder is used for this purpose.) Whenever the user changes the text to filter for, it is neccessary to reprocess and redisplay the recorded log messages. This might require to much time to be processed in the swing thread, therefore the new class ItemTextPane is introduced. It can display an expanding list of items, and uses and worker thread when the list is replaced.
Now LoggingPanel can be used to record and show the calling class of the log messages.
I implemented all the changes we discussed. |
Thank you very much! 🍻 |
Incredible, it happened! Cool! Thank you too 🍻 |
Add features to the scijava console window:
Class LoggingPanel can be used by comprehensive plug ins to show only a subset of log messages related to them.
This pull request depends on scijava/scijava-common#255