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

Add sapi_windows_console_size() #17057

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Dec 5, 2024

This function is supposed to retrieve the width and height of a console window on Windows. It expects a single stream argument, and if that is a console, returns its width and height as two element array (aka. pair). On failure the function returns false.


var_dump(sapi_windows_console_size(STDOUT));

outputs something like

array(2) {
  [0]=>
  int(80)
  [1]=>
  int(25)
}

I'm just dropping this here for discussion.

TODO:

  • also implement for POSIX systems (and rename function)
  • re-use in phpdbg

This function is supposed to retrieve the width and height of a console
window on Windows.  It expects a single stream argument, and if that is
a console, returns its width and height as two element array (aka.
pair).  On failure the function returns false.
} else if (php_stream_can_cast(stream, PHP_STREAM_AS_FD | PHP_STREAM_CAST_INTERNAL) == SUCCESS) {
php_stream_cast(stream, PHP_STREAM_AS_FD | PHP_STREAM_CAST_INTERNAL, (void*)&fileno, 0);
} else {
php_error_docref(NULL, E_WARNING, "not able to analyze the specified stream");
Copy link
Member Author

Choose a reason for hiding this comment

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

After reading https://bugs.php.net/bug.php?id=79805, I think the warning should be dropped (we only warn about this when sapi_windows_vt100_support() is used as setter).

Comment on lines +1796 to +1799
if (php_stream_can_cast(stream, PHP_STREAM_AS_FD_FOR_SELECT | PHP_STREAM_CAST_INTERNAL) == SUCCESS) {
php_stream_cast(stream, PHP_STREAM_AS_FD_FOR_SELECT | PHP_STREAM_CAST_INTERNAL, (void*)&fileno, 0);
} else if (php_stream_can_cast(stream, PHP_STREAM_AS_FD | PHP_STREAM_CAST_INTERNAL) == SUCCESS) {
php_stream_cast(stream, PHP_STREAM_AS_FD | PHP_STREAM_CAST_INTERNAL, (void*)&fileno, 0);
Copy link
Member Author

Choose a reason for hiding this comment

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

I had copied this code from sapi_windows_vt100_support() without thinking. Now I had a closer look, and wondered why we even try to cast to PHP_STREAM_AS_FD_FOR_SELECT (we don't actually want to select). Looking at the implementation of php_stdiop_cast() it's clear: this cast won't flush (what we indeed don't want), but other possibly castable stream might not implement PHP_STREAM_AS_FD_FOR_SELECT . So these two cast attempts make some sense (although they rely on a leaky abstraction).

Then I wondered why we first check if we can cast and then actually do cast. Regarding the implementation that makes not much sense, but for the sake of abstraction it is correct. On the other hand, asking whether we can cast to PHP_STREAM_AS_FD actually flushes (for stdio stream), so this is somewhat (s/somewhat/completely) broken anyway.

Well, probably best to leave as is for now.

@nielsdos
Copy link
Member

nielsdos commented Dec 6, 2024

Maybe I'm doing a stupid suggestion, but I suppose that on Unix-likes we can also have this kind of function. I don't know if it exists yet, but:

  1. If it does, it would make sense to have a single function implementing the console size getter
  2. If it does not, maybe the function should be called something non-Windows specific and Unix-likes support should be added

@NattyNarwhal
Copy link
Member

IIRC, that's done with TIOCGWINSZ on Unix-likes, it seems phpdbg is already doing this:

PHPDBG_API uint32_t phpdbg_get_terminal_width(void) /* {{{ */
{
uint32_t columns;
#ifdef _WIN32
CONSOLE_SCREEN_BUFFER_INFO csbi;
GetConsoleScreenBufferInfo(GetStdHandle(STD_OUTPUT_HANDLE), &csbi);
columns = (uint32_t) csbi.srWindow.Right - csbi.srWindow.Left + 1;
#elif defined(HAVE_SYS_IOCTL_H) && defined(TIOCGWINSZ)
struct winsize w;
columns = (uint32_t) ioctl(fileno(stdout), TIOCGWINSZ, &w) == 0 ? w.ws_col : 80;
#else
columns = 80;
#endif
return columns;
} /* }}} */
PHPDBG_API uint32_t phpdbg_get_terminal_height(void) /* {{{ */
{
uint32_t lines;
#ifdef _WIN32
CONSOLE_SCREEN_BUFFER_INFO csbi;
if (GetConsoleScreenBufferInfo(GetStdHandle(STD_OUTPUT_HANDLE), &csbi)) {
lines = (uint32_t) csbi.srWindow.Bottom - csbi.srWindow.Top + 1;
} else {
lines = 40;
}
#elif defined(HAVE_SYS_IOCTL_H) && defined(TIOCGWINSZ)
struct winsize w;
lines = (uint32_t) ioctl(fileno(stdout), TIOCGWINSZ, &w) == 0 ? w.ws_row : 40;
#else
lines = 40;
#endif
return lines;
} /* }}} */

@cmb69
Copy link
Member Author

cmb69 commented Dec 6, 2024

Thanks @nielsdos, that makes perfect sense.

And thank you @NattyNarwhal; I wasn't aware that this is already implemented for phpdbg.

@bukka
Copy link
Member

bukka commented Dec 8, 2024

I don't really get why it's named sapi_... when it's a stream funcion but there is already sapi_windows_vt100_support so guess it's better to keep it consistent.

If it's used in phpdbg, it would make sense to use a common function for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants