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

Merged
merged 6 commits into from
May 31, 2024

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:
image

@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?

import java.util.Date;
import java.util.List;
import java.util.concurrent.BlockingDeque;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused? otherwise this seems fine to me!

@yezr
Copy link
Collaborator

yezr commented May 30, 2024

nit: can we make the highlight apply only to the text instead of the entire line? In that example video when all of the visible lines have a background there is no contrast with the regular background so it maybe looks like the background is usually that color?

Everything else looks great! The quality of life stuff like right click copy is 🔥

@artoonie
Copy link
Collaborator Author

Happy to make that change! Just want to flag something first:

That's how it used to look here, but in almost all cases with an error, the error is only one severe surrounded by many infos, unlike the demo video I posted. I changed it from only highlighting the text to highlighting the entire line because it makes it easier to see at-a-glance when scrolling through the logs which lines had errors. It's especially helpful if you have sidescroll, since you might be scrolled over to the right and scrolling up, and if the error line is short you won't notice the error line at all, whereas if it's the full line you'll see it regardless of horizontal scroll.

In light of this reasoning, can you let me know if you still prefer it with only the text background highlighted?

Happy to share side-by-side screenshots, it's only one line of code difference between the two options.

@yezr
Copy link
Collaborator

yezr commented May 30, 2024

Thanks for the context Armin. Let's keep the full line highlighting.

Copy link
Collaborator

@yezr yezr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

@artoonie artoonie merged commit bf032c7 into develop May 31, 2024
1 check passed
@artoonie artoonie deleted the bugfix/issue-831_fix-log-output-slow branch May 31, 2024 19:35
artoonie added a commit that referenced this pull request May 31, 2024
* make the log output faster, and add colors too

* spruce up the style

* Issue 830: prevent lagging

* remove unused import

* re-enable and fix word wrap

* better warning color

---------

Co-authored-by: Armin Samii <[email protected]>
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