doc: add note to tty.WriteStream event 'resize' on Windows by Dean-Coakley · Pull Request #13576 · nodejs/node

Dean-Coakley · 2017-06-09T13:47:27Z

Unsure how specific to be on the issue. Also looking for feedback on poor commit message.

tniessen · 2017-06-09T14:12:23Z

doc/api/tty.md

Unrealiable → Unreliable

What exactly do you want to say? Please try to use whole sentences if possible. Do you mean that the execution of this event is unreliable on Windows?

How about something like:

*Note:* Terminal window resize events are unreliable on Windows.

@gibfahn As per your comment I would suggest to include a note about having to use setRawMode on Windows.

That seems much better. Is it ok as 2 commits or do I need to squash/rebase?

gibfahn · 2017-06-09T15:20:24Z

doc/api/tty.md

I think what @bzoz was saying in #13197 (comment) is that this line is needed on Windows, which implies it isn't needed on other platforms.

The code snippet without this sample works reliably in Unix, and even with this sample it's not reliable on Windows (#13197 (comment)), so I think it's probably best to just not include this (especially if this leads to people not being able to Ctrl+C their program).

That's correct. (as far as I know) Removed. I don't think it's necessary to mention in commit messages since I both added and removed it?

gibfahn · 2017-06-09T15:22:07Z

doc/api/tty.md

How about something like:

*Note:* Terminal window resize events are unreliable on Windows.

gibfahn · 2017-06-09T15:24:01Z

The commit message isn't bad, but it is over 50chars. Maybe:

doc: note 'resize' event unreliability on Windows

jasnell · 2017-06-09T17:34:17Z

doc/api/tty.md

 });
 ```

+*Note:* Unrealiable event handler execution on all Windows platforms.


This is a bit terse... can you expand this just a bit more?

I could add something like
Event handler only is only executed when the width is changed

It was also suggested here that raw mode must be enabled but I did not find this in my own testing.

tniessen · 2017-06-15T10:31:01Z

doc/api/tty.md

 });
 ```

+*Note:* Unrealiable event handler execution on all Windows platforms. The Event handler is only executed for changes in the width of the window and the console must be in raw mode.


Unrealiable → Unreliable

The Event handler → The event handler

I would prefer @gibfahn's suggestion: *Note:* Terminal window resize events are unreliable on Windows. The event handler is only... There is no reason not to use a whole sentence here.

Please try to limit the line length to 80 chars by breaking it into multiple lines if necessary.

Thanks for the review. I believe I have implemented all the above!

bzoz · 2017-06-16T06:31:01Z

doc/api/tty.md

 ```

+*Note:* Terminal window resize events are unreliable on Windows. The event 
+handler is only executed for changes in the width of the window and the 


If the console buffer height is changed, the event will also triggered. The buffer is not shrinked when downsizing widow, so it can look like changing height is ignored. If however one sets buffer height to something small in the options and then increase window height the event will be triggered.

@bzoz that seems like a lot of information, could you suggest the exact wording you want?

On Windows the console size is the virtual buffer size, not the size of the console window itself. If you shrink window height, the buffer size will remain the same, so no event will be fired. If you enlarge window enough that it will be higher than the buffer, Windows will update the buffer size and event will be signaled.

On Windows the event reliably detected only if the console is read (e.g. process.stdin.resume(); or process.stdin.on('data',...)) in raw mode.

If the console is not read, or is read in line mode, this event can still be signaled. This will happen if a terminal sequence that moves the cursor is used. Calling process.stdin._refreshSize() will also trigger the event if the console size has been changed.

One more clarification: although the event is signaled only when the buffer height is changed and not the window size, Node.js reports the console window height, not the height of the buffer itself.

@bzoz so you think the **Note:** should look like this?

Note: On Windows the console size is the virtual buffer size, not the size of the console window itself. If you shrink window height, the buffer size will remain the same, so no event will be fired. If you enlarge window enough that it will be higher than the buffer, Windows will update the buffer size and event will be signaled.

On Windows the event reliably detected only if the console is read (e.g. process.stdin.resume(); or process.stdin.on('data',...)) in raw mode.

If the console is not read, or is read in line mode, this event can still be signaled. This will happen if a terminal sequence that moves the cursor is used. Calling process.stdin._refreshSize() will also trigger the event if the console size has been changed.

Although the event is signaled only when the buffer height is changed and not the window size, Node.js reports the console window height, not the height of the buffer itself.

That seems like a massive amount of info.

Yep, I think there is too much detail in this note. It would also make ._refreshSize() somewhat documented function. I propose this:

Note: On Windows resize events will be emitted only if stdin is unpaused (by a call to resume() or by adding data listener) and in raw mode. It can be also triggered if terminal control sequence that moves the cursor is written to the screen.

Note: On Windows when changing console height the resize event will be signaled only if console screen buffer height was also changed. For example shrinking the console window height will not cause the resize event to be emitted. Increasing the console window height will only be registered when new console window height is greater than current console buffer size.

gibfahn

LGTM pending nits and a review from @bzoz (and anyone else from @nodejs/platform-windows )

gibfahn · 2017-07-07T08:46:08Z

doc/api/tty.md

 ```

+*Note:* On Windows resize events will be emitted only if stdin is unpaused 
+(by a call to resume() or by adding data listener) and in raw mode. It can be 


resume() -> \resume()``

adding data -> adding a data

can be also -> can also be

gibfahn · 2017-07-07T08:46:38Z

doc/api/tty.md


+*Note:* On Windows resize events will be emitted only if stdin is unpaused 
+(by a call to resume() or by adding data listener) and in raw mode. It can be 
+also triggered if terminal control sequence that moves the cursor is written to 


if terminal -> if a terminal

gibfahn · 2017-07-07T08:46:59Z

doc/api/tty.md

+the screen.
+
+*Note:* On Windows when changing console height the resize event will be 
+signaled only if console screen buffer height was also changed. For example 


if console -> if the console

gibfahn · 2017-07-07T08:47:14Z

doc/api/tty.md

+*Note:* On Windows when changing console height the resize event will be 
+signaled only if console screen buffer height was also changed. For example 
+shrinking the console window height will not cause the resize event to be 
+emitted. Increasing the console window height will only be registered when new 


when new -> when the new

gibfahn · 2017-07-07T08:47:21Z

doc/api/tty.md

+signaled only if console screen buffer height was also changed. For example 
+shrinking the console window height will not cause the resize event to be 
+emitted. Increasing the console window height will only be registered when new 
+console window height is greater than current console buffer size.


than current -> than the current

bzoz · 2017-07-07T09:30:46Z

LGTM with @gibfahn nits

gibfahn · 2017-07-07T14:15:43Z

doc/api/tty.md

-(by a call to resume() or by adding data listener) and in raw mode. It can be 
-also triggered if terminal control sequence that moves the cursor is written to 
-the screen.
+(by a call to \resume()`` or by adding a data listener) and in raw mode. It can 


You've changed to \resume()'', you need to have `resume()`

gibfahn · 2017-07-07T14:17:13Z

doc/api/tty.md


 *Note:* On Windows resize events will be emitted only if stdin is unpaused 
-(by a call to \resume()`` or by adding a data listener) and in raw mode. It can 
+(by a call to `\resume()` or by adding a data listener) and in raw mode. It can 


Remove backslash please

gibfahn

LGTM, thanks a lot @Dean-Coakley !

bzoz · 2017-07-07T15:46:29Z

I've opened a PR in libuv that improves the resize detection: libuv/libuv#1408. In any case this can still be landed, it will take some time before this gets landed in libuv and included in release that node will use.

refack

Small nits, but they need to be addressed

refack · 2017-07-07T21:53:04Z

doc/api/tty.md

 });
 ```

+*Note:* On Windows resize events will be emitted only if stdin is unpaused 


Tiny nit: *Note:* -> *Note*:

Hmm...I copied this mistake from async_notes.md Should that be fixed in a separate PR?

Yep.
Ref: #13133

refack · 2017-07-07T21:55:27Z

doc/api/tty.md

+also be triggered if a terminal control sequence that moves the cursor is written 
+to the screen.
+
+*Note:* On Windows when changing console height the resize event will be 


*Note:* -> *Note*:

IMHO the two paragraphs could be merged:
Also, the resize event will be signaled only if the console screen buffer height was also changed...

Sorry, but I'm not entirely sure what you want here.

Can you paste the body of text you'd like? Or be very verbose about what should be replaced.

*Note*: On Windows resize events will be emitted only if stdin is unpaused (by a call to `resume()` or by adding a data listener) and in raw mode. It can also be triggered if a terminal control sequence that moves the cursor is written to the screen. Also, the resize event will only be signaled if the console screen buffer height was also changed. For example shrinking the console window height will not cause the resize event to be emitted. Increasing the console window height will only be registered when the new console window height is greater than the current console buffer size.

Check that it is still true (the word also implies both conditions need to be met)

See if you like it. This one is just a suggestion.

refack · 2017-07-07T22:01:06Z

@Dean-Coakley It's a good comment to have, I've been digging around this issue a bit as well. So I'm sorry to block so late in the process, but since doc changes don't get functional-tested, we only have eyeballs to validate all the tiny nits.

Dean-Coakley · 2017-07-08T15:46:53Z

@refack No worries about blocking at all. Hopefully my next PR will have less nits 😄

refack · 2017-07-08T15:55:15Z

@refack No worries about blocking at all. Hopefully my next PR will have less nits 😄

Don't count on it, doc changes get a lot of comments. Code goes in smoother 😉

PR-URL: nodejs#13576 Fixes: nodejs#13197 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

refack · 2017-07-09T22:00:29Z

Landed in ff07eaa

refack · 2017-07-09T22:01:07Z

@Dean-Coakley Thank you very much 🥇

PR-URL: #13576 Fixes: #13197 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. tty Issues and PRs related to the tty subsystem. labels Jun 9, 2017

Dean-Coakley changed the title ~~doc: fix tty.WriteStream event 'resize' on Windows~~ doc: add note to tty.WriteStream event 'resize' on Windows Jun 9, 2017

tniessen reviewed Jun 9, 2017

View reviewed changes

gibfahn reviewed Jun 9, 2017

View reviewed changes

doc: note 'resize' event unreliability on Windows

6da59fe

Dean-Coakley force-pushed the WinTty branch from d3b685e to 6da59fe Compare June 9, 2017 16:25

doc: correct 'resize' event note

a45bd1d

jasnell reviewed Jun 9, 2017

View reviewed changes

doc: add more detail on note

84db75d

tniessen requested changes Jun 15, 2017

View reviewed changes

doc: correct char limit

63fa375

tniessen approved these changes Jun 15, 2017

View reviewed changes

jasnell approved these changes Jun 15, 2017

View reviewed changes

bzoz suggested changes Jun 16, 2017

View reviewed changes

doc: add more detail on note

86dfd24

gibfahn reviewed Jul 7, 2017

View reviewed changes

Dean-Coakley added 2 commits July 7, 2017 12:48

doc: fix details on note

192a56f

doc: fix err on resume note

784055f

bzoz approved these changes Jul 7, 2017

View reviewed changes

gibfahn reviewed Jul 7, 2017

View reviewed changes

doc: remove backslash

05ce78a

gibfahn approved these changes Jul 7, 2017

View reviewed changes

refack suggested changes Jul 7, 2017

View reviewed changes

doc: merge note paragraphs

5944bf0

refack approved these changes Jul 9, 2017

View reviewed changes

refack closed this Jul 9, 2017

addaleax mentioned this pull request Jul 11, 2017

v8.2.0 proposal #13744

Merged

MylesBorins added the land-on-v6.x label Aug 16, 2017

MylesBorins mentioned this pull request Aug 16, 2017

v6.11.3-proposal #14852

Merged

Dean-Coakley deleted the WinTty branch March 2, 2020 23:04

Uh oh!

Conversation

Dean-Coakley commented Jun 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Jun 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bzoz commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn left a comment

Choose a reason for hiding this comment

Uh oh!

bzoz commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dean-Coakley Jul 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn Jul 7, 2017 •

edited

Loading

bzoz commented Jul 7, 2017 •

edited

Loading

Dean-Coakley Jul 8, 2017 •

edited

Loading