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

doc: add note to tty.WriteStream event 'resize' on Windows #13576

Closed
wants to merge 9 commits into from

Conversation

Dean-Coakley
Copy link
Contributor

Fixes: #13197

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

@nodejs-github-bot 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 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
doc/api/tty.md Outdated
process.stdout.on('resize', () => {
console.log('screen size has changed!');
console.log(`${process.stdout.columns}x${process.stdout.rows}`);
});
```

*Note:* Unrealiable event handler execution on all Windows platforms.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. UnrealiableUnreliable
  2. 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like:

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/api/tty.md Outdated
@@ -82,12 +82,15 @@ or `writeStream.rows` properties have changed. No arguments are passed to the
listener callback when called.

```js
process.stdin.setRawMode(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

doc/api/tty.md Outdated
process.stdout.on('resize', () => {
console.log('screen size has changed!');
console.log(`${process.stdout.columns}x${process.stdout.rows}`);
});
```

*Note:* Unrealiable event handler execution on all Windows platforms.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like:

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

@gibfahn
Copy link
Member

gibfahn commented Jun 9, 2017

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

doc: note 'resize' event unreliability on Windows

doc/api/tty.md Outdated
@@ -88,6 +88,8 @@ process.stdout.on('resize', () => {
});
```

*Note:* Unrealiable event handler execution on all Windows platforms.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

doc/api/tty.md Outdated
@@ -88,6 +88,8 @@ process.stdout.on('resize', () => {
});
```

*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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. UnrealiableUnreliable
  2. The Event handlerThe event handler
  3. 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.
  4. Please try to limit the line length to 80 chars by breaking it into multiple lines if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/api/tty.md Outdated
@@ -88,6 +88,10 @@ process.stdout.on('resize', () => {
});
```

*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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/api/tty.md Outdated
@@ -88,6 +88,17 @@ process.stdout.on('resize', () => {
});
```

*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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resume() -> \resume()``

adding data -> adding a data

can be also -> can also be

doc/api/tty.md Outdated
@@ -88,6 +88,17 @@ process.stdout.on('resize', () => {
});
```

*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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if terminal -> if a terminal

doc/api/tty.md Outdated
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if console -> if the console

doc/api/tty.md Outdated
*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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when new -> when the new

doc/api/tty.md Outdated
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

than current -> than the current

@bzoz
Copy link
Contributor

bzoz commented Jul 7, 2017

LGTM with @gibfahn nits

doc/api/tty.md Outdated
(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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

doc/api/tty.md Outdated
@@ -89,7 +89,7 @@ process.stdout.on('resize', () => {
```

*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
Copy link
Member

@gibfahn gibfahn Jul 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove backslash please

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot @Dean-Coakley !

@bzoz
Copy link
Contributor

bzoz commented Jul 7, 2017

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.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nits, but they need to be addressed

doc/api/tty.md Outdated
@@ -88,6 +88,17 @@ process.stdout.on('resize', () => {
});
```

*Note:* On Windows resize events will be emitted only if stdin is unpaused
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Dean-Coakley Dean-Coakley Jul 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.
Ref: #13133

doc/api/tty.md Outdated
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*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.
  1. Check that it is still true (the word also implies both conditions need to be met)
  2. See if you like it. This one is just a suggestion.

@refack
Copy link
Contributor

refack commented Jul 7, 2017

@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
Copy link
Contributor Author

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

@refack
Copy link
Contributor

refack commented Jul 8, 2017

@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 😉

refack pushed a commit to refack/node that referenced this pull request Jul 9, 2017
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
Copy link
Contributor

refack commented Jul 9, 2017

Landed in ff07eaa

@refack refack closed this Jul 9, 2017
@refack
Copy link
Contributor

refack commented Jul 9, 2017

@Dean-Coakley Thank you very much 🥇

addaleax pushed a commit that referenced this pull request Jul 11, 2017
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>
@addaleax addaleax mentioned this pull request Jul 11, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
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>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
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>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
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>
@Dean-Coakley Dean-Coakley deleted the WinTty branch March 2, 2020 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants