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

Input Expansion #81

Closed
4 tasks done
TimonPost opened this issue Jan 18, 2019 · 16 comments
Closed
4 tasks done

Input Expansion #81

TimonPost opened this issue Jan 18, 2019 · 16 comments
Milestone

Comments

@TimonPost
Copy link
Member

TimonPost commented Jan 18, 2019

This is a tracking issue for the work what still needs to be done for reading input.

  • Reading Keys Presses
    This feature should porovide support for the following Key Events
    - winapi example code.
    - Special key combinations
  • Mouse Events (example)
    - Allow to capture mouse down, up and hold events.
    - Allow seeing which mouse button is pressed.
  • cleaning up old threads
  • Testing
@imdaveho
Copy link
Contributor

imdaveho commented Jan 29, 2019

I started down this road as well; I was approaching it by turning the winapi objects (keyevent and mouseevent) into vt sequences so that only the windows portion needs to be compiled on windows machines, but still run through unix-y csi handling to turn keyboard or mouse input events into a common "Event" enum similar to termion.

I decided on this approach because windows is providing support for vt seqs in future cmd and powershell updates, so this wrapper would just be for backwards compatibility...so I didn't want to overcomplicate the codebase by having to maintain 2 separate input handling logic if eventually most new windows systems would simply be handling vt just fine...what do you think?

@TimonPost
Copy link
Member Author

Thanks for looking into this, as you might have seen I've moved the input logic to crossterm_input, I'm very curious to see your solution! I saw the termion implementation as well and kind of wanted to have the same API.

I prefer to have the WinApi code as well, maybe it is 400 more lines of code, but this is the goal of crossterm, that it supports all platforms even Windows 7 and 8. I've had a few people creating issues for those systems so there are still people using it. Also, all the other code currently has a WinApi + ANIS implementation and because of that, it seems a little odd to me to not support keybindings for those systems.

So yes I still prefer having them both. If you have other problems or questions feel free to ask them. There are still some things that might be unclear in the code base. And some things that could be definitely done better. Also, if you got discord, let me know, this might be easier to communicate over some things.

@TimonPost
Copy link
Member Author

Any updates on this? If you are not working on this I will give it a try.

@imdaveho
Copy link
Contributor

imdaveho commented Feb 3, 2019 via email

@TimonPost
Copy link
Member Author

Nice, yes, sure, no worries. Just making sure if there is progress.

@imdaveho
Copy link
Contributor

imdaveho commented Feb 5, 2019

https://github.com/imdaveho/crossterm/tree/pr-upstream--add-input-events/crossterm_input/src/input

Hey @TimonPost, I realize that there is actually quite a lot of work, and with the recent 0.6.* release, I actually get a lot of errors trying to run examples...I think it has to do with the folder placement.

The work involved:

  • set and reset terminal modes (eg. windows mouse input and input modes; *nix input modes) per Screen instance
  • define and handle event types we want to support
  • handle mouse support
  • properly Drop a thread once done per Screen instance
  • test, test, test! w/ examples

I'm also not too sure the direction you want to take this library (given how you modularized it and re-organized the folder structure)

So I'm going to leave this code(most likely broken; definitely incomplete) as-is right now as reference. IF you want me to make a PR just to look at diffs LMK; else most of the changes are to /crossterm_input.

My goal here is to effectively write a translation layer from windows consoleapi into VT sequences so that everything gets handled the same way. See attached diagram:

windows console handling for crossterm

Sorry that the code is a bit of a mess though...but I wanted to show progress and see how to divide and conquer once I realized that the work involved is a lot more than I had previously thought...

@TimonPost
Copy link
Member Author

TimonPost commented Feb 5, 2019

This looks very interesting, I'll come back with a bigger story later if you don't mind, due to the fact I'm not at my PC now. I am more than happy by helping out solving the errors and making clear the direction of this crate, notice that I already created milestones and issues. I've got some plans to improve it even more, but some refactoring cannot be done until this is implemented. But I'll come back at this, great work!

@TimonPost
Copy link
Member Author

TimonPost commented Feb 6, 2019

Okay

1. The direction of this crate

  • I made some changes to the structure of crossterm, please check out the release notes of what I changed.
  • The crossterm book describes how the introduced feature flags are working.
  • All modules e.g. input, style, terminal, screen, cursor have been moved into their own crate. This allows uses to use only a specific functionality instead of all those modules.
    - Crossterm is a kind of a wrapper for all those modules and you are allowed you pick the features you want with feature flags; as explained in the book.

2. Error in the examples
First, the async_input example was doing: cursor moving, switching to alternate modes, enabling raw modes, clearing of the screen. But because those functionalities were moved to their own crate as described above at 1 and crossterm_input does not have any reference to those crates.

Because of this, we can't use those crates in our demonstration. I had two choices: 1, leave the example code and leave a note for enabling the raw mode, 2, move the async_input over to crossterm examples where we do have access to those modules.

The input example imports the Screen type, however, this could be removed since it is not used; I forgot to do that.

3. Your idea
I really like your proposal, but to make things clear you want to translate ANSI-escape codes to WinApi actions?

4. Other
There are two options and you have to make a choice here. 1) I can help you out fixing this code, I fix the examples and I will work with you on your branch. 2) you said it is to much work, I am willing to take the work over and moving on with this idea only thing I am concerned about is you invested your time and you won't get acknowledged for your work as a contributor to this crate within GitHub. So it is up to you to decide whether you want to continue or not.

@imdaveho
Copy link
Contributor

imdaveho commented Feb 6, 2019

@TimonPost

3. I want to translate the traditional Windows Console API into VT sequences such that the parsing for crossterm Events can go through a common, platform independent layer eg. parse_event() that consumes a buffer/stream of VT sequences. This is because Windows has already conceded that VT sequences are what most developers are used to, and have made updates to cmd and PowerShell to emit VT sequences instead of using ConsoleAPI objects. src

4. Because there is already a direction you're heading with crossterm; I don't want to make assumptions and design decisions that might conflict with what you want to accomplish with this library. I agree with most of the high level ideas you have, like making everything modular. Although I think some of those modules will most often be used together anyway and modularizing might not be the highest thing on my priority list.

Also, in my exploration of input handling, I also realized that the work was starting to spill over to other considerations:

  • set and reset terminal modes (like what I mentioned in another issue)
  • clean up created threads
  • provide a different user interface to receive Event enums and handle them vs. read_async()

These would seem to overlap with your design vision, and therefore, I'd rather defer to you to integrate my work into how you want to proceed, taking into consideration my idea of standardizing input handling across windows and *nix systems.

I'm more than willing to collaborate and contribute; but in this case, there are a few key pieces of functionality that I think would be more efficient if you saw it all the way through.

Note: I am interested in getting this library to a v1 and beyond. I don't mind not being listed as contributor in this case, because I'm pretty sure I'll contribute down the line. How can I find you on Discord anyway?

Note 2: I guess if you could give guidance on the bullets above regarding thread and mode handling, I can then merge in my input handling code for keyboard and mouse, no problem.

1. and 2. Yes, I think I'll have to dive deeper into the docs and review the release notes more closely. Eventually, I'll be more than happy to add to documentation and user guides/tutorials.

@TimonPost
Copy link
Member Author

TimonPost commented Feb 6, 2019

  1. Yes, some modules might be used together indeed. A perfect example of this input, async input needs raw mode to be enabled.

Why did I split up everything? There were a couple of reasons:

  1. First, there were some dependencies trough all the modules who could be removed. By splitting up everything allowed me to remove most of the deps who where not necessary.
  2. User where requesting that they only wanted to use one module and don't wanted to deal with all the other modules. With feature flags you can reduce the size of crossterm and you won't depend on stuff you don't have to.
  3. It allows me to implement and release specific changes without releasing crossterm itself.

Jup, I encountered the same problems you just listed. And with a little investigation, it is possible to do it right.

Vision Continued
There is currently a type TerminalOutput I want to get rit of this type completely. I can explain in discord if you want to know why I introduced it, why it is still needed in some cases and why it is a pain in the head now.

Fortunately, due to my API-abstraction, I can change the whole back end (UNIX and Windows implementations) without changing anything to the API.

Next, to that, I want something like this possible and I want to support input and mouse, key bindings like termion. As concerning to the input, I have no problems you making any API-changes. I don't have any major other plans for now, so I won't break your code again :).

You can find my discord username here: https://daposto.stackstorage.com/s/FpVS06zCd1UAePx. So that we can communicate more easily. Here we can continue discussing the things who are not clear to you yet.

@prabirshrestha
Copy link
Contributor

I'm also very interested in this. I'm very new to rust so don't have much idea about this, but I have found xi-term's implementation very clean. https://github.com/xi-frontend/xi-term/blob/9038e2493d59890dad59887a0a167796edc9a1ea/src/core/terminal.rs

Based on xi-term and termion I had come up with this generic api.

use std::sync::mpsc::{self, Receiver, Sender};
use termion::event::Event;

pub type RenderTarget = MouseTerminal<AlternateScreen<RawTerminal<Stdout>>>;

pub struct Terminal {
    stdout: RenderTarget,
    pub terminal_events: Receiver<TerminalEvent>,
}

pub enum TerminalEvent {
    Resize(Result<(u16, u16), io::Error>), // (width, height)
    Input(Result<Event, io::Error>),
}

impl Terminal {
    // some helper funcs. omitting constructors

    pub fn clear(&mut self) -> Result<(), io::Error> {
        write!(self.stdout, "{}", clear::All)
    }

    pub fn goto(&mut self, x: u16, y: u16) -> Result<(), io::Error> {
        write!(self.stdout, "{}", cursor::Goto(x, y))
    }

    pub fn hide_cursor(&mut self) -> Result<(), io::Error> {
        write!(self.stdout, "{}", cursor::Hide)
    }

    pub fn show_cursor(&mut self) -> Result<(), io::Error> {
        write!(self.stdout, "{}", cursor::Show)
    }
}

impl Write for Terminal {
    fn write(&mut self, data: &[u8]) -> io::Result<usize> {
        self.stdout.write(data)
    }

    fn flush(&mut self) -> io::Result<()> {
        self.stdout.flush()
    }
}

Using it is also very clean.

fn process_terminal_events(&mut self) -> Result<bool, io::Error> {
    for term_evt in self.terminal.terminal_events.try_recv() {
        match term_evt {
            TerminalEvent::Resize(Ok((col, row))) => {
                self.resize_editor(col, row)?;
                return Ok(true);
            }
            TerminalEvent::Input(Ok(event)) => match event {
                Event::Key(key) => return Ok(self.process_key_press(key)?),
                Event::Mouse(me) => match me {
                    MouseEvent::Press(_, a, b)
                    | MouseEvent::Release(a, b)
                    | MouseEvent::Hold(a, b) => {
                        self.terminal.goto(a, b)?;
                        self.terminal.flush()?;
                    }
                },
                _ => {}
            },
            _ => {}
        }
    }

    Ok(true)
}

Theoretically you could also make the event be just u8 and tell the user to call parse event manually.

With async/await and mio support it could be pretty generic.

@Canop
Copy link
Contributor

Canop commented Mar 26, 2019

The discussion here is wide. Perhaps should we have more specific tracking issues ?
(I'm for example especially interested in parsing key presses and I can create the issue if you want)

@TimonPost
Copy link
Member Author

TimonPost commented Mar 27, 2019

Yea it is unfortunately, I updated this issue since this already a tracking issue.

Currently there is a branch input_handling with a working implementation. It is already working and almost ready for release, only windows is having some problems with reading the key input, I am investigating that and doing some refactoring and cleaning up.

This branch handles key, mouse special key inputs.

@Canop
Copy link
Contributor

Canop commented Mar 28, 2019

There doesn't seem to be a "synchronous" implementation of event parsing in the input_handling branch, am I right ? I was expecting something like

trait ITerminalInput {
    fn read_event(&self) -> Result<InputEvent, Error>;

Are users expected to wrap read_char into an iterator for parse_event themselves ?

Is there a chat-like place where you'd be willing to discuss tests, implementation, etc. on this ?

@TimonPost
Copy link
Member Author

Please come over to the discord server I just created: https://discord.gg/K4nyTDB.

@TimonPost
Copy link
Member Author

fixed in #111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants