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

IDLE: Squeezed line count ignores wrapping before newline #79389

Closed
taleinat opened this issue Nov 10, 2018 · 17 comments
Closed

IDLE: Squeezed line count ignores wrapping before newline #79389

taleinat opened this issue Nov 10, 2018 · 17 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes topic-IDLE type-bug An unexpected behavior, bug, or error

Comments

@taleinat
Copy link
Contributor

BPO 35208
Nosy @terryjreedy, @taleinat, @miss-islington
PRs
  • bpo-35208: Fix IDLE Squeezer line counting #10449
  • [3.7] bpo-35208: Fix IDLE Squeezer line counting (GH-10449) #11305
  • 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 = 'https://github.com/terryjreedy'
    closed_at = <Date 2018-12-24.12:22:01.665>
    created_at = <Date 2018-11-10.07:18:21.251>
    labels = ['3.8', 'expert-IDLE', 'type-bug', '3.7']
    title = 'IDLE: Squeezed line count ignores wrapping before newline'
    updated_at = <Date 2018-12-24.16:42:40.229>
    user = 'https://github.com/taleinat'

    bugs.python.org fields:

    activity = <Date 2018-12-24.16:42:40.229>
    actor = 'miss-islington'
    assignee = 'terry.reedy'
    closed = True
    closed_date = <Date 2018-12-24.12:22:01.665>
    closer = 'taleinat'
    components = ['IDLE']
    creation = <Date 2018-11-10.07:18:21.251>
    creator = 'taleinat'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 35208
    keywords = ['patch']
    message_count = 17.0
    messages = ['329600', '329602', '331722', '331742', '331756', '331757', '331758', '331759', '331779', '331784', '331885', '331904', '332278', '332386', '332398', '332492', '332494']
    nosy_count = 3.0
    nosy_names = ['terry.reedy', 'taleinat', 'miss-islington']
    pr_nums = ['10449', '11305']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue35208'
    versions = ['Python 3.7', 'Python 3.8']

    @taleinat
    Copy link
    Contributor Author

    Squeezing a single long line with a newline, e.g. 'a'*200, results in "Squeezed text (1 lines)".

    Also, changing the width of the window (even to very small widths) doesn't affect the number of lines reported when squeezing (when squeezing *after* the width change).

    @taleinat taleinat added 3.7 (EOL) end of life 3.8 only security fixes labels Nov 10, 2018
    @taleinat taleinat added topic-IDLE type-bug An unexpected behavior, bug, or error labels Nov 10, 2018
    @taleinat
    Copy link
    Contributor Author

    See PR #54658 with a fix.

    @terryjreedy
    Copy link
    Member

    Changing the width of the window changes the number of visible lines because of line wrapping. But it does not change the number of logical lines. My understanding is that squeezer currently reports the latter, and that is not necessarily a bug. Not that I am considering replacing line wrapping with a horizontal scrollbar.

    @terryjreedy
    Copy link
    Member

    Note that I *am* considering ...

    It seems that a reasonable rule might be to squeeze if n lines or the equivalent of n full lines (75 chars each) in total characters. In other words, if lines >= N or chars >= to 75*N: squeeze(). Do we have a rule not to squeeze fewer but longer lines?

    @taleinat
    Copy link
    Contributor Author

    Squeezer currently does take wrapping into account, i.e. it does take IDLE's "soft" line wrapping into account when counting lines. Therefore the auto-squeeze criterion of "at least X lines" does lead to auto-squeezing for any mixture of very long lines and/or many short lines.

    However, there is currently a bug in the line counting mechanism which is causing it to not count long, soft-wrapped lines properly in some cases. This issue is about that bug, and the PR fixes it. Merging this fix should result in the originally intended behavior, which is similar to what Terry described in his last comment here.

    @terryjreedy
    Copy link
    Member

    In the example I gave on the PR, 200 70 char lines, the squeezed box says 200 lines with or without line wrapping (before the patch).

    What is a simple case that you think is buggy?

    @taleinat
    Copy link
    Contributor Author

    As I mentioned when opening this issue: 'a'*200 (or 'a'*10000).

    @taleinat
    Copy link
    Contributor Author

    Note the lack of newline at the end; that triggers the problematic path in the current calculation.

    @terryjreedy
    Copy link
    Member

    There are 2 questions. 1. Should we review squeezer, in light of further complemplation and experience, and possibly patch it in a couple of weeks or so? Yes.

    1. Is there such a severe bug that we should possibly rush a fix and ask Ned to cherry-pick something after the well announced deadline? Since I see behavior different from your report, I don't currently see a reason to ask.

    I have tested with Windows 3.7.1 64 bit installed, Windows 3.7.2c1+ 32 bit repository debug build from yesterday, Mac 3.7.2rc1 64 bit installed.

    'a'*200 and 'a'*200 + '\n' both display over 3 lines and are *not* autosqueezed. Both squeeze as 3 lines. Unsqueeze, narrow window, and resqueeze, and the result is *4* lines, not 3. Line single lines with 20000 and 20002 characters, a'*20000 and 400 * (48*'a'+'\n), are autosqueezed, as more than 250 lines, depending on the window width. Note interactive echo uses repr(), so that the '\n' converted to 1 newline is converted back to '\n'. Lines here also depends on width.

    print(200 * (70*'a'+'\n')) is squeezed as 200 lines regardless of wrapping or not. So the effect of wrapping on line count depends on the number of lines.

    print(40 * (100*'a'+'\n')) (4000 chars) is not squeezed, even though it would be by the 50 lines * 75 chars/line (3750 chars) rule I suggested.

    Further experiments shows that autosqueezing only applies to single writes to sys.stdout (and maybe to sys.stderr). print(40 * (100*'a'+'\n'), 40 * (100*'a'+'\n')) prints 40 lines twice with separate write calls and is not autosqueezed. Manual squeeze squeezes contiguous stdout or stderr blocks even if the former is the result of more than one print arg or calls. Stdout blocks separated by an input() response are autosqueezed separately. This all seems fine. The doc could use a few more words.

    @taleinat
    Copy link
    Contributor Author

    My mistake on the reproduction; try: print('a'*10000+'\n'). It does not trigger auto-squeezing, though this is an obvious case where it should be triggered. (Manual squeezing also results in showing just 1 as the line count.)

    @terryjreedy
    Copy link
    Member

    My suggest simple rule would sqeeze it. With n = 50 and k = 75,

    if len(s) > n*k or s.count('\n') > n: squeeze(s)

    would squeeze at 3750. With k = 50, as 2500.

    I am not sure yet what to do about wrapping. For development, a count of actual lines might be more helpful than a variable number of wrapped lines.

    @taleinat
    Copy link
    Contributor Author

    Terry, I'm not sure I follow your thinking, but it feels like over-thinking. This is a simple bug with a simple fix.

    Barring the current bug (which the attached PR fixes), the current logic counts lines, taking soft wrapping into account. This was chosen to make things as simple as possible for the users, in terms of what's displayed on squeezed buttons and in terms of configuring auto-squeezing. The fix is simple and with it Squeezer works quickly and consistently.

    If you'd like to change the behavior, let's discuss that elsewhere.

    @terryjreedy
    Copy link
    Member

    Tal, trying to understand your confused description of what behavior you want to fix required me to experiment and think. There are at least 2 separate issues: triggering of auto-squeeze and lines reported (regardless of what triggers squeezing). The following pair of experiments exhibits inconsistency in both respects.

    >>> print('a'*3920) # Fills 49 80-char lines, correctly not squeezed.
    ...
    >>> print('a'*3921)  # Wrapped to 50 lines, correctly auto squeezed.
    [Squeezed text (50 lines).]  # Correct number when reporting wrapped lines.
    >>> print('a'*3921+'\n')  # Ditto, but not auto-squeezed.
    ...
    # Squeeze manually
    [Squeezed text (1 line).]  # Different line count -- of output lines.
    >>> print('a'*3920+'\na')  # Not initially squeezed, '2 lines'.

    From msg331784 it appears that you are more concerned here with auto squeeze triggering than with line count. Now that I think I know what you are trying to fix, I can review the code change.

    I agree to consider the ambiguity between output lines and display lines, and the effect on line count, later.

    Part of my thinking with the simple auto-squeeze formula, besides just simplifying the code, it this. Raymond claimed that squeezing slows down printing. If measurably true, one way to avoid a slow down would be to use a simple heuristic formula to estimate the number of wrapped lines instead of exactly counting. This would be a separate issue, if needed.

    @taleinat
    Copy link
    Contributor Author

    Part of my thinking with the simple auto-squeeze formula, besides just simplifying the code, it this. Raymond claimed that squeezing slows down printing. If measurably true, one way to avoid a slow down would be to use a simple heuristic formula to estimate the number of wrapped lines instead of exactly counting. This would be a separate issue, if needed.

    After Raymond opened issue bpo-35196, I checked and found several places where the auto-squeeze code was being inefficient. There's a PR ready with those optimizations attached to that issue (GH-10454).

    @terryjreedy
    Copy link
    Member

    The specific bug being fixed here is that wrapped lines before newline are ignored because after

            if s[pos] == '\n':
                linecount += 1
                current_column = 0

    this block

            if current_column > 0:
                lines, column = divmod(current_column - 1, linewidth)
                linecount += lines
                current_column = column + 1

    has no effect. The fix, in the PR, is to put the linecount increment in the \n block before resetting current_column. (Since the column>0 block also has no net effect after \t, I removed it)

    @terryjreedy terryjreedy changed the title IDLE: Squeezed lines count ignores window width IDLE: Squeezed line count ignores wrapping before newline Dec 23, 2018
    @taleinat
    Copy link
    Contributor Author

    New changeset 44a79cc by Tal Einat in branch 'master':
    bpo-35208: Fix IDLE Squeezer line counting (GH-10449)
    44a79cc

    @miss-islington
    Copy link
    Contributor

    New changeset 0e0cc55 by Miss Islington (bot) in branch '3.7':
    bpo-35208: Fix IDLE Squeezer line counting (GH-10449)
    0e0cc55

    @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
    3.7 (EOL) end of life 3.8 only security fixes topic-IDLE type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants