rcv icon indicating copy to clipboard operation
rcv copied to clipboard

make the log output faster, and add colors too

Open artoonie opened this issue 1 year ago • 3 comments

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, which looks cleaner to me, but does add side-scroll.

Feedback please:

  1. Is the sidescroll acceptable?
  2. Are the colors I chose for warning and severe okay?

artoonie avatar May 08 '24 20:05 artoonie

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

yezr avatar May 09 '24 15:05 yezr

https://github.com/BrightSpots/rcv/assets/537316/fa2aa0da-ee3c-49b4-8c8f-ca9c1597ff03

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

artoonie avatar May 10 '24 19:05 artoonie

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

tarheel avatar May 19 '24 21:05 tarheel

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?

artoonie avatar May 27 '24 23:05 artoonie

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 🔥

yezr avatar May 30 '24 17:05 yezr

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.

artoonie avatar May 30 '24 18:05 artoonie

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

yezr avatar May 30 '24 20:05 yezr