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 "os.get_terminal_size()" function #57818

Closed
denilsonsa mannequin opened this issue Dec 16, 2011 · 71 comments
Closed

Add "os.get_terminal_size()" function #57818

denilsonsa mannequin opened this issue Dec 16, 2011 · 71 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@denilsonsa
Copy link
Mannequin

denilsonsa mannequin commented Dec 16, 2011

BPO 13609
Nosy @loewis, @amauryfa, @pitrou, @vstinner, @giampaolo, @merwok
Files
  • termsize.diff
  • termsize.diff.1
  • termsize.diff.2: third version of the patch, after review
  • termsize.diff.3: fourth version of the patch, after more reviews
  • termsize.diff.4
  • termsize.diff.5: sixth version of the patch, after review
  • termsize.diff.6
  • termsize.diff.7: eighth version of the patch
  • termsize.diff.8
  • get_terminal_size_pipe.patch
  • get_terminal_size_pipe-2.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2012-05-16.15:43:07.521>
    created_at = <Date 2011-12-16.01:01:38.409>
    labels = ['type-feature', 'library']
    title = 'Add "os.get_terminal_size()" function'
    updated_at = <Date 2012-05-16.15:43:07.520>
    user = 'https://bugs.python.org/denilsonsa'

    bugs.python.org fields:

    activity = <Date 2012-05-16.15:43:07.520>
    actor = 'pitrou'
    assignee = 'none'
    closed = True
    closed_date = <Date 2012-05-16.15:43:07.521>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2011-12-16.01:01:38.409>
    creator = 'denilsonsa'
    dependencies = []
    files = ['23981', '23983', '24117', '24127', '24150', '24157', '24369', '24373', '24379', '24503', '24504']
    hgrepos = []
    issue_num = 13609
    keywords = ['patch']
    message_count = 71.0
    messages = ['149586', '149595', '149597', '149613', '149616', '149617', '149620', '149627', '149629', '149630', '149631', '149653', '150287', '150388', '150419', '150454', '150456', '150462', '150474', '150475', '150481', '150689', '150693', '150695', '150701', '150703', '150715', '150722', '150731', '150732', '150759', '150775', '150776', '151391', '151729', '151740', '151742', '151749', '151755', '151772', '152330', '152334', '152360', '152388', '152389', '152390', '152394', '152395', '152396', '152397', '152398', '152807', '152808', '152913', '152914', '152915', '153184', '153185', '153231', '153232', '153233', '153235', '153236', '153237', '153238', '153380', '153382', '153390', '153494', '153495', '160877']
    nosy_count = 13.0
    nosy_names = ['loewis', 'amaury.forgeotdarc', 'pitrou', 'vstinner', 'techtonik', 'giampaolo.rodola', 'eric.araujo', 'Arfrever', 'zbysz', 'denilsonsa', 'neologix', 'rosslagerwall', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue13609'
    versions = ['Python 3.3']

    @denilsonsa
    Copy link
    Mannequin Author

    denilsonsa mannequin commented Dec 16, 2011

    Please add a function called "os.get_terminal_size()" that should return a tuple "(width, height)".

    The built-in "argparse" module would directly benefit from this function, as it would wrap the help text correctly. I'm pretty sure many users would also benefit from it.

    Once this function gets implemented, issue bpo-13041 can be trivially fixed. There are some suggestions about how to implement this feature in issue bpo-8408.

    [extra feature:] After this function gets implemented, it might be a good idea to implement some kind of API to detect size changes without requiring probing. (or, at least, the documentation should give some directions about how to achieve it)

    @denilsonsa denilsonsa mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Dec 16, 2011
    @pitrou
    Copy link
    Member

    pitrou commented Dec 16, 2011

    Well, do you want to propose a patch yourself?
    See http://docs.python.org/devguide/ for how to contribute a patch.

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Dec 16, 2011

    The patch is already almost there (in bpo-13041). I'll post an updated version here in a moment.

    @denilsonsa
    Copy link
    Mannequin Author

    denilsonsa mannequin commented Dec 16, 2011

    Zbyszek, I just looked at 1 and I disagree that the environment variable should have higher precedence. In fact, I believe it should have lower precedence, and should be used as a fallback.

    Reason? Imagine a program is launched, and thus it has COLUMNS set to some value. While the program is running, the user resizes the terminal. I believe (though I have not tested) that the envvar will keep the old value, and this function would return wrong dimensions.

    In my opinion, the system-specific code (termios/windll) should be tried first, and then the envvars (as fallback). If all else fails, maybe it would be better to return (None, None) and let the caller of this function handle that.

    @giampaolo
    Copy link
    Contributor

    http://bugs.python.org/file23241/patch1.1.diff
    This looks like something which would fit better into shutil module rather than os.
    Also, the Windows implementation should not rely on ctypes.

    @giampaolo
    Copy link
    Contributor

    Plus, you should provide also heigth, not only width, possibly as a namedtuple:

    >>> import shutil
    >>> shutil.get_terminal_size()
    size(width=80, heigth=170)
    >>>

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Dec 16, 2011

    One reply to three comments:

    [To myself]:

    I'll post an updated version here in a moment.
    Hm, it's not so simple. The implementation is simple,
    but the configure defines are harder than I thought.
    But I'm getting there :)

    Zbyszek, I just looked at [1] and I disagree that the environment variable should have higher precedence. In fact, I believe it should have lower precedence, and should be used as a fallback.

    I disagree. $COLUMNS will usually _not_ be set:

    $ echo $COLUMNS
    119
    
    $ ./python -c 'import os; print("COLUMNS" in os.environ)'
    False

    As I wrote before, the shell only sets COLUMNS for use in the shell, without exporting. Giving a precedence to the environment variable (if it is set by the user) allows to do something like this:
    $ COLUMNS=80 python program.py
    ... do whatever is to be dome for 80 columns ...
    and the normal case (with dynamic checking) will also work correctly.

    Anyway, I think that two functions should be provided: a "raw" one, which does the ioctl, and the "normal" one, which queries $COLUMNS and the the "raw" function. If different behaviour is wanted, than just
    the "raw" one can be called.

    > Also, the Windows implementation should not rely on ctypes.
    Of course. For windows I have:
      #include <conio.h>
      ...
      GetConsoleScreenBufferInfo(handle, &csbi)
      ...

    This looks like something which would fit better into shutil module rather than os.
    This is problematic. The docstring for shutil says:
    Utility functions for copying and archiving files and directory trees.
    So it doesn't seem to fit at all.

    OTOH, there's termios and tty, but they are UNIX only. Module curses is also UNIX only, and slightly different topic, because get_terminal_width should be independent of curses.

    Plus, you should provide also heigth, not only width, possibly as a namedtuple.
    Agreed.

    @giampaolo
    Copy link
    Contributor

    The docstring for shutil says: Utility functions for copying and
    archiving files and directory trees. So it doesn't seem to fit at all.

    Well... shutil should stand for "shell utilities" and it already contains stuff which is not strictly related to file/directory direct operations (see shutil.disk_usage and upcoming shutil.which).

    But maybe you're right... I don't know...
    My point is that a function which attempts different fallbacks (ask OS -> ask os.environ -> return (None, None) / raise exception) sounds more like an utility function rather than something belonging to os module, where you tipically see 1-to-1 interfaces for the underlying system functions.

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Dec 16, 2011

    This is proof-of-concept implementation.

    This adds two modules: termsize (python) and _termsize (C). The first one contains the get_terminal_size user-facing function and namedtuple definition. The second on contains the function query_terminal_size which does the real job.

    Two simple tests are added. I'm not sure if it is feasible to test with different terminal sizes: it certainly _could_ be done, e.g. by launching an xterm with a specified geometry, but this would be much
    more complicated than this implementation, so probably not worth it.

    This was only tested on 64-bit linux, seems to work.

    I'm not sure how to the configure tests should be done: right now the presence of <sys/ioctl.h> is checked, and it is used. If not available,
    <conio.h> is checked, and used. Otherwise NotImplementedError is thrown.

    Would be nice to test on a mac and other systems, but I don't have one available at the moment unfortunately.

    I think that the python part (termsize.py) should be either merged with os.py (or whatever home we find for this function), or rewritten in C in _termsizemodule.c and only imported into os. But since it hasn't yet been decided where this should go, keeping it separate is easier for now.

    @amauryfa
    Copy link
    Member

    Don't forget the Windows platform... here is an implementation: https://bitbucket.org/hpk42/py/src/980c8d526463/py/_io/terminalwriter.py#cl-284
    But it would be better written in C, of course.

    @pitrou
    Copy link
    Member

    pitrou commented Dec 16, 2011

    Ok, a couple of general comments:

    • there is no point having a separate module for a single function; I think the os module (and posixmodule.c for the C side) is a reasonable place where to put this
    • C code should be indented with 4 spaces increments and no tabs (see PEP-7)
    • constants in C code should be uppercase
    • C code should be C89-compliant and therefore we don't use named struct initializers (such as ".m_size = 0")

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Dec 17, 2011

    Here's updated version: termsize.diff.1

    Ok, a couple of general comments:

    • there is no point having a separate module for a single function; I > think the os module (and posixmodule.c for the C side) is a
      reasonable place where to put this
      Done. (But posixmodule.c is so enourmous... I feel bad making it even longer.)
    • C code should be indented with 4 spaces increments and no tabs (see > PEP-7)
    • constants in C code should be uppercase
    • C code should be C89-compliant and therefore we don't use named
      struct initializers (such as ".m_size = 0")
      All done, I hope.

    This version was tested on linux/amd64 and win32 (XP).

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Dec 28, 2011

    Seems to work also on kfreebsd/debian (with eglibc).

    @pitrou
    Copy link
    Member

    pitrou commented Dec 30, 2011

    (review posted on http://bugs.python.org/review/13609/show )

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Dec 31, 2011

    Thanks for the review!

    New version is attached. The code is actually slightly shorter, but
    there are more docs.

    Doc/library/os.rst | 52 +++++++++++++++++++
    Doc/whatsnew/3.3.rst | 5 +
    Lib/os.py | 43 +++++++++++++++
    Lib/test/test_termsize.py | 31 +++++++++++
    Misc/NEWS | 3 +
    Modules/posixmodule.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++
    configure | 2
    configure.in | 2
    pyconfig.h.in | 3 +
    9 files changed, 264 insertions(+), 2 deletions(-)

    The patch also lacks some documentation update in Doc/library/os.rst.
    Added as a subsection under "File Descriptor Operations" section.
    I also added an entry to Misc/NEWS and Doc/whatsnew/3.3.rst.

    Lib/os.py:833: def get_terminal_size(columns=80, rows=25):
    The arguments seem ignored in the function body, so I think they should simply
    be removed.
    The implementation was borked and didn't actually do what the
    docstring said. It should be fixed now.

    I want to retain the default values, because get_terminal_size()
    should "do the right thing" in case stdout is not a terminal,
    without the user needing to wrap it in a try..except block
    or some other test. Putting the default values as parameters
    makes it clear what will be returned in case query fails, and
    makes it easy to override those defaults.

    To make it clearer that those are the fallback values, I renamed the
    parameter to 'fallback=(c, r)', which should be understandable even
    without reading the docstring. Having it as a single parameter might
    also make it easier to add something like 'minimum=(c, r)' in the
    future.

    I wonder if you shouldn't try to explicitly pass sys.stdout.fileno() here, or
    perhaps sys.__stdout__.fileno() since the former could be overriden.
    OK, changed to sys.__stdout__.fileno().

    I think that sys.__stdout__ is better, because when stdout is overridden,
    it is usually with something that is not a terminal. Changing the output
    terminal is probably pretty rare.

    Actually, perhaps get_terminal_size() should have an optional file descriptor
    argument.
    Querying anything other than stdout should be pretty rare. If
    necessary, query_terminal_size() can be called explicitly. Also,
    the variables $COLUMNS and $ROWS are usually meant to refer to stdout,
    so get_terminal_size() is about stdout and ROWS and COLUMNS, and the
    other function allows full control.

    Lib/test/test_termsize.py:1: import unittest
    This file seems to lack an if __name__ == '__main__' like other test files
    OK.

    Besides, why not simply put the tests in test_os.py?
    The tests were nicely self-contained. I wanted to be able to do:
    ./python Lib/test/test_termsize.py
    and
    ./python Lib/test/test_termsize.py | cat
    but I guess it can be easily moved to test_os.py if necessary.

    Lib/test/test_termsize.py:7: def set_environ(**variables):
    There's already EnvironmentVarGuard in test.support.
    OK.

    Lib/test/test_termsize.py:7: def set_environ(**variables):
    There's already EnvironmentVarGuard in test.support.
    Lib/test/test_termsize.py:25: self.assertTrue(0 < size.columns)
    Better to use assertGreater here, failure messages will be more informative.
    Lib/test/test_termsize.py:33: self.assertTrue(size.columns == 777)
    And better to use assertEqual here.
    OK.

    Typo here ("imlmented").
    OK.

    Modules/posixmodule.c:10571: return PyErr_Format(PyExc_ValueError)
    I don't think there's any point in changing the error here. Just let
    a normal OSError be raised.
    OK, s/ValueError/OSError/ here and the the windows case below too.

    Modules/posixmodule.c:10573: return PyErr_SetFromErrno(PyExc_IOError);
    For the record, in 3.3, PyExc_IOError is an alias of PyExc_OSError.
    OK, s/IOError/OSError/ for clarity.

    Modules/posixmodule.c:10599: return PyErr_Format(PyExc_IOError, "error %i",
    (int) GetLastError());
    Just use PyErr_SetFromWindowsErr(GetLastError()).
    OK.

    Modules/posixmodule.c:11086: {"query_terminal_size", query_terminal_size,
    METH_VARARGS, termsize__doc__},
    I don't think there's any point in making this helper function public, so I'd
    rename it _query_terminal_size or something.
    The idea is that query_terminal_size() can be used to do the
    low-level query ignoring $COLUMNS and $ROWS and returing the real
    error if something goes wrong. If is documented, so I think that
    it can be "public".

    I've also improved the docstrings slightly.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 2, 2012

    Thanks for the updated patch.
    I think I would like other people's opinions about the dual-function approach.

    @giampaolo
    Copy link
    Contributor

    What I would do:

    • build the namedtuple in Python rather than in C
    • I don't particularly like CamelCased names for nametuples (I would use "size" instead of "TerminalSize")
    • on UNIX, the ioctl() stuff can be done in python rather than in C
    • use C only on Windows to deal with GetStdHandle/GetConsoleScreenBufferInfo; function name should be accessed as nt._get_terminal_size similarly to what we did for shutil.disk_usage in bpo-12442.
    • do not raise NotImplementedError; if the required underlying functions are not available os.get_terminal_size should not exists in the first place (same as we did for shutil.disk_usage / bpo-12442)

    Also, I'm not sure I like fallback=(80, 25) as default, followed by:

        try:
            size = query_terminal_size(sys.__stdout__.fileno())
        except OSError:
            size = TerminalSize(fallback)
    

    That means we're never going to get an exception.
    Instead I would:

    • provide fallback as None
    • let OSError propate unless fallback is not None

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Jan 2, 2012

    Nice patch :-)

    I think the two function approach works well.

    Since you have already checked that termsize is not NULL, Py_DECREF can be used instead of Py_CLEAR.

    Would it not be better to use sys.__stdout__ instead of 1 in the documentation and to use STDOUT_FILENO instead of 1 in the code?

    A brief google suggested that Solaris requires sys/termios.h for TIOCGWINSZ. Perhaps also only define TERMSIZE_USE_IOCTL if TIOCGWINSZ is defined.
    Like so:
    #if defined(HAVE_SYS_IOCTL_H)
    #include <sys/ioctl.h>
    #if defined(TIOCGWINSZ)
    #define TERMSIZE_USE_IOCTL
    #else
    #define TERMSIZE_USE_NOTIMPLEMENTED
    #endif
    #elif defined(HAVE_CONIO_H)
    #include <windows.h>
    #include <conio.h>
    #define TERMSIZE_USE_CONIO
    #else
    #define TERMSIZE_USE_NOTIMPLEMENTED
    #endif

    (didn't check the windows parts)

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Jan 2, 2012

    Thanks for the reviews!

    • build the namedtuple in Python rather than in C
      I started this way, but if two functions are used, it is nicer to have them both
      return the same type. If it was defined in the Python part, there would be
      loop in the sense that os would import the function from _posixmodule, and
      _posixmodule would import the namedtuple from os. The code for the tuple is fairly simple,
      mostly docs, and saving a few lines just doesn't seem to be worth the complication of doing
      it other way around.
    • I don't particularly like CamelCased names for nametuples (I would use "size" instead of "TerminalSize")
      I was following the style in _posixmodule: there is WaitidResultType and SchedParamType.
      "size" seems to generic, maybe something like "terminal_size" would be better (SchedParamType
      is exported as sched_param).

    Will change to "terminal_size".

    • on UNIX, the ioctl() stuff can be done in python rather than in C
      It can, but it requires extracting the bytes by hand, doing it in C is cleaner.
      The C implementation is only 5 lines (in addition to the code necessary for windows)
      and again it seems simpler this way. Also the configuration is kept in one
      place in the #ifdefs at the top, not in two different files.
    • use C only on Windows to deal with GetStdHandle/GetConsoleScreenBufferInfo; function name should be
      accessed as nt._get_terminal_size similarly to what we did for shutil.disk_usage in bpo-12442.
    • do not raise NotImplementedError; if the required underlying functions are not available
      os.get_terminal_size should not exists in the first place (same as we did for shutil.disk_usage / bpo-12442)

    I think that there is a difference in "importance" -- shutil.disk_usage cannot be
    faked, or ignored, and if it is unavailable, then some work-around must be implemented.
    OTOH, the terminal size is an embellishment, and if a wrong terminal size is used, the output
    might wrap around or be a bit narrow (the common case), but like in argparse.FancyFormatter,
    just continuing is probably reasonable. So to save the users of the function the trouble
    to wrap it in some try..except, let's cater for the common case.

    This same argument goes for defining the function only if an implementation is available:
    argparse would need something like:
    try:
    os.get_terminal_size
    except NameError:
    def get_terminal_size():
    return (80, 25)
    which just doesn't seem to _gain_ anything.

    Also, I'm not sure I like fallback=(80, 25) as default, followed by:
    [...]
    That means we're never going to get an exception.
    Instead I would:

    • provide fallback as None
    • let OSError propate unless fallback is not None
      Again, if the user forgets to add the fallback, and only tests in a terminal, she
      would get an unnecessary surprise when running the thing in a pipe. So the fallback
      would have to be almost always provided... and it will almost always be (80, 25).
      So let's just provide it upfront, and let the user call the low-level function if
      they want full control.

    -----------------------------------------------------------------------------------

    I think the two function approach works well.
    :)

    Since you have already checked that termsize is not NULL, Py_DECREF can be used instead of Py_CLEAR.
    OK.

    Would it not be better to use sys.__stdout__ instead of 1 in the documentation and to use STDOUT_FILENO
    instead of 1 in the code?
    OK.

    A brief google suggested that Solaris requires sys/termios.h for TIOCGWINSZ. Perhaps also only define
    TERMSIZE_USE_IOCTL if TIOCGWINSZ is defined.
    Hm, I don't have solaris available, so I won't be able to check easily, but
    maybe sys/termios.h should be imported if TIOCGWINSZ is not available. Would
    be nice to provide the implementation if possible.

    Like so:
    Something like:

    #if defined(HAVE_SYS_IOCTL_H)
    #include <sys/ioctl.h>
    #if defined(TIOCGWINSZ)
    #define TERMSIZE_USE_IOCTL
    #else

    #if defined(HAVE_SYS_TERMIOS_H)
    #include <sys/termios.h>
    #if defined(TIOCGWINSZ)
    #define TERMSIZE_USE_IOCTL
    #else
    #define TERMSIZE_USE_NOTIMPLEMENTED

    #endif
    #elif defined(HAVE_CONIO_H)
    #include <windows.h>
    #include <conio.h>
    #define TERMSIZE_USE_CONIO
    #else
    #define TERMSIZE_USE_NOTIMPLEMENTED
    #endif

    @rosslagerwall
    Copy link
    Mannequin

    rosslagerwall mannequin commented Jan 2, 2012

    I'll try & investigate Solaris a bit...

    Also, what should be the default terminal size?

    Gnome-terminal and xterm seem to default to 80x24, not 80x25.

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Jan 2, 2012

    Updated patch termsize.diff.3

    • s/TerminalSize/terminal_size/ in Python code
    • change the fallback to (80, 24) (seems to be the commonest default)
    • s/Py_CLEAR/Py_DECREF/
    • use STDOUT_FILENO
    • add more hrefs in docs
    • include <termios.h> is available (untested fix for solaris compatibility)
    • rename TerminalSize as terminal_size in Python code

    I tested this iteration only on linux and windows, but it is not substantially changed, so should still work on *bsd. (I previously
    tested on debian/kfreebsd and dragonflybsd and it seemed functional).

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jan 5, 2012

    I haven't read much of this issue, but I strongly dislike the use of named tuples. Either we want people to use named fields, then we should use a regular class (possibly with slots), or we want to define the result as two values, then there should be a plain tuple result.

    named tuples should only be used as a compatibility mechanism, when the first design used tuples, and it was later found that additional values need to be returned which would change the number of values (or the original design was considered bad because it returned too many positional values to properly keep track).

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Jan 5, 2012

    I haven't read much of this issue, but I strongly dislike the use of
    named tuples.
    I don't really have a very strong opinion, but (cols, rows) does seem a lot like a tuple -- it really is just a pair of values without other function or state. Still I would much prefer to say
    get_terminal_size().columns
    than
    get_terminal_size()[0]
    So a bare tuple seems like the worst choice.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 5, 2012

    The point of named tuples here is that you can use both
    get_terminal_size().columns
    or
    columns, rows = get_terminal_size()
    depending on the situation.
    Also, the better repr() makes debugging easier.

    @vstinner
    Copy link
    Member

    vstinner commented Jan 6, 2012

    Some comments about termsize.diff.3.

    I don't see why there are two functions, one should be enough: get_terminal_size() should be dropped, and query_terminal_size() renamed to get_terminal_size(). As said before, I don't think that reading ROWS and COLUMNS environment variables is useful. If a program chose to rely on these variables, it can reimplement its own "try env var or fallback on get_terminal_size()" function.

    get_terminal_size() should not have a fallback value: it should raise an error, and the caller is responsible to decide what to do in this case (e.g. catch the exception and use its own default value). Most functions in the posix module work like this, and it avoids the difficult choice of the right default value. (fallback=None is an hack to avoid an exception, it's not the pythonic.)

    I don't think that it's possible that stdin, stdout and/or stderr have its own terminal. I suppose that the 3 streams are always attached to the same terminal. So I don't see why the function would take an argument. Tell me if I am wrong.

    Instead of using sys.__stdout__.fileno(), you can directly use 1 because Python always create sys.__stdout__ from the file descriptor 1.

    I think that a tuple (columns, rows) would be just fine. A namedtuple helps when you have a variable number of fields, or more than 3 fields, but here you just have 2 fields, it's not too much difficult to remember which one contains the columns.

    I would prefer an optional function, instead of implementing a function raising a NotImplementedError. All other posix functions are defined like this.

    ioctl() is already exposed in the fcntl module, I don't see a specific test for <sys/ioctl.h> header. It looks like the module is always compiled on Unix, I don't see how fcntl and ioctl are tested in setup.py.

    I don't think that you need <conio.h> to get GetConsoleScreenBufferInfo(), <windows.h> should be enough. So just check for "#ifdef MS_WINDOWS".

    Your function is helpful, and it is surprising that nobody proposed to implement it in Python. Some libraries did already implement their own function (like the "py" library).

    @pitrou
    Copy link
    Member

    pitrou commented Jan 6, 2012

    I don't think that it's possible that stdin, stdout and/or stderr have
    its own terminal. I suppose that the 3 streams are always attached to
    the same terminal. So I don't see why the function would take an
    argument. Tell me if I am wrong.

    I think it can be useful in case the program creates its own
    session/terminal using openpty?

    Instead of using sys.__stdout__.fileno(), you can directly use 1
    because Python always create sys.__stdout__ from the file descriptor
    1.

    From pythonrun.c:

    /* Set sys.stdin */
    fd = fileno(stdin);
    [...]
    /* Set sys.stdout */
    fd = fileno(stdout);
    [...]
    /* Set sys.stderr, replaces the preliminary stderr */
    fd = fileno(stderr);
    

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jan 6, 2012

    Zitat von Antoine Pitrou <report@bugs.python.org>:

    Antoine Pitrou <pitrou@free.fr> added the comment:

    The point of named tuples here is that you can use both
    get_terminal_size().columns
    or
    columns, rows = get_terminal_size()
    depending on the situation.

    And my point is that we should make the choice which
    of these is more obvious, and drop the other.

    Also, the better repr() makes debugging easier.

    A class could still have a nice repr.

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Jan 6, 2012

    Following comments by Martin and Victor, here is next version: termsize.diff.4

    Changes:

    • just check for defined(MS_WINDOWS) and rely on <windows.h>.
    • rename query_terminal_size to get_terminal_size_raw
      This way it should be clearer that the second one is low-level,
      and should be less exposed. I don't want to call it
      _get_terminal_size() because it is not just an implementation
      detail and would sometimes be called directly.
    • NotImplementedError is gone. get_terminal_size_raw() is not
      defined if not possible.

    Non-changes:

    • sys.__stdout__.fileno() is not changed to 1, because as Antoine
      pointed out, it is set at runtime.
    • STDOUT_FILENO: not defined on windows, so just use 1 and add a comment
    • fd argument is retained, because we might want to test terminals
      opened with openpty.
    • two functions: still there. I think that get_terminal_size() should
      provide an easy-to-use, even trivial-to-use, way to get a sensible
      value without writing a wrapper. In the _majority_ of cases the
      wrapper would be something like get_terminal_size() currently.
    • named tuple: like Antoine said, it gives nice syntax.
    • testing for <sys/ioctl.h>: in termios and other modules, setup.py
      first tests if we are on unix. But there might be unices without
      TIOCGWINSZ, and non-unix systems where TIOCGWINSZ _is_ defined,
      so it seems a functional test is simpler and more robust.

    Tested on linux and windows XP.

    @vstinner
    Copy link
    Member

    termsize.diff.7:

    • shutil.get_terminal_size(): I would prefer columns and lines
      variable names instead of "ccc" and "rrr"
    • "Right now the values are not cached, but this might change." why
      would it be cached? this sentence can just be removed
    • I would prefer os.get_terminal_size() instead of
      os.query_terminal_size(), I didn't know other function using the verb
      "query" in Python
    • To keep os simple, os.query_terminal_size() can return a simple,
      whereas shutil.get_terminal_size() returns a namedtuple
    • test_os.py: use @unittest.skipUnless() on TermsizeTests to check if
      os.query_terminal_size() is available
    • test.py, test_does_not_crash() catchs OSError, you may only ignore
      some error codes, not any. For example, skip the test if stdout is not
      a tty

    + try:
    + size = os.query_terminal_size(sys.__stdout__.fileno())
    + except (NameError, OSError):
    + size = os.terminal_size(fallback)

    AttributeError should be catched here, not NameError. Or better, check
    if os has a query_terminal_size() function.

    +.. function:: get_terminal_size(fallback=(columns, lines))

    Hum, you may document the default value: (80, 24).

    shutil.get_terminal_size() doesn't allow to choose the file
    descriptor. Would it be useful to allow to get the size of sys.stderr
    or another TTY?

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Jan 31, 2012

    Well, right now it's just one function. Functionality which you propose
    could of course be useful, but let's leave if for another day, another
    issue.

    See also http://bugs.python.org/issue13609#msg149627 and bpo-444582 and
    bpo-12442 -- shutil is growing in different directions, and terminal size
    can be one of them.

    > Antoine Pitrou added the comment:
    > "except Exception" clauses in the tests are too broad.
    Fixed.
    > Otherwise, looks fine.

    > STINNER Victor added the comment:
    > - shutil.get_terminal_size(): I would prefer columns and lines
    > variable names instead of "ccc" and "rrr"
    Changed (this was a leftover from the version when there was no named
    tuple).

    > - I would prefer os.get_terminal_size() instead of
    > os.query_terminal_size(), I didn't know other function using the verb
    > "query" in Python
    Changed. I used os.g_t_s and shutil.g_t_s in docs so it should be clear
    which is which.

    > - To keep os simple, os.query_terminal_size() can return a simple,
    > whereas shutil.get_terminal_size() returns a namedtuple
    We would have two very similar functions returning something different.
    Also, the amount of code saved will be negligible, because the named
    tuple is mostly docs. I prefer to keep it in os to keep both functions
    similar.

    > - test_os.py: use @unittest.skipUnless() on TermsizeTests to check if
    > os.query_terminal_size() is available

    • To keep os simple, os.query_terminal_size() can return a simple,
      whereas shutil.get_terminal_size() returns a namedtuple
      AttributeError should be catched here, not NameError. Or better, check
      if os has a query_terminal_size() function.
      Fixed. I changed the tests a bit, e.g. to take into account that stty
      queries stdin, not stdout.

    > Hum, you may document the default value: (80, 24).
    Done.

    shutil.get_terminal_size() is tied to COLUMNS and ROWS and thus makes
    most sense for stdout. But I guess that an optional parameter could be
    added:
    shutil.get_terminal_size(fd=None, fallback=(80, 24))
    where fd could be either an integer, or an object with .fileno().
    I don't know.

    > - "Right now the values are not cached, but this might change." why
    > would it be cached? this sentence can just be removed
    Done. In theory it could be cached with watching for SIGWINCH, but I'll
    just remove the comment for now.

    Thanks for the review and comments!

    Patch version nine attached: termsize.diff.8

    @techtonik
    Copy link
    Mannequin

    techtonik mannequin commented Jan 31, 2012

    What happens with COLUMNS env variables if terminal is resized after the Python script is executed. Will get_terminal_size() return new value?

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Jan 31, 2012

    the Python script is executed. Will get_terminal_size()
    > return new value?
    Please see previous discussion and the docs (and the SUSv2 specs).
    The env. var. is for overriding.

    @techtonik
    Copy link
    Mannequin

    techtonik mannequin commented Jan 31, 2012

    On Tue, Jan 31, 2012 at 7:48 PM, Zbyszek Szmek <report@bugs.python.org>wrote:

    Zbyszek Szmek <zbyszek@in.waw.pl> added the comment:

    > the Python script is executed. Will get_terminal_size()
    > return new value?
    Please see previous discussion and the docs (and the SUSv2 specs).
    The env. var. is for overriding.

    Could you just answer the question or provide a relevant link
    (unfortunately I don't have time to read SUSv2 specs)?

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Jan 31, 2012

    @techtonik
    Copy link
    Mannequin

    techtonik mannequin commented Feb 7, 2012

    All right, I've found some time to grep conversation related to COLUMNS/ROWS environment/shell variable.

    +1 for low level system wrapper to get current stdout console size
    -1 on COLUMN/ROWS "business logic"

    My user story 001:
    I need exact values of my console and don't want them to be overridden by environment/shell variables.
    -- or --
    When troubleshooting problems with console width on user side and don't want this feature to implicitly depend on some variables in user environment.

    @vstinner
    Copy link
    Member

    vstinner commented Feb 7, 2012

    +1 for low level system wrapper to get current stdout console size

    So use os.get_terminal_size()

    -1 on COLUMN/ROWS "business logic"

    So don't use shutil.get_terminal_size(), but it looks like their is a
    use case for this feature.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 8, 2012

    New changeset c92f8de398ed by Antoine Pitrou in branch 'default':
    Issue bpo-13609: Add two functions to query the terminal size:
    http://hg.python.org/cpython/rev/c92f8de398ed

    @pitrou
    Copy link
    Member

    pitrou commented Feb 8, 2012

    The patch is finally committed. Thank you Zbyszek for having been quite patient.
    (according to Murphy's laws, this will surely break some buildbots)

    @pitrou pitrou closed this as completed Feb 8, 2012
    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Feb 8, 2012

    Thanks for merging!

    I'll try to keep an eye on the buildbot results, but please add me to
    any bugs in case I miss something.

    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Feb 12, 2012

    test_stty_match() should be skipped also when os.isatty(sys.__stdin__.fileno()) is False.
    This test fails when test suite is run by Portage (Gentoo package manager), which somehow sets null input.

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Feb 12, 2012

    Hi,

    looking at the tests, the test should be skipped with 'stty invocation
    failed'. Does something different happen? Can you post the output from
    the tests?

    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Feb 12, 2012

    Actually this test fails due to another reason. I'm still investigating it.

    In the meantime, a different bug was found:

    # stty size | cat
    46 157
    # python3.3 -c 'import os; print(os.get_terminal_size())' | cat
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    OSError: [Errno 22] Invalid argument

    Especially redirection to less is often used. stty size works, so it should be possible to fix Python.

    @Arfrever Arfrever mannequin reopened this Feb 12, 2012
    @vstinner
    Copy link
    Member

    Using strace, I see that stty calls ioctl(TIOCGWINSZ) on stdin (fd=0)
    if it failed on stdout (fd=1), whereas Python only tries stdout.

    Attached patch implements a similar idea.

    @pitrou
    Copy link
    Member

    pitrou commented Feb 12, 2012

    Your patch is wrong. It always discards ioctl(stdout), even if it was successful.

    @vstinner
    Copy link
    Member

    New try (I fixed my email address and the patch).

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Feb 12, 2012

    Using strace, I see that stty calls ioctl(TIOCGWINSZ) on stdin (fd=0)
    if it failed on stdout (fd=1), whereas Python only tries stdout.
    It was done this way by design. Maybe checking stdin can be also useful,
    but it is a rather big change in semantics. I think it should be a separate bug.

    It is pretty common for programs to behave differently when run through pipe, even if stdin is on a tty. stty is rather the exception than the rule. E.g. almost all programs disable color when piped explicitly through less. 'dpkg | cat' ignores terminal width. So does git and ls.
    stty is special, because the only purpose of that program is to query terminal size, but it cannot be taken as a model for the behaviour of a general purpose program.

    @Arfrever
    Copy link
    Mannequin

    Arfrever mannequin commented Feb 12, 2012

    The purpose of os.get_terminal_size() is the same as stty size, so stty size could be a model for behavior of os.get_terminal_size().

    @vstinner
    Copy link
    Member

    E.g. almost all programs disable color when piped explicitly through less.

    Using my patch, you can use os.get_terminal_size(sys.stdout.fileno()) if you like to get an error if sys.stdout is a pipe. My patch only changes the behaviour if no argument is specified (hum, the "special" behaviour should be documented). Or you can also check explicitly sys.stdout.isatty().

    It is just more convinient to fallback to stdin if stdout is not a TTY.

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Feb 15, 2012

    New try (I fixed my email address and the patch).

    The purpose of os.get_terminal_size() is the same as stty size, so
    stty size could be a model for behavior of os.get_terminal_size().
    I guess that this fallback will make some things easier. As long it is documented, the user should be able to get whatever behaviour they want.

    I have four small questions:

    • -1 is used as the argument meaning "try stdout and stdin". But -1 is
      also used the error return value for open(2) and other system
      functions. I know that Python checks the return value to throw an
      exception, and -1 shouldn't "leak" and be passed by mistake to
      os.get_terminal_size(). Can someone confirm that this is
      the case?
    • Why stderr is not checked too? It would make os.get_terminal_size()
      return the real terminal value in a the middle of a pipe:
      cat | python -c 'import os; os.get_terminal_size()' | cat.
    • some doc change is missing
    • in the new version the line with ioctl(fd, TIOCGWINSZ, &w) repeats
      three times. It think it could be reduced to two without loss
      of readability.

    @denilsonsa
    Copy link
    Mannequin Author

    denilsonsa mannequin commented Feb 15, 2012

    On Tue, Feb 14, 2012 at 22:17, Zbyszek Szmek <report@bugs.python.org> wrote:

    I have four small questions:

    • -1 is used as the argument meaning "try stdout and stdin".
    • Why stderr is not checked too?

    I propose the following solution: accept either an integer or a
    sequence of integers as the fd parameter. In the second case, try each
    FD until one of them returns the terminal size.

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Feb 15, 2012

    Wouldn't this be quite unwieldy to use:
    os.get_terminal_size(sys.__stdout__.fileno(),
    sys.__stdin__().fileno(),
    sys.__stderr__.fileno())
    ?

    @pitrou
    Copy link
    Member

    pitrou commented Feb 16, 2012

    I don't think there's much point in the proposed complications.
    If you're willing to know the terminal size, you're probably interested in displaying something in it (using stdout), so why would you care about stderr or stdin?

    @zbysz
    Copy link
    Mannequin

    zbysz mannequin commented Feb 16, 2012

    Stdout can be connected to a pipe, e.g to less, which in turn might
    be connected to a terminal. The program can then display output properly
    scaled for the terminal, assuming that because stdin is
    connnected to a terminal, output will eventually reach the same
    terminal. Sometimes this is true, sometimes not.

    @pitrou
    Copy link
    Member

    pitrou commented May 16, 2012

    I am closing as fixed. If you want to propose further enhancements, please open a new issue.

    @pitrou pitrou closed this as completed May 16, 2012
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants