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

"resize" event not working? #54

Closed
laurent22 opened this issue Oct 4, 2017 · 18 comments
Closed

"resize" event not working? #54

laurent22 opened this issue Oct 4, 2017 · 18 comments

Comments

@laurent22
Copy link
Contributor

I'm trying to get the resize event to work, or simply to get the new terminal height whenever it's resized, but it's not working so far.

I've narrowed it down to this simple script:

var term = require( 'terminal-kit' ).terminal ;

setInterval(function() {
	console.info(term.height);
}, 1000);

term.on('resize', function(width, height) {
	console.info('Resize: ' + width + ', ' + height);
});

This will continuously print the same size, even as I resize the terminal, and the "resize" event is never fired.

If I Ctrl+C the script and start it again, it will then pick up the new terminal size, so it's getting it right but it just never updates once the script starts.

I've tried in ConEmu and the default Windows terminal, both under WSL and DOS but none of these are working. Any idea what could be the issue?

@cronvel
Copy link
Owner

cronvel commented Oct 5, 2017

@laurent22 Can you test it with those events:

  • process.stdout.on( 'resize' , callback )
  • process.on( 'SIGWINCH' , callback )

Terminal-kit use Node for that, and I don't know if Windows reports terminal resizing at all.

Please, can you check the process.stdout.isTTY value.

@cronvel
Copy link
Owner

cronvel commented Oct 5, 2017

Apparently, this is a known issues in the node.js repository .

@laurent22
Copy link
Contributor Author

laurent22 commented Oct 5, 2017

process.on( 'SIGWINCH' , callback )

Seems that's the key. If I add an event listener for this, then the rest of it suddenly works. i.e. this doesn't work:

var term = require( 'terminal-kit' ).terminal ;

console.info('process.stdout.isTTY = ' + process.stdout.isTTY);

setInterval(function() {
	console.info(term.height);
}, 1000);

term.on('resize', function(width, height) {
	console.info('Resize: ' + width + ', ' + height);
});

process.stdout.on('resize', function() {
	console.info('Resize (process.stdout)');
});

However if I add this at the bottom:

process.on('SIGWINCH', function() {
	console.info('Resize (SIGWINCH)');
});

Then this event handler and the two previous ones also start working, so it seems adding this triggers something that makes terminal-kit (and node) detect resize events correctly.

process.stdout.isTTY is true on my machine.

Maybe the solution would be to enable the SIGWINCH method if the operating system is Windows, regardless of isTTY?

@cronvel
Copy link
Owner

cronvel commented Oct 5, 2017

@laurent22 Wow, what an incredibly awkward behavior... -_-'
Was that tested inside WSL?

@laurent22
Copy link
Contributor Author

Yes that was inside WSL, from ConEmu (in tmux if it makes a difference).

@cronvel
Copy link
Owner

cronvel commented Oct 6, 2017

@laurent22 Does this fix this issue?

@laurent22
Copy link
Contributor Author

Unfortunately it still shows the same problem. I've tried running the file all-event-test.js and it didn't detect the resize event. However again if I add this line at the bottom: process.on('SIGWINCH', function() {}); then the call term.on( 'resize'... magically starts working.

@cronvel
Copy link
Owner

cronvel commented Oct 6, 2017

@laurent22 Argh... what is the value of process.platform?

@laurent22
Copy link
Contributor Author

Oh right I see you have a check on process.platform, but on Windows WSL it actually returns "linux". If needed there's a package here that can detect WSL: https://github.com/ianmitchell/is-windows-bash

@refack
Copy link

refack commented Oct 13, 2017

Can anyone check if this reproduces with [email protected]?

@cronvel
Copy link
Owner

cronvel commented Oct 14, 2017

@laurent22 ?

@laurent22
Copy link
Contributor Author

@refack, yes it's still happening in node 8.7.0. This doesn't work;

setInterval(function() {}, 1000);

process.stdout.on('resize', function() {
	// Never fires
	console.info('Resize (process.stdout)');
});

But with the empty process.on('SIGWINCH', function() {}); at the bottom it does:

setInterval(function() {}, 1000);

process.stdout.on('resize', function() {
	console.info('Resize (process.stdout)'); // It works
});

process.on('SIGWINCH', function() {});

@refack
Copy link

refack commented Oct 19, 2017

@laurent22 thanks!
Now waiting for nodejs/node#16225

@laurent22
Copy link
Contributor Author

Brilliant, thanks for the update!

@cronvel
Copy link
Owner

cronvel commented Oct 20, 2017

Great!
That reminds me that I definitively need a Windows maintainer...

@laurent22
Copy link
Contributor Author

I can help with that if needed. I'm using Terminal Kit quite a lot in a note-taking app I'm working on at the moment (it's here but not officially released yet) and I'm only using Windows. For the most part I didn't have any issue actually other than the resize issue, which is more a Node regression.

@refack
Copy link

refack commented Oct 20, 2017

If you need Windows specific review, feel free to ping me as well.

@cronvel
Copy link
Owner

cronvel commented Oct 20, 2017

@laurent22 @refack Thanks a lot, it will be really useful. I don't have Windows anymore and I never used it for dev, so I terribly lack experience. And there are many specific environments like Cygwin and WSL, or various terminals more or less compliant (putty, console2, etc).
I realize that there are more Node.js developers working on this platform than I could imagine, and I would be happy if they could fully enjoy the lib ;)

@cronvel cronvel closed this as completed Mar 27, 2018
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

3 participants