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

Syntax highlighting for fenced code blocks, in command help output, on macOS works #7472

Open
nazmulidris opened this issue Jan 27, 2024 · 12 comments

Comments

@nazmulidris
Copy link
Contributor

Current behavior

Currently, for Ockam Command CLI help output, the syntax highlighting for fenced code blocks has been disabled. It didn't work properly on macOS.

This commit removes the use of the use of FencedCodeBlockHighlighter::process_line().

Here's an example on macOS of the current behavior. Run ockam project ticket --help. It produces the following output in the Examples section. Note that it does not have any syntax highlighting.

Image

Desired behavior

With syntax highlighting via syntect crate, it should be whatever syntect produces for sh syntax set.

Here are some places to get started in the code.

@YorickdeJong
Copy link

Hey I can take a look at this one if it's still free

@nazmulidris
Copy link
Contributor Author

@YorickdeJong That's awesome, this is all yours. Please let us know if you have any questions as you explore. You can also ask questions on the contributors discord https://discord.ockam.io

@YorickdeJong
Copy link

Cool thanks! I checked out the podcasts online and I really like the project! Combining rust, swift and microservices, awesome

@YorickdeJong
Copy link

Quick question, it seems that a few years ago this project stopped using the main branch. can I assume that the developer branch is the production branch?

@nazmulidris
Copy link
Contributor Author

@YorickdeJong That's right. The develop branch is the "main" branch 👍🏽

@YorickdeJong
Copy link

YorickdeJong commented Jan 29, 2024

Quick update, I ran a test and debugged the FencedCodeBlockHighlighter struct. It seems that the problem arises from the THEME variable. On MacOS it cannot determine the terminal background and sets it to None. In turn, this causes the process_line() function to panic, as the FencedCodeBlockHighlighter.inner variable is also set to None.

static THEME: Lazy<Option<Theme>> = Lazy::new(|| {
    let theme_name = match TerminalBackground::detect_background_color() {
        TerminalBackground::Light => "base16-ocean.light",
        TerminalBackground::Dark => "base16-ocean.dark",
        TerminalBackground::Unknown => "base16-ocean.dark" //return None,
    };
    let mut theme_set = ThemeSet::load_defaults();
    let theme = theme_set.themes.remove(theme_name).unwrap();
    Some(theme)
});

When I ran the test with:

TerminalBackground::Unknown => "base16-ocean.dark"

instead of

TerminalBackground::Unknown => None

I did get colors to show on macOS

image

I also managed to get the correct coloring for the issue at hand in the test:

image

However, it seems that running ockam project ticket --help seems to not be affected by the implemented changes. This arises from the fact that it has is_markdown() configured, which doesn't have syntax_highlighted enabled

if is_markdown() {
        Box::leak(body.to_string().into_boxed_str())
    } else {
        let syntax_highlighted = process_terminal_docs(body.to_string());
        Box::leak(syntax_highlighted.into_boxed_str())
    }
}

@nazmulidris
Copy link
Contributor Author

nazmulidris commented Jan 29, 2024

@YorickdeJong Good job examining the code 👍🏽 .

process_terminal_docs()

You might not see ockam project ticket --help produce syntect highlighted output in the help / examples section at the bottom of the output because currently the call to process_line() has been removed from here process_terminal_docs().

This is what the old (removed) code used to look like here:

...
    for line in LinesWithEndings::from(input.as_str()) {
        // Try to process fenced code blocks
        if code_highlighter.process_line(line, &mut output) {
            continue;
        }

        // Replace headers with bold and underline text
        if HEADER_RE.is_match(line) {
...

is_markdown()

docs.rs is used to post process clap help text output (before it is displayed to stdout), as well as generate the MD manual that is can be seen here: https://command.ockam.io/manual/. Here's more info on this: build-trust/ockam-documentation#16

The code path that you're looking for is the else block:

pub(crate) fn render(body: &str) -> &'static str {
    if is_markdown() {
        Box::leak(body.to_string().into_boxed_str())
    } else {
        let syntax_highlighted = process_terminal_docs(body.to_string());
        Box::leak(syntax_highlighted.into_boxed_str())
    }
}

static THEME

I think the expression assigned to needs some updating.

If it isn't possible to determine what the background color is (based on those heuristics), ie, the theme_name is TerminalBackground::Unknown, then it should default to base16-ocean.dark. That heuristic to determine the background isn't really reliable. It's a heuristic after all. What are your thoughts on this?

call process_line() from process_terminal_docs()

Let's say the fallback theme is base16-ocean.dark, and process_line() is called (currently the call to this function has been removed).

  • The implementation of process_line() itself needs to be examined as well. For eg, it uses the as_24_bit_terminal_escaped(). It states in the docs for this function that is is meant for debugging and testing only. With the use of this function there's a need to append a RESET ANSI escape code as well (output.push("\x1b[0m".to_string());). You might try getting the syntax highlighting to work with this existing implementation though. Just to confirm that we are heading in the right direction.

  • Instead it may be better to create an implementation to generate these ANSI escape sequences manually using the r3bl_ansi_color crate. Here's an example of how this might be done (from a different crate that also does something similar): here.

@YorickdeJong
Copy link

YorickdeJong commented Jan 29, 2024

Hi @nazmulidris,

Thank you for the detailed feedback. I've reintroduced the call to process_line() within process_terminal_docs() to ensure that code blocks are processed and highlighted correctly.

/// Use a shell syntax highlighter to render the fenced code blocks in terminals
fn process_terminal_docs(input: String) -> String {
    let mut output: Vec<String> = Vec::new();
    let mut code_highlighter = FencedCodeBlockHighlighter::new();

    for line in LinesWithEndings::from(&input) {
        
        // Check if the current line is a code block start/end or content.
        let is_code_line = code_highlighter.process_line(line, &mut output);
        if !is_code_line {
            // The line was not part of a code block, so process normally.

            // Replace headers with bold and underline text
            if HEADER_RE.is_match(line) {
                output.push(line.to_string().bold().underlined().to_string());
            }
            // Replace subheaders with underlined text
            else if line.starts_with("#### ") {
                output.push(line.replace("#### ", "").underlined().to_string());
            }
            // Catch all other lines
            else {
                output.push(line.to_string());
            }
        }
    }
    output.join("")
}

Such that my tests are able to include color into the output for shell commands. I should have mentioned that, apologies.

is_markdown(): I think the reason we aren't seeing any colors for the markdown menu is that syntax_highlight isn't configured for it

static THEME: I agree with your suggestion that we should default "base16-ocean.dark" if no background can be determined, as most developers use dark mode anyways.

Manual ANSI Sequences: I plan to explore the use of the r3bl_ansi_color crate to manually generate ANSI escape sequences for syntax highlighting tomorrow. Will keep you posted.

@nazmulidris
Copy link
Contributor Author

nazmulidris commented Jan 29, 2024

@YorickdeJong Thanks for the clarification 🙏🏽 . That's great that you are calling
let is_code_line = code_highlighter.process_line(line, &mut output);.

Re: is_markdown()

We actually do not want those ANSI escape sequences to be generated for Markdown generation. We only want them to be generated for stdout display output. So the is_markdown() check is working as intended 👍🏽 .

Re: static THEME

That's great that you agree with that too 👏🏽 .

Manual ANSI sequences

Please let me know if you have any questions about this. The example I shared with you is from the r3bl_tui crate which does not output to r3bl_ansi_color.

  • However, it does have the algorithm that can be copied in order to port that implementation so that it goes from syntect -> r3bl_ansi_color output -> stdout.
  • The AnsiStyledText struct implements Display trait, so you can just use println!("{}... or format!("{}... to send it to stdout.

@YorickdeJong
Copy link

YorickdeJong commented Jan 30, 2024

Hi @nazmulidris,

Thanks for your suggestions.

Re: Manual ANSI sequences

I implemented the r3bl_ansi_color library:

    #[allow(dead_code)]
    pub fn process_line(&mut self, line: &str, output: &mut Vec<String>) -> bool {

        if let Some(highlighter) = &mut self.inner {
            if line == "```sh\n" {
                self.in_fenced_block = true;
                return true;
            }

            if !self.in_fenced_block {
                return false;
            }

            if line == "```\n" {
                // Push a reset to clear the coloring.
                output.push("\x1b[0m".to_string());
                self.in_fenced_block = false;
                return true;
            }

            // Highlight the code line
            let ranges: Vec<(Style, &str)> = highlighter
                .highlight_line(line, &SYNTAX_SET)
                .unwrap_or_default();

            // Convert each syntect range to an ANSI styled string
            convert_syntect_style_to_ansi(output, &ranges);

            true
        } else {
            false
        }
    }
}

With a converted function:

pub fn convert_syntect_style_to_ansi(output: &mut Vec<String>, ranges: &Vec<(Style, &str)>) {
    for (style, text) in ranges {
        let ansi_styled_text = AnsiStyledText {
            text: text,
            style: &[
                StyleAnsi::Foreground(Color::Rgb(style.foreground.r, style.foreground.g, style.foreground.b)),
            ],
        };

        output.push(ansi_styled_text.to_string());
    }
}

My tests come back positively. It should be noted that I currently only add Foreground coloring, but if needed bold, italic, underlining and background options can also be set.

Running ockam project ticket --help

I'm still having problems coloring the results from running the command ockam project ticket --help. These results still don't show color, whereas the test I'm running do show color. It seems that when running the code in debug mode, text is colored as well. I included a picture, on the left in debug mode and on the right in normal mode, to show case what I mean.

image

Would you happen to know why the color is still missing in the normal mode? Thanks!

@nazmulidris
Copy link
Contributor Author

@YorickdeJong The code using the conversion to AnsiStyledText looks good! We can incorporate italic, bold, underline as well if we need.

How did you run command in debug mode? I wasn't sure how you were able to run it in debug vs normal mode. Also if you can raise a PR with the code changes that you've made, that will allow me to take a look at the code that you've created, and I can take a look at why the colorization isn't making it to the output of ockam project ticket --help.

Also, you might need to create a separate PR to accept the CLA, in order for us to be able to merge your PRs in the future.

@YorickdeJong
Copy link

YorickdeJong commented Jan 31, 2024

Hi @nazmulidris,

Thanks! I have created a pull request: #7495. In this PR I still included the tests that I ran for the coloring functions, if needed I can remove those.

The commands that I ran to enter debug mode and run the ockam command are:

lldb target/debug/ockam
process launch -- project ticket --help

I think found the silly reason for the difference between the two. When running ockam project ticket help, I am using the production version of ockam instead of the local version, hence no changes are applied. Apologies for the confusion this has caused

I am looking forward to your review, let me know if any changes need to be made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants