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

Performing actions on keypress #49

Closed
freekmurze opened this issue Aug 1, 2017 · 15 comments · Fixed by #70
Closed

Performing actions on keypress #49

freekmurze opened this issue Aug 1, 2017 · 15 comments · Fixed by #70

Comments

@freekmurze
Copy link

freekmurze commented Aug 1, 2017

First of all, thank you for your excellent work on this. Really like the package.

I'm having trouble figuring out how actions could be performed on keypress.

This is the screen of a console app I'm building
screen shot 2017-08-01 at 23 12 20

This is the code that tries to figure out which key has been pressed.

Unfortunately it know only works by pressing the letter and then enter. I want to get rid of the required press on enter to invoke the action.

How can this be achieved?

@freekmurze
Copy link
Author

Tried a whole lot of variations to get this to work but no luck.

Latest attempt:

        $this->io->getInput()->on($eventName, function ($line) use ($callable) {
            $callable($line);

            $this->io->getReadline()->setInput('')->clear();
        });

Hope to find a solution soon, this is the last problem I need to solve before tagging a new release of my package.

@thecrypticace
Copy link

I was peeking around (because of your tweet) and came up w/ pretty much the same solution as you. But that one works for me. Does it do something it shouldn't?

@freekmurze
Copy link
Author

The thing is with that code in my previous comment, actions get executed multiple times. So pressing "a" runs the tests, pressing "a" again runs the tests twice, and so on. Can't find a good way to reset what has been typed before.

@thecrypticace
Copy link

Yeah, I noticed this but I can't figure out why. If I replace the callable with a simple echo I don't get repeated echoes or characters. Which is strange.

@thecrypticace
Copy link

Got it: $this->io->getInput()->removeAllListeners();

@thecrypticace
Copy link

That appears to work for at least the "all tests" case.

@freekmurze
Copy link
Author

Mmm, I don't understand 100% how you fixed this.

Tried:

        $this->io->getInput()->on($eventName, function ($line) use ($callable) {
            $callable(trim($line));

            $this->io->getInput()->removeAllListeners();
        });

Doesn't seem to be working...

@thecrypticace
Copy link

Oh sorry, in your Terminal.php's removeAllListeners method:

public function removeAllListeners()
{
    $this->io->removeAllListeners();
    $this->io->getInput()->removeAllListeners(); // Added this line

    return $this;
}

This appears to break input on the filter screens though. Looking into that now.

@freekmurze
Copy link
Author

Yeah, just figured it out and bumped on the breaking filter screens. Thanks for figuring this out together with me :-)

@thecrypticace
Copy link

👍

@freekmurze
Copy link
Author

freekmurze commented Aug 1, 2017

Got it!

https://github.com/spatie/phpunit-watcher/blob/f954fd2/src/Terminal.php#L25-L38

I'm thinking removeAllListeners removed some internal listeners as well. Solved it by using once.

Seems hacky, but it works.

@clue
Copy link
Owner

clue commented Aug 2, 2017

I'm glad you've got this sorted out already! Let's recap some of the nifty details here.

The following code will invoke the callback whenever data arrives on STDIN:

$stdio->getInput()->on('data', function ($data) {
    var_dump($data);
});

If you only want to execute this callback once for when the next input arrives, you should indeed use once() instead:

$stdio->getInput()->once('data', function ($data) {
    var_dump($data);
});

Both should work for the use case you've mentioned above, but note they actually work on raw data received from STDIN, which may expose more "data" than you may be aware of. This event will be emitted whenever new data on the STDIN stream is ready, which may not only include "user key presses".

In particular, special keys such as cursors or Fn will actually emit binary terminal codes that consist of multiple bytes (which may in turn contain ASCII bytes). Similarly, pressing non-ASCII keys such as ä will emit a single event with multiple bytes. Additionally, copy-pasting things into the terminal will emit a single event with multiple bytes.

To make things worse, keep in mind that the loop will process data as soon as it (is aware that it) arrives. This means that if you either block the loop or copy-paste big inputs, you may actually receive data in chunks which may contain multiple bytes. Additionally, these chunks can potentially be split at byte ranges so that a single UTF-8 characters can be broken into multiple chunks with incomplete UTF-8 sequences.

All of these are probably not an issue for your particular use case, so you should be able to consume this stream just fine. But these combined are the reason why the Readline class does all the heavy lifting internally and only exposes the data when the user presses ENTER.

I'm not opposed to exposing things through an easier API, but I'd love to hear some more thoughts on this first! Thanks for sparking this discussion, which I think is a very good step towards this API! 👍

@freekmurze
Copy link
Author

I'm not a big user of ReactPHP so I don't know what a good API for most users might be, this is just my two little cents with only my specific problem in mind.

For my package it would have been nice that a $stdio->onKeypress($callable) function would have been present.

@clue clue added this to the v2.1.0 milestone Dec 17, 2017
@clue
Copy link
Owner

clue commented Dec 17, 2017

Thanks for all your input so far!

Note that the above code will have to be deprecated via #62 for the upcoming v1.2.0 release. This is required in preparation for the upcoming v2.0.0 release (#50).

A proper way to register custom functions to key events has been worked out already and will be added again with the v2.1.0 release that will be released shortly thereafter.

This means that you can keep relying on the above behavior with the old version for now and I will update this ticket again for the v2.1.0 release 👍

@clue
Copy link
Owner

clue commented Jan 25, 2018

I've just filed #70 to add this for the upcoming release :shipit:

I would love some feedback, so it would be much appreciated if anybody happens to have a chance to check this out before merging.

@clue clue closed this as completed in #70 Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants