Skip to content

feat: updated log viewer #58

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ronnakamoto
Copy link

@ronnakamoto ronnakamoto commented May 17, 2025

This PR includes significant improvements to the log viewer.

Thunder.Log.Viewer.mov

@@ -26,11 +28,149 @@ pub struct ConsoleCommand {
command: thunder_app_cli_lib::Command,
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub enum LogLevel {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you just use tracing::Level here?


fn color(&self) -> Color32 {
match self {
LogLevel::Error => Color32::from_rgb(255, 85, 85), // Bright red
Copy link
Collaborator

Choose a reason for hiding this comment

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

These colors should be defined as consts somewhere

}

impl LogEntry {
fn parse(line: &str) -> Option<Self> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of parsing log lines, we should refactor LineBuffer to store unformatted log lines, and then you won't need LogEntry either

}
}

fn filtered_logs(&self) -> Vec<&LogEntry> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Filtering should be set in the tracing filter for the log buffer, in app/main.rs.


// Main log view
// Create a clone of the filtered logs to avoid borrow issues
let filtered_logs: Vec<LogEntry> = self.filtered_logs().iter().map(|&log| log.clone()).collect();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cloning the logs could be extremely expensive! This should be avoided if possible

&& !self.running_command.load(atomic::Ordering::SeqCst),
command_input,
// Parse logs from the line buffer
self.parse_logs();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Parsing all logs from the line buffer on each update could be very expensive. If you must process the log lines, either do it in the line buffer, or use streams so that only new log lines get parsed

);

if command_input_resp.ctx.input_mut(|input| {
input.consume_key(Modifiers::NONE, Key::Enter)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can the user input a multiline command? I think it is best to preserve the existing behavior with Shift+Enter

}

if ui.button("Run").clicked() && app.is_some() && !self.running_command.load(atomic::Ordering::SeqCst) {
self.console_command(app.unwrap());
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it is safe to unwrap app here, then you should also do so when accepting a new command with the Enter key

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.

2 participants