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

[Library] nth option is broken? #300

Closed
murlakatamenka opened this issue Apr 4, 2020 · 7 comments
Closed

[Library] nth option is broken? #300

murlakatamenka opened this issue Apr 4, 2020 · 7 comments
Milestone

Comments

@murlakatamenka
Copy link

examples/nth.rs:

extern crate skim;
use skim::prelude::*;
use std::io::Cursor;

pub fn main() {
    let input = "foo 123";

    let options = SkimOptionsBuilder::default()
        .nth(Some("2"))
        .query(Some("f"))
        .build()
        .unwrap();
    let item_reader = SkimItemReader::default();

    dbg!(options.nth);

    let items = item_reader.of_bufread(Cursor::new(input));
    let selected_items = Skim::run_with(&options, Some(items))
        .map(|out| out.selected_items)
        .unwrap_or_else(|| Vec::new());

    for item in selected_items.iter() {
        print!("{}{}", item.output(), "\n");
    }
}
cargo run --example nth

It generates a match foo 123 although it shouldn't match anything (2nd field consists of numbers, we match on a letter). Am I doing anything wrong?

skim in terminal works as expected

> echo "foo 123" | sk --nth=1 --filter f
foo 123
> echo "foo 123" | sk --nth=2 --filter f
# no output, exit code 1
@lotabout lotabout added this to the v0.8.3 milestone Apr 8, 2020
@lotabout
Copy link
Collaborator

lotabout commented Apr 8, 2020

This issue is somehow a technical inconsistency.

  • Previously skim provides a all in one API that accepts strings. So the options could control how the lines are parsed and matched(e.g. nth).
  • Current API could receive a stream of user customized items. It allows more customization but relies on user to provide item-specific info(e.g. nth)
  • Currently SkimItemReader is only a simple wrapper that turns a BufRead into lines of string and nothing more.
  • Thus not only nth, but also with-nth, ansi and other item specific options will stop working.

As for how to solve this problem, I now realized that maybe customization of items won't be common? I'll try to provide more helpful reader helpers(e.g. better SkimItemReader)

@murlakatamenka
Copy link
Author

Thanks, I see it's a work in progress and things need time to stabilize and get shaped.

To be honest I quite struggle using skim as library once thing get a bit different from given examples.

For instance, slightly modified custom_item.rs, where we have a struct with id: u32 and inner: String, want to match just on inner but output id:

use skim::prelude::*;
use std::fmt;
struct MyItem {
    id: u32,
    inner: String,
}

impl fmt::Display for MyItem {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "{}\t{}", self.id, self.inner)
    }
}

impl SkimItem for MyItem {
    fn display(&self) -> Cow<AnsiString> {
        Cow::Owned(self.to_string().into())
    }

    fn text(&self) -> Cow<str> {
        // commented lines will produce out of bound
        // Cow::Borrowed(&self.inner)
        // Cow::Owned(self.inner.to_string())

        Cow::Owned(self.to_string())
    }
}

pub fn main() {
    let options = SkimOptionsBuilder::default().query(Some("7b")).build().unwrap();

    let vec = vec![
        MyItem {
            inner: "foo".to_string(),
            id: 123,
        },
        MyItem {
            inner: "bar".to_string(),
            id: 789,
        },
    ];

    let (tx_item, rx_item): (SkimItemSender, SkimItemReceiver) = unbounded();
    for item in vec {
        let _ = tx_item.send(Arc::new(item));
    }
    drop(tx_item); // so that skim could know when to stop waiting for more items.

    let selected_items = Skim::run_with(&options, Some(rx_item))
        .map(|out| out.selected_items)
        .unwrap_or_else(|| Vec::new());

    for item in selected_items {
        if let Some(item) = (*item).as_any().downcast_ref::<MyItem>() {
            println!("{}", item.id);
        }
    }
}

Matching on inner causes out of bound panics in threads, so I use to_string() twice just to make it work.


Actually, I found why it panics. SkimItem::get_matching_ranges() gets its range from display() rather than text(). text() is shorter than display() in the code above. I guess it's not intended to be this way.

I think the code above doesn't benefit from working with a struct instead of a just String. I'd be glad to get some feedback or if more complex custom_item.rs example is added.

@murlakatamenka
Copy link
Author

murlakatamenka commented Apr 13, 2020

I did explore how others use skim but most of them used just String + SkimItemReader::of_bufread() and matched on the whole lines.

Current workaround of how to match on needed range:

  • use the same string for text() and display()
  • override get_matching_ranges() with required range

String can be wrapped into struct that implements SkimItem trait.

But that's really a workaround, feels somewhat artificial.


Also I noticed that Skim::filter got moved into binary. Is it possible to use skim non-interactively now?

@lotabout
Copy link
Collaborator

lotabout commented Apr 14, 2020

SkimItem::get_matching_ranges() gets its range from display() rather than text(). text() is shorter than display() in the code above. I guess it's not intended to be this way

Thanks for point this out, it is a bug.

Is it possible to run use skim non-interactively now?

Yes. With --filter option.

@murlakatamenka
Copy link
Author

I'm talking about library use of Skim::filter (https://docs.rs/skim/0.8.0/skim/struct.Skim.html#method.filter), it's been moved into binary and only Skim::run_with is available now (https://docs.rs/skim/0.8.1/skim/struct.Skim.html) for calling from library.

@lotabout
Copy link
Collaborator

@murlakatamenka Misunderstood. No longer available.

@murlakatamenka
Copy link
Author

Thank you 👍

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

No branches or pull requests

2 participants