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

Add utility function ansi::slice_ansi_str #206

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

Conversation

remi-dupre
Copy link
Contributor

@remi-dupre remi-dupre commented Feb 7, 2024

Hi!

While working on a bug of indicatif (cf. console-rs/indicatif#627), I needed an ANSI-aware slicing method. I think it made more sense to add it in this crate.

/// Slice a `&str` in terms of text width. This means that only the text
/// columns strictly between `start` and `stop` will be kept.
///
/// If a multi-columns character overlaps with the end of the interval it will
/// not be included. In such a case, the result will be less than `end - start`
/// columns wide.
///
/// If non-empty head and tail are specified, they are inserted between the
/// ANSI symbols from truncated bounds and the slice.
///
/// This ensures that escape codes are not screwed up in the process.
pub fn slice_str<'a>(s: &'a str, head: &str, bounds: Range<usize>, tail: &str) -> Cow<'a, str> {
    ...
}

This is a generalisation of truncate_str 😃

@remi-dupre remi-dupre marked this pull request as ready for review February 7, 2024 18:24
@remi-dupre remi-dupre force-pushed the ansi-slice branch 3 times, most recently from 3811bcc to 3324138 Compare February 7, 2024 21:52
src/unix_term.rs Outdated
@@ -56,7 +56,7 @@ pub fn terminal_size(out: &Term) -> Option<(u16, u16)> {
#[allow(clippy::useless_conversion)]
libc::ioctl(out.as_raw_fd(), libc::TIOCGWINSZ.into(), &mut winsize);
if winsize.ws_row > 0 && winsize.ws_col > 0 {
Some((winsize.ws_row as u16, winsize.ws_col as u16))
Some((winsize.ws_row, winsize.ws_col))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just a random (new?) clippy lint : these are already u16 🤷

src/utils.rs Outdated
Comment on lines 71 to 77
AnsiCodeIterator::new(s)
.filter(|(_, is_ansi)| !is_ansi)
.map(|(sub, _)| str_width(sub))
.sum()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not related to the MR, but while implementing a less optimized draft of my fix I thought this could spare a few allocs. This is probably not something to care about so I wouldn't mind if you want to revert this for the sake of simplification.

@remi-dupre remi-dupre force-pushed the ansi-slice branch 4 times, most recently from 0f33b85 to ce77cc5 Compare February 7, 2024 23:25
@remi-dupre remi-dupre changed the title Add function ansi::slice_ansi_str Add utility function ansi::slice_ansi_str Feb 8, 2024
@djc
Copy link
Member

djc commented Feb 10, 2024

Can you avoid let .. else in order to pass the MSRV tests?

@remi-dupre
Copy link
Contributor Author

Indeed, that should be good now 👍

@remi-dupre
Copy link
Contributor Author

remi-dupre commented Feb 12, 2024

And format is also a recent-is feature, on it 😬

I'll try to build on 1.56, it didn't work last time I tried it because it fails to resolve regex = "=1.10.3" 😞

EDIT : Ok, got it

@remi-dupre
Copy link
Contributor Author

Should be good, make test runs with rust 1.56 on my machine 👍

@djc djc requested a review from mitsuhiko February 12, 2024 10:41
@remi-dupre remi-dupre force-pushed the ansi-slice branch 3 times, most recently from 97b3716 to 66bb444 Compare March 9, 2025 09:47
@remi-dupre
Copy link
Contributor Author

Hi!

I bumped into this while doing some cleanup and I think it might still be useful. So I've rebased it on latest main!

@djc
Copy link
Member

djc commented Mar 9, 2025

I definitely don't want to accept a semver-incompatible change for this. Is there a way to make this compatible?

@remi-dupre
Copy link
Contributor Author

remi-dupre commented Mar 9, 2025

Hi!

You're right that it would be a shame.

Although I think I made a mistake with semver : I only added a new function without affecting the behavior of the existing, as we're still on unstable scheme 0.x.y this should only affect the last (y) component.

I changed the target version to 0.15.12 and made sure that I didn't modify previously existing test cases 👍

@djc
Copy link
Member

djc commented Mar 11, 2025

Okay, this doesn't currently pass the MSRV jobs in CI, so that's something that will have to be fixed. I'd like a cleaner set of changes for review, with separate commits or PRs for each of these changes:

  • clippy fixes
  • saving allocations on measure_text_width()
  • introducing slice_ansi_str()

Please avoid unrelated changes as much as possible (as such, keep functions/logic in the place as much as possible to minimize the diffs). You can leave the version bumping and changelog to me (note that the CHANGELOG.md file is no longer being used).

@remi-dupre remi-dupre force-pushed the ansi-slice branch 2 times, most recently from 3740c3f to 1599bf9 Compare March 11, 2025 22:09
@remi-dupre
Copy link
Contributor Author

Hello mon ami ! 🥖

Those are super relevant comments 👍

  1. I've created two smaller MRs for parasitic changes
    a. fix: clippy lint (clippy::unnecessary cast) #247
    b. Implement measure_text_width with no allocation #246
  2. Following your advice, I've created another to suggest a fix to the MSRV issue : chore: pin once_cell<1.21 to enforce MSRV #248
  3. I've removed version & changelog related diffs

@remi-dupre
Copy link
Contributor Author

I'm putting this in draft until #250 is solved.

@remi-dupre remi-dupre marked this pull request as draft March 15, 2025 12:15
@remi-dupre remi-dupre marked this pull request as ready for review March 16, 2025 13:25
@remi-dupre
Copy link
Contributor Author

remi-dupre commented Mar 16, 2025

Rebased & ready for review again! 😄

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