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

Command API experiment #175

Merged
merged 19 commits into from
Jul 24, 2019
Merged

Command API experiment #175

merged 19 commits into from
Jul 24, 2019

Conversation

TimonPost
Copy link
Member

@TimonPost TimonPost commented Jul 7, 2019

This pull request is an experiment with issue: #171, it introduces a macro which writes commands onto the given Write type. The use can manually call flush to clear the buffer. By using this API, we can improve the performance bottleneck we are currently experimenting because there is a lot of flushing going on.

fn main() {
    let mut stdout = ::std::io::stdout();

    for x in 0..30 {
        for y in 0..30 {
            schedule!(stdout,Goto(x, y), Output(String::from("#")));
        }
    }

    thread::sleep(Duration::from_millis(3000));
    stdout.flush();
}
  • Write documentation in book
  • Write example: /examples/command.rs
  • Implement the command below
✔ Goto
✔UP
✔Down
✔Left
✔Right
✔SavePos
✔ResetPos
✔Hide
✔Show
✔Blink

✔ SetFg
✔ SetBg
✔ SetAttr
✔ Print Styled Text

❌ Reset
✔ Clear
✔ Scroll Up
✔ Scroll Down
❌ SetSize

- [ ] Make sure all commands are documented

Please checkout `crossterm_cursor/example/cursor.rs` for the actual implementation

@TimonPost TimonPost changed the title WIP: Initialize command API, macro, goto, output commands WIP: Command API experiment Jul 7, 2019
@TimonPost TimonPost mentioned this pull request Jul 7, 2019
2 tasks
@zesterer
Copy link

zesterer commented Jul 11, 2019

Hi. As someone looking to use this library, I'd be quite saddened if macros were introduced into the public API for this. In my humble opinion, macros should be used specifically to extend Rust's syntax, not for something as trivial as variadic functions. May I suggest using a builder pattern instead?

write_chain(stdout)
    .and(Goto(x, y))
    .and(Output(String::from("#")))
    .finish();

In my opinion, this is far more intuitive and doesn't undermine the rich semantics and visual guarantees that make Rust such a pleasant language to work with.

@TimonPost
Copy link
Member Author

Thanks for your input. Indeed macro's should be used for cases where they are meant for. Although macros are also super powerful in rust, and it is a feature a lot of people use. Also in public API.

So we either do:

write_chain(stdout)
    .and(Goto(x, y))
    .and(Output(String::from("#")))
    .finish();

Or

schedule!(stdout, Goto(x, y), Hide, Output("#"));

I have several problems with the builder idea

  1. If we do the builder approach we have to queue commands into some vector, store them as traits, which will result in the dynamic dispatch.
  2. We can't less easily move the chain builder around. It needs to be a mutable type, because of the add method. When working in a multithreaded application we are probably going to get mutexes and a lot of arcs around this type.
  3. Boilerplate code
    We can choose to create a new chain every time we want to queue another command. As a workaround of point 2, this would look like:
write_chain(stdout)
    .and(Goto(x, y));
// some code
write_chain(stdout)
    .and(Goto(x, y));
// some code
write_chain(stdout)
    .and(Goto(x, y))
   finishe();

This would work way better with the macro where we do:

 schedule!(stdout, Goto(x, y);
// some code
 schedule!(stdout, Goto(x, y);
// some code
 schedule!(stdout, Goto(x, y);
stdout.flush()
  1. In most cases, the user only wants to use the default stdout.
    With the builder solution, we would have to create an other function that creates the builder which would have a default argument with std::io::stdout().
default_write_chain(stdout)
    .and(Goto(x, y));

And with macro's we can just leave a way that argument:

 schedule!(Goto(x, y);

So all with all, I still prefer the macro functions,

  1. they can have different arguments and are more flexible,
  2. are stateless, and thus no mutexes or atomic types are needed,
  3. less boilerplate code

@zesterer
Copy link

To address your concerns:

  1. There is absolutely no need to do dynamic dispatch, in the same way that Rust's iterator API doesn't do dynamic dispatch. Encoding this stuff at the type level with a builder pattern is quite trivial.

  2. Why not? Iterators can be moved around, and this would have a similar internal representation

  3. Given the circumstance you mention, I suggest a better API:

A trait implemented for all Write types, that looks permits this:

stdout
    .queue(Goto(x, y))
    .queue('!')
    .queue(Goto(a, b))
    .flush();

In the single case, it's just a matter of doing stdout.queue(Goto(x, y)) which keeps the API very simple.

  1. You seem to be forgetting that macros come with a lot of disadvantages. I'm sure as the author you do not see these disadvantages, but they tend to be an indicator of 'magic' code. When working with macros, it's hard to predict their behaviour and Rust developers have been conditioned to abandon existing rules about how code runs when seeing a macro. For this reason, I'd really advise avoiding them.

@TimonPost
Copy link
Member Author

I'll take your concern into consideration, I will make a test that works with the type system as well.

@TimonPost
Copy link
Member Author

TimonPost commented Jul 15, 2019

I currently implemented via the function as well:

We have two traits:

pub trait QueueableCommand<T: Display> {
    fn queue(mut self, command: impl Command<AnsiType = T>) -> Self;
}

pub trait ExecutableCommand<T: Display> {
    fn execute(mut self, command: impl Command<AnsiType = T>) -> Self;
}

I implement those traits for all types implementing Write

impl<T, A> QueueableCommand<A> for T
where
    A: Display,
    T: Write,
{
    fn queue(mut self, command: impl Command<AnsiType = A>) -> Self {
        schedule!(self, command);
        self
    }
}

impl<T, A> ExecutableCommand<A> for T
where
    A: Display,
    T: Write,
{
    fn execute(mut self, command: impl Command<AnsiType = A>) -> Self {
        execute!(self, command);
        self
    }
}

Now I am able to call the queue and executing functionality on Stdout:

 let mut stdout = ::std::io::stdout();

    stdout
        .queue(Goto(5, 5))
        .queue(Output("#".to_string()))
        .flush();

   stdout.execute(Goto(4,5));

And it works also :)

@TimonPost
Copy link
Member Author

TimonPost commented Jul 15, 2019

We have two options:

  1. Keep the macros for users that want to have less boilerplate code and don't bother working with macros while also keeping the easier option.
  2. Remove the macro's and only have support for the builder functions.

macros

// writing on default stdout
schedule!(Goto(x, y), Output("#"));
::std::io::flush();

// writing on a custom type that implements `Write`
schedule!(my_type, Goto(x, y), Output("#"));

Builder:

// writing on default stdout
 ::std::io::stdout()
     .queue(Goto(5, 5))
     .queue(Output("#".to_string()))
     .flush();

// writing on a custom type that implements `Write`
my_type
     .queue(Goto(5, 5))
     .queue(Output("#".to_string()))
     .flush();

@zesterer
Copy link

Nice job! I much prefer this design, thanks for working on it.

@TimonPost TimonPost changed the title WIP: Command API experiment Command API experiment Jul 24, 2019
@TimonPost
Copy link
Member Author

Will be merged now, documentation, examples, and comments are all finished. I implemented this branch for both TUI and cursive, to test the functionality. Will be released soon under version 10

@TimonPost TimonPost merged commit 1a60924 into master Jul 24, 2019
@TimonPost TimonPost deleted the command_api branch September 16, 2019 09:59
december1981 pushed a commit to december1981/crossterm that referenced this pull request Oct 26, 2023
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