alanvardy / tod

An unofficial Todoist command line client written in Rust
MIT License
104 stars 9 forks source link

Dynamically adjust list height based on terminal size #751

Closed stacksjb closed 5 months ago

stacksjb commented 6 months ago

If terminal size is smaller than the number of projects or tasks listed, it is truncated, so you have to scroll your terminal window in order to view your inquire scrolling - though the inquire scroll could simply be truncated and scrolled within.

This can be adjusted with the page_size parameter and can be detected with the term_size crate (https://crates.io/crates/term_size), example code like so:


     // Determine the terminal height and set the page size accordingly
        let page_size = if let Some((_, height)) = term_size::dimensions() {
            height as usize - 2 // Leave some space for the prompt
        } else {
            10 // Fallback page size
        };
alanvardy commented 6 months ago

Very cool, thanks

alanvardy commented 5 months ago

PR done

stacksjb commented 5 months ago

I think this might need to be reverted/removed. I'm seeing some really weird behavior with lists that exceed the window size that i wasn't before. Might have something to do with the buffer not being cleared (I've seen similar issues before with other code) but this was not an issue prior - are you switching terminal input modes between raw and line? Usually I've seen this issue when the terminal input isn't switched or cleared.

If I have a list that massively exceeds the window size, it doesn't work correctly at all: 1) While the first line item is highlighted, it places a cursor halfway down the screen 2) If I try to scroll, I get massive chunks of blank/whitespace and it doesn't go linearly.

Notice in below image (only visible if I expand my terminal window), it shows the top one highlighted, but puts my cursor a full screen down towards the bottom

image

stacksjb commented 5 months ago

image The above is after trying to scroll twice. You can see it replicates the entry and leaves large black chunks. This is an unedited screenshot, exactly as it appears.

stacksjb commented 5 months ago

Really weird issues testing this, the way it is working in TOD is not consistent with the way my test app works.

stacksjb commented 5 months ago

I'm not sure what is going on here, some weird issue with the way this app is generating the lists.

By default, the page_size for inquire is 7. It doesn't do any scaling up to larger or smaller displays.

I can manually set that to a larger number, like so: } else { Select::new(desc, options) .with_page_size(16) .prompt() .map_err(Error::from) } And as long as that number doesn't exceed display window height, my options scroll just fine. However, if it exceeds the window height, it breaks the scrolling

stacksjb commented 5 months ago

Ok, further testing seems to show that for some reason the actual terminal size being printed is twice what it should be. If I manually (externally/on the terminal window, not within the terminal) scroll up, when displaying menus that are larger than the terminal size, I noticed the actual menu size was always double the window size (i.e. it printed twice as much above as it should have). I can easily reproduce this by simply having a list of projects or options that is greater than the window size.

This code fixes the issue and works consistently - I'm just dividing it in half before returning value.

       .with_page_size(get_terminal_height() as usize)

And in my get_terminal_height

 fn get_terminal_height() -> u16 {
    let (_, height) = terminal_size().unwrap();
    (height / 2) - 1
}

Do you maybe have logic somewhere already calculating height or options or somewhere else? Not sure why as in my example code the base height works, but with this function the exact term height returns double.

I also tested with both the termion::terminal_size and the terminal_size crates, and both caused the same results, so it doesnt' appear to be tied to the crate in use.

stacksjb commented 5 months ago

Going back to the code as it is in the repo, this single modification on line 116 makes it work consistently (although the 3 is probably more than we need, we only need 1-2 depending on how many lines are being printed prior to the inquire)

Modify line 116 of input.rs so it matches as follows

/// Select an input from a list
pub fn select<T: Display>(
    desc: &str,
    options: Vec<T>,
    mock_select: Option<usize>,
) -> Result<T, Error> {
    if cfg!(test) {
        if let Some(index) = mock_select {
            Ok(options
                .into_iter()
                .nth(index)
                .expect("Must provide a vector of options"))
        } else {
            panic!("Must set mock_select in config")
        }
    } else {
        Select::new(desc, options)
            .with_page_size(page_size() / 2)
            .prompt()
            .map_err(Error::from)
    }
}

/// Gets the desired number of visible options for select menu
fn page_size() -> usize {
    match terminal_size() {
        Some((Width(_), Height(height))) if height >= 6 => (height - 3).into(),
        // We don't want less than 3 options
        Some(_) => 3,
        None => 7,
    }
}

I have no idea why it's twice what it should be as this doesn't match code in my other app, but it is working consistently after making this tweak, regardless of terminal size (I can resize it any size and it works as expected).

stacksjb commented 5 months ago

Above tweak is committed on the page_size_hack branch and working for me

alanvardy commented 5 months ago

Crazy, I tried it on a really small window but didn't use a huge list. Open a PR!

stacksjb commented 5 months ago

Thx. Still baffles me why this is required because I tested this in my other CLI app with no issues. I will debug and test and get a PR submitted if it matches what I expect

alanvardy commented 5 months ago

Addressed in this PR: https://github.com/alanvardy/tod/pull/805