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

Implement stdout columns and rows #6520

Merged
merged 16 commits into from Jul 10, 2020
Merged

Conversation

@sebastienfilion
Copy link
Contributor

sebastienfilion commented Jun 27, 2020

To be able to build rich CLI tools, it is very useful to be able to calculate the terminal's number of columns and rows.
Although we could use tput through Deno.run, this approach is a bit slow and convoluted.

I know there was already an attempt to solve this issue but it seems to have died out.

This PR showcases two approaches:

  1. Exposing ioctl winsize
const { columns, rows } await Deno.getConsoleSize(Deno.stdout.rid);
... 
const { innerHeight, innerWidth } await Deno.getConsoleSize(Deno.stdout.rid);

2. Adding properties to STDOUT like in Node.JS

const { columns, rows } = Deno.stdout; 

Note: The PR was only tested on a UNIX system.

Resolves: #3456
References: #3790

@sebastienfilion sebastienfilion force-pushed the sebastienfilion:feature/ioctl-winsize branch 2 times, most recently from 8acbbf2 to dbd8294 Jun 27, 2020
cli/js/ops/ioctl.ts Outdated Show resolved Hide resolved
cli/ops/ioctl.rs Outdated Show resolved Hide resolved
Copy link
Contributor

caspervonb left a comment

This is a good start but it needs more work.

  • A tty op shouldn't go on the file interface
  • This should probably be put behind unstable
  • The op should be available on windows, return an error.
cli/js/files.ts Outdated Show resolved Hide resolved
cli/ops/ioctl.rs Outdated Show resolved Hide resolved
cli/js/deno.ts Outdated Show resolved Hide resolved
@sebastienfilion
Copy link
Contributor Author

sebastienfilion commented Jun 27, 2020

@caspervonb Thank you very much for all your feedback! I was expecting some of your comments but with your guidance, I will be more effective!
I think I have the snippet to support windows, I will try to find a way to test it.

As I mentioned in the description of the PR, I've already implemented it on the global namespace as:

const [ columns, rows ] = await Deno.winsize(Deno.stdout.rid);

So, you think that getConsoleSize is a better name; Might be better than getWindowSize (as in clib) since it could be confused with a browser window...

@sebastienfilion sebastienfilion force-pushed the sebastienfilion:feature/ioctl-winsize branch 3 times, most recently from 2bad7d4 to 7ac7605 Jun 28, 2020
@sebastienfilion
Copy link
Contributor Author

sebastienfilion commented Jun 28, 2020

@caspervonb I made the changes that you requested.
For now, I'm throwing OpError::not_implemented() for Windows... I have an old Surface somewhere to test an implementation... But I think this can be another PR...

I'd like your opinion on a couple of things:

  1. I added a documentation block along with my function declaration declare namespace Deno...; Can you think of another place where I should be documenting this function?
  2. Would you write any unit test for this function?
@sebastienfilion sebastienfilion marked this pull request as ready for review Jun 28, 2020
Copy link
Contributor

caspervonb left a comment

Missing tests and a few nits.

cli/ops/tty.rs Outdated Show resolved Hide resolved
cli/ops/tty.rs Outdated Show resolved Hide resolved
cli/ops/tty.rs Outdated Show resolved Hide resolved
@sebastienfilion
Copy link
Contributor Author

sebastienfilion commented Jun 28, 2020

Missing tests and a few nits.

I asked in a previous comment about what tests you would implement; aside from testing that the method is available on the Deno namespace...
I don't know where to get started if we need to mock a console 😆

@caspervonb
Copy link
Contributor

caspervonb commented Jun 28, 2020

I asked in a previous comment about what tests you would implement;

Think we can at-least assert that the values are >= 0 without breaking out virtual tty.

@sebastienfilion sebastienfilion force-pushed the sebastienfilion:feature/ioctl-winsize branch 3 times, most recently from d5c8303 to 0f1d44b Jun 28, 2020
cli/ops/tty.rs Show resolved Hide resolved
@ry
Copy link
Member

ry commented Jul 2, 2020

Hi @sebastienfilion - nice patch!

I agree with @caspervonb that it still needs some work, but this is close to something we can land.

@sebastienfilion sebastienfilion force-pushed the sebastienfilion:feature/ioctl-winsize branch 3 times, most recently from 0f6ce74 to f7a09f3 Jul 6, 2020
@sebastienfilion sebastienfilion changed the title Implement ioctl winsize as a new binding Implement stdout columns and rows Jul 6, 2020
@sebastienfilion
Copy link
Contributor Author

sebastienfilion commented Jul 6, 2020

Alright, so I "finished" implementing the Windows counterpart of the feature and reworked the code a little.
Now, I'm having issues with the handle on Windows for the STDOUT -- The handle returned from cli/ops/tty.rs#get_windows_handle for rid 1 and the handle returned from winapi::um::processenv::GetStdHandle(winapi::um::winbase::STD_OUTPUT_HANDLE) are different... The former returns { columns: 0, rows: 0 }, which is incorrect 😄
I can just omit the rid on both UNIX and Windows. But I would appreciate it if anyone was able to find an issue with my previous implementation - which I left in comments for the sake of this conversation.

On another note, my build now fails because of something with cli/tsc.rs -- I rebased my branch with deno/master

@sebastienfilion sebastienfilion force-pushed the sebastienfilion:feature/ioctl-winsize branch 3 times, most recently from ef808e5 to 57c6e97 Jul 6, 2020
@sebastienfilion sebastienfilion force-pushed the sebastienfilion:feature/ioctl-winsize branch from 57c6e97 to ec37918 Jul 7, 2020
cli/ops/tty.rs Outdated Show resolved Hide resolved
cli/ops/tty.rs Outdated Show resolved Hide resolved
fx
Copy link
Member

ry left a comment

@sebastienfilion - looking good! I made some minor changes and renamed it to "Deno.consoleSize" (shorter).

I wonder what this function is called in Python, Node, and Go? Maybe we can figure out a common denominator of names.

Now, I'm having issues with the handle on Windows for the STDOUT

I would be fine if you left Windows not_implemented!() for now.

@lucacasonato
Copy link
Member

lucacasonato commented Jul 8, 2020

I wonder what this function is called in Python, Node, and Go? Maybe we can figure out a common denominator of names.

@ry

@sebastienfilion
Copy link
Contributor Author

sebastienfilion commented Jul 8, 2020

I would be fine if you left Windows not_implemented!() for now.

@ry
The issue is that the handle extrapolated from the resource table and and the default defined in the winapi crate are different. The retrieved handle is 4294967295 and the default handle is 4294967285...
I ran this to try to compare the addresses unsafe { winapi::um::processenv::GetStdHandle(get_windows_handle(&std_file)? as u32) } as u32

Since the work was done for Windows to be supported - for most use-cases at least - I added an assertion that the rid for the console should always be 1 (STDOUT) on Windows; I think it would be an acceptable compromise.
If someone, at some point in time, has a use-case requiring Deno.consoleSize to handle arbitrary consoles, we can re-open that can of worm.

@caspervonb
Copy link
Contributor

caspervonb commented Jul 8, 2020

Also for naming; in Go there's x/crypto/ssh/terminal which exposes terminal.getSize(fd) so that's two for getTerminalSize.

Preferable to have the verb in there IMHO as this will throw when called on a non tty rid. Non verb feels out of place in the context of JavaScript.

@sebastienfilion sebastienfilion force-pushed the sebastienfilion:feature/ioctl-winsize branch from dd1f9c2 to ffea7e0 Jul 9, 2020
caspervonb and others added 2 commits Jul 9, 2020
Fix consoleSize unit tests
cli/ops/tty.rs Outdated Show resolved Hide resolved
@sebastienfilion
Copy link
Contributor Author

sebastienfilion commented Jul 9, 2020

Preferable to have the verb in there IMHO as this will throw when called on a non tty rid.

I agree with caspervonb.

cli/ops/tty.rs Outdated Show resolved Hide resolved
@ry
ry approved these changes Jul 10, 2020
Copy link
Member

ry left a comment

LGTM - thanks Sebastien and Casper!

@ry ry merged commit 1bcc35b into denoland:master Jul 10, 2020
7 checks passed
7 checks passed
test_release macOS-latest
Details
test_release windows-2019
Details
test_release ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.