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

make the log output faster, and add colors too #832

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

Conversation

artoonie
Copy link
Collaborator

@artoonie artoonie commented May 8, 2024

Closes #831
Closes #830

The issue was that appending to the textarea used string concatenation, which required reading the entire log string and recreating it each time a new log message was added. That made each subsequent GUI run slower, and also explains why we didn't see this in the unit tests: it's only a GUI issue. It also caused the GUI to hang for a short bit of time each time a log message was added.

I replaced the TextArea with a ListView. This makes it trivial to do something that I've wanted for a while: colors for log levels. I can change the foreground or background color. I would appreciate feedback on how this looks:

Screenshot 2024-05-08 at 1 42 37 PM

I also removed the word wrap, since the ListView seems to hiccup if there are changing line widths during fast scrolling. If needed, I can try to overcome this, but I like the way it looks without word wrap.

@artoonie artoonie force-pushed the bugfix/issue-831_fix-log-output-slow branch from 253cc41 to c1dbaef Compare May 8, 2024 20:48
@artoonie artoonie force-pushed the bugfix/issue-831_fix-log-output-slow branch from c1dbaef to 66a89ef Compare May 8, 2024 22:08
@artoonie artoonie requested a review from yezr May 8, 2024 22:13
@yezr
Copy link
Collaborator

yezr commented May 9, 2024

I love the colors. Side scroll totally works. My only nit is the line spacing. Is it possible to move the lines closer?
image

@artoonie
Copy link
Collaborator Author

artoonie commented May 10, 2024

demo.mov

Sounds good. I spruced up the style a little more:

  1. Reduced line spacing
  2. Decided to go back to word-wrap, but this is an easy change either way. I realize there's important info at the end of the line, and may be better to not force a scroll this has changed after this demo video was made
  3. Made the highlight reach across the whole line
  4. Added the ability to "select" each row and have that row maintain its log severity color (row = a single log string, which may span multiple lines)
  5. Added the ability to copy lines via right click

@tarheel
Copy link
Contributor

tarheel commented May 19, 2024

The code changes seem pretty uncontroversial. Will let @yezr accept.

@artoonie
Copy link
Collaborator Author

From https://docs.oracle.com/javase/8/javafx/api/javafx/application/Platform.html#runLater-java.lang.Runnable-

NOTE: applications should avoid flooding JavaFX with too many pending Runnables. Otherwise, the application may become unresponsive. Applications are encouraged to batch up multiple operations into fewer runLater calls. Additionally, long-running operations should be done on a background thread where possible, freeing up the JavaFX Application Thread for GUI operations.

That was the cause of the GUI lag that has existed outside of this PR, but wasn't as noticeable until the new GUI's loading bar.

@tarheel there is a small final commit on this PR that fixes it -- it's important to get this right to avoid race conditions where the log output never updates. It's the first and only use of a synchronized block in the codebase. Can you take a quick look at commit 21dc086?

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.

Tabulate takes ~5 seconds longer on consecutive runs UI hangs during tabulation
3 participants