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

Allow for OSC / Control Characters #137

Closed
dsully opened this issue Mar 27, 2024 · 9 comments · Fixed by #138 or #140
Closed

Allow for OSC / Control Characters #137

dsully opened this issue Mar 27, 2024 · 9 comments · Fixed by #138 or #140
Labels
t: feature A new feature

Comments

@dsully
Copy link
Contributor

dsully commented Mar 27, 2024

A detailed description of the feature you would like to see added.

Hi - I'd like to include OSC-8 hyperlinks in various columns - unfortunately that causes the column (and table) width to be incorrectly calculated.

Explain your usecase of the requested feature

See above.

Here's an example OSC-8 sequence:

printf '\033]8;;https://github.com\033\\This is a link\033]8;;\033\\\n'

Alternatives

No response

Additional context

No response

@dsully dsully added the t: feature A new feature label Mar 27, 2024
@Nukesor
Copy link
Owner

Nukesor commented Mar 28, 2024

Comfy-table has gained support for this behavior via a community contribution.
From my knowledge, that functionality is stable but not yet widely battle-tested.

Since this has been asked for multiple times, I moved the detailed feature-flag documentation from the Cargo.toml to the the README :)

Could you check whether this answers your question and whether you would have found it? I'm not sure where best to place feature flag documentation, as there's sadly no real place for it in the rust ecosystem yet.

https://github.com/Nukesor/comfy-table/blob/ea2c1391d23d8f5357d2885b741c92a080672de9/README.md

@Nukesor
Copy link
Owner

Nukesor commented Mar 28, 2024

Also, if you're interested, It would be awesome if somebody that actually uses this wrote some property tests for this feature (a new property test file would make sense) :D

Theres a huge amount of possible edge-cases when splitting ansi escape sequences and I really would like to see more test coverage on this :)

@Nukesor
Copy link
Owner

Nukesor commented Apr 1, 2024

ping @dsully

@dsully
Copy link
Contributor Author

dsully commented Apr 1, 2024

Heya - I assume the feature is custom_styling? I've not had a chance to try this yet. Hopefully today or tomorrow.

@dsully
Copy link
Contributor Author

dsully commented Apr 2, 2024

So this kind of works:

CleanShot 2024-04-01 at 19 15 25@2x

The cell the OSC-8 escape codes are in are properly sized, but the header cell is not.

If I remove .set_header(...):

CleanShot 2024-04-01 at 19 17 48@2x

let mut table = Table::new();

table
    .load_preset("││──╞═╪╡│    ┬┴┌┐└┘")
    .set_content_arrangement(ContentArrangement::Dynamic)
    .set_header(
            vec![
                "Version",
                "Revision",
                "Branch",
                "Published",
                "Deprecated",
                "State",
                "Flags",
                "Deprecated By",
            ]
        .iter()
        .map(|&title| Cell::new(title))
        .collect::<Vec<_>>(),
    );
fn hyperlink(url: &str, text: &str) -> String {
    format!("\x1B]8;;{url}\x1B\\{text}\x1B]8;;\x1B\\")
        .yellow()
        .underline(Color::Yellow)
        .to_string()
}

Any thoughts here?

@Nukesor
Copy link
Owner

Nukesor commented Apr 2, 2024

Ook.

Since I didn't write this and have never used this, I didn't know the full capabilities of that logic. I kind of expected OSC to work just fine, however it seems that this hasn't been implemented yet.

Would you mind investigating and extending that logic? All current logic is gated behind the custom_styling feature flag, so it should be at least somewhat easy to find the logic thet needs to be extended :)

Original ANSI issue:
#77

@dsully
Copy link
Contributor Author

dsully commented Apr 2, 2024

I looked at it a bit yesterday. I'll try and find time this week to dig deeper.

Is the cell width calculator behind that flag as well?

@Nukesor
Copy link
Owner

Nukesor commented Apr 2, 2024

Yep. That should be the measure_text_width function, which is in content_split/{custom_styling.mod,normal.rs}

Depending on where this should be handled, this is something that might need to be added to the console library upstream.

@dsully
Copy link
Contributor Author

dsully commented Apr 4, 2024

PR above. console doesn't do the right thing. I'll look into addressing it there as well, but for now I'm using ansi-str which does the right thing. Would love a release if this looks good to you. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: feature A new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants