bugGNU nano - Bugs: bug #50621, in current git, an overlong line...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #50621: in current git, an overlong line can cause a Badness warning

Submitter:  Benno Schulenberg <bens>
Submitted:  Thu 23 Mar 2017 08:38:08 PM UTC
   
 
Severity:  3 - Normal Status:  Fixed
Assigned to:  dolorous Open/Closed:  Closed

Jump to the original submission

Tue 28 Mar 2017 06:23:07 PM UTC, comment #28: 

No problem.  And yes, that's the right fix; good catch.

David Lawrence Ramsey <dolorous>
Group Member
Tue 28 Mar 2017 05:49:10 PM UTC, comment #27: 

Ah.  Okay.  Thanks.  Applied to git, 93c62489.

In the subsequent commit I've also corrected a computation.  I couldn't find anything that went wrong because of it, but... I'm pretty sure the strnlenpt() was incorrect.

Benno Schulenberg <bens>
Group administrator
Tue 28 Mar 2017 01:50:16 PM UTC, comment #26: 

Here's a fix.  In partial-fix-4a, I forgot to handle the case where the line at the top of the screen was edittop; the compensation isn't needed in that case because of firstcolumn.

(file #40181)

David Lawrence Ramsey <dolorous>
Group Member
Tue 28 Mar 2017 12:57:26 PM UTC, comment #25: 

And scrolling either up or down means either with the arrow keys, or Meta-=/Meta--.

David Lawrence Ramsey <dolorous>
Group Member
Tue 28 Mar 2017 12:25:22 PM UTC, comment #24: 

No problem.  But this isn't quite fixed yet.  In an 80x24 screen, with current git, and with your original lotsofexes file, run:

nano --ignore --softwrap lotsofexes

and you'll still get badness warnings when scrolling either up or down.  And adding --smooth to the above options doesn't change that.

David Lawrence Ramsey <dolorous>
Group Member
Tue 28 Mar 2017 10:42:03 AM UTC, comment #23: 

Fixed in git, 66ef8f45.  Thanks for the patches.

Benno Schulenberg <bens>
Group administrator
Mon 27 Mar 2017 11:06:54 PM UTC, comment #22: 

Oops, make that partial fix 4a (relative to 4, there's a better comment, and softwrap-specific calculations are only done in softwrap mode).

(file #40176)

David Lawrence Ramsey <dolorous>
Group Member
Mon 27 Mar 2017 10:48:47 PM UTC, comment #21: 

And here's partial fix 4, which seems to fix the other display issues (except for some display issues with the mark, but that's a separate bug).

(file #40175)

David Lawrence Ramsey <dolorous>
Group Member
Mon 27 Mar 2017 08:50:46 PM UTC, comment #20: 

I'm looking into it.

However, the fix for bug #50653, commit 244a503, only fixes the problem for Up and Down; in smooth scrolling mode, PageUp and PageDown preserve placewewant the same way Up and Down do, so they need an equivalent fix.  The attached patch should provide it.

(file #40173)

David Lawrence Ramsey <dolorous>
Group Member
Mon 27 Mar 2017 06:39:09 PM UTC, comment #19: 

(For cross reference: the Badness that was reported in comment #9 has been rereported as bug #50657, and has been fixed in git -- with a mistake in the commit message: the scrolling directions are reversed.)

Benno Schulenberg <bens>
Group administrator
Mon 27 Mar 2017 04:00:00 PM UTC, comment #18: 

Please have a look at current git and at bug #50653.  I think it might fix the redraw problems you are seeing with your time-and-magix text (when combined with your partial-3 fix).

(For cross reference: the bug where PageUp failed to redraw the top line has been reported as bug #50645, and has been fixed.)

Benno Schulenberg <bens>
Group administrator
Mon 27 Mar 2017 12:59:17 AM UTC, comment #17: 

Addendum: The change is PageDown behavior is a change from nano's previous behavior, but it seems to be exactly what Pico does (as far as I can tell; since Pico obviously lacks smooth scrolling, I have to add lines until Pico's screen centering behavior lines things up the right way).  But this is off-topic.

David Lawrence Ramsey <dolorous>
Group Member
Sun 26 Mar 2017 11:42:02 PM UTC, comment #16: 

It turns out that the top line not changing in paragraph.txt is a bug in the PageUp/PageDown functions regarding the top line's being scrolled off the screen.  I'll report it separately, and I have a fix for it.

Having PageDown work when it's at the bottom of the file is a separate bug.

David Lawrence Ramsey <dolorous>
Group Member
Sun 26 Mar 2017 08:52:07 PM UTC, comment #15: 

If it stops at a nonzero value, it won't update any softwrapped line.  I figured that the nrows count should be decremented by the number of rows update_line() returns, not by one for each row (a holdover from before softwrap mode).  Here's a patch which adds that.

However, there's still one issue with it: it occasionally will not draw the top line when moving up, or the bottom line when moving down.  The former case I can reproduce with paragraph.txt.  Run:

nano +12 --ignore --softwrap paragraph.txt

Press PageDown, so that the screen scrolls (it shouldn't do this, since it's at the bottom of the file, but that's a separate bug I'll have to report, assuming it isn't caused by the patch in some way), and then press PageUp.  The cursor will be placed back at the top of the file, but the top line won't change until you press PageUp again (Ctrl-L won't work for some reason).

The latter case I can only reproduce so far with the overly large file.

(file #40153)

David Lawrence Ramsey <dolorous>
Group Member
Sun 26 Mar 2017 08:28:07 PM UTC, comment #14: 

The for loop needs to stop as soon as update_line() returns a nonzero value.

Benno Schulenberg <bens>
Group administrator
Sun 26 Mar 2017 08:02:53 PM UTC, comment #13: 

Pushing the starting row one step back when scrolling forward should be covered by the nrows increase; if nrows is increased, then the go_forward_chunks() call moves one less row forward.  As for backward, that's a good point.  The approach here should probably change to accommodate both possibilities.  (I think adding 2 to nrows seemed to cover both possibilities before, which is why it was in there before.)

Also, some of the comments in edit_scroll() refer to scrolling backward or forward, while other comments refer to scrolling up or down; that should probably be fixed to avoid confusion.

David Lawrence Ramsey <dolorous>
Group Member
Sun 26 Mar 2017 06:22:01 PM UTC, comment #12: 

Without your patch applied and with your recipe of 'nano +5,161 --ignore --softwrap paragraph.txt', typing ten times <Down> and then ten times <Up>, I get three warnings:

Badness: tried to display a chunk on row 21
Badness: tried to display a chunk on row 22
Badness: tried to display a chunk on row 26

Using +9,161 instead, and typing twelve times <Up>, I get just:

Badness: tried to display a chunk on row 21

After applying your patch, the warning for the +9,161 case is gone.  For the +5,161 case, just this warning remains:

Badness: tried to display a chunk on row 21

One thing that I find odd is that the comments speak of a row /before/ the scrolled region.  But that is true only when scrolling forward.  When scrolling backward, it should redraw the row or line directly /after/ the scrolled region.  Also, when wanting to draw also the line /before/ the scrolled region, it seems to me it is not enough to simply increase the number of rows that need to be drawn; it should also push the starting row or line one step back -- but I don't see that anywhere.

Benno Schulenberg <bens>
Group administrator
Sun 26 Mar 2017 06:13:54 PM UTC, comment #11: 

And even with the patch applied, there are still warnings with your lotsofexes file in the first post (they might be caused by the same issues as with the particular file mentioned in the previous post; I don't know yet).  Open an 80x24 terminal, and run

nano --ignore --softwrap lotsofexes

Once the file is loaded, press the down arrow until you scroll.  Every time you scroll, the warning will pop up.

There's another issue.  Opening an 80x24 terminal, and run

nano --ignore --softwrap --smooth lotsofexes

Once the file is loaded, press Ctrl-^ to turn the mark on, and press the down arrow until you scroll.  Every time you scroll with the mark on, the warning will pop up.

In short, non-smooth scrolling triggers it upon scrolling whether the mark is on or not, and smooth scrolling triggers it upon scrolling only when the mark is on.

David Lawrence Ramsey <dolorous>
Group Member
Sun 26 Mar 2017 03:31:03 PM UTC, comment #10: 

Thanks.  I've melded your work with mine into the attached patch.

However, that's not the only badness warning that remains.  With the attached file paragraph.txt, in an 80x24 terminal, try running:

nano +5,161 --ignore --softwrap paragraph.txt

Move down with the arrow keys until you scroll (you'll end up on line 9, column 161), and then move up with the arrow keys until you scroll back again.  When you scroll back again, that's when the warning happens.  Oddly enough, starting nano with +9,161 and scrolling up from there doesn't trigger the warning.

And the warning can still be triggered with the mark on when scrolling down with a particular file with long lines, but there are two problems: copyright applies to the file in question, and the file is about 50k (uncompressed, anyway).  I can't figure out what's triggering the warning, or I'd come up with a smaller test case.  I suppose I could email it to you privately?

(file #40148, file #40149)

David Lawrence Ramsey <dolorous>
Group Member
Sun 26 Mar 2017 11:42:00 AM UTC, comment #9: 

The only Badness warning that remains is when doing:

  stty cols 60 && src/nano --ignore +,77 NEWS

and then typing M-=.

Benno Schulenberg <bens>
Group administrator
Sun 26 Mar 2017 11:24:39 AM UTC, comment #8: 

It was unclear how exactly scrolling down was broken by your partial fix... until I realized it was scrolling up (in my parlance) that was broken.

Well, the latter is easily fixed by deleting the optimization twenty lines down, commenting out this block:

//     if (!ISSET(SOFTWRAP) &&
//             ((i == 1 && direction == UPWARD) ||
//             (i == nrows && direction == DOWNWARD))) {
//         if (line_needs_update(openfile->placewewant, 0))
//             update_line(line, (line == openfile->current) ?
//                     openfile->current_x : 0);
//     } else

So... it looks like an effective patch will delete a whole bunch lines.  Yippie.  :)

Benno Schulenberg <bens>
Group administrator
Sat 25 Mar 2017 06:06:30 AM UTC, comment #7: 

And the partial fix breaks non-softwrap mode when scrolling down with the mark off.  Stumped again.

David Lawrence Ramsey <dolorous>
Group Member
Sat 25 Mar 2017 06:02:37 AM UTC, comment #6: 

Here's a patch containing a partial fix, which should also make that code chunk a bit clearer.  With the patch applied, nano now only redraws the row before the scrolled region if the mark is on or if it's not on the first "page", as intended.

This change means that, when running your test in softwrap mode, the badness error will now only occur if the mark is on, instead of all the time.  (Obviously, it shouldn't be occurring at all, but if it's occurring less often, I'd call that progress.)

(file #40124)

David Lawrence Ramsey <dolorous>
Group Member
Sat 25 Mar 2017 03:08:34 AM UTC, comment #5: 

And the previous comment's references to the mark also apply to horizontally scrolled lines.  If you're not on the first "page" and scroll three rows up, you need to redraw the fourth row at the top to account for that too, since current has changed.  Otherwise, the line you started on will still be drawn as horizontally scrolled even though it no longer is.

David Lawrence Ramsey <dolorous>
Group Member
Sat 25 Mar 2017 12:22:27 AM UTC, comment #4: 

Well... I originally wrote edit_scroll() ages ago, I didn't make the comment specific enough, and I can't remember offhand why it's necessary, hence my grasping at straws in an attempt to fix it.

Although I can answer one of your questions.  In your hypothetical example, if the cursor is in the middle of the line, you turn the mark on, and then move one character left (so that the mark covers one character in the middle of the line), and then you scroll three rows up, you need to redraw the three rows at the top with new content.

But, since the ending mark position has changed due to scrolling three rows up, you need to redraw the fourth row at the top to account for that, since that's the row the mark starts on.  Otherwise, all text to the left of the original marked character will not have the mark properly displayed.  Ditto for scrolling down.

Now that I think about it, it may not be necessary to add 2 when nrows is not 1; just adding 1 in all cases may be enough.  However, that will need testing to be certain it's correct.  And if you want to adjust that comment to something more to your liking, go ahead.

David Lawrence Ramsey <dolorous>
Group Member
Fri 24 Mar 2017 08:56:33 PM UTC, comment #3: 

[I don't understand that comment that you're changing with your last patch.  How can the rows before /and/ after the scrolled region be both on the screen?  Say we scrolled three lines backwards, then all the rows in the edit window have been moved three rows down, and the three rows at the top need to be redrawn with new content.  (And the fourth row from the top needs to be redrawn... why exactly?)  Sure, there is a line /after/ the scrolled region that is visible on screen ((when scrolling backward), but never any line before it.  Mutatis mutandis for scrolling forward.  So... why does it add 2 when nrows is not 1?]

Benno Schulenberg <bens>
Group administrator
Thu 23 Mar 2017 09:23:35 PM UTC, comment #2: 

Never mind the last patch: the code is needed not just for horizontally scrolled lines, but also marked ones, so it breaks display of marked text.

The attached patch still improves the comment associated with that code to mention this.  I'm still reasonably sure that the code is what's causing the problem, but I don't know how to change it yet without breaking something else.

(file #40103)

David Lawrence Ramsey <dolorous>
Group Member
Thu 23 Mar 2017 09:00:46 PM UTC, comment #1: 

I think I know what's causing this: some code in edit_scroll() that's being run unconditionally when it should only be run in non-softwrap mode (since it's supposed to cover horizontally scrolled lines, which don't apply in softwrap mode).  The attached patch against current git seems to fix it.

(file #40102)

David Lawrence Ramsey <dolorous>
Group Member
Thu 23 Mar 2017 08:38:08 PM UTC, original submission:  

To reproduce, first do:

  for x in $(seq 2001); do echo -n "xx "; done >lotsofexes

then run:

  stty cols 80 && stty rows 24 && src/nano --ignore --soft --smooth --nowrap lotsofexes

Then type <PageDown> <PageUp>.  No problem.  Type M-/  M-\.  No problem.  Type <Down> a few times, then <Up>.  No problem.  Now type M-=.  Beep!  "Badness: tried to display a chunk on row 75".

When you keep typing M-=, the row number counts down.  For some reason it tries to display a chunk that is one chunk beyond the last chunk of the softwrapped line.  Somehow it tries to display one more chunk than the softwrapped line actually has.

Benno Schulenberg <bens>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dolorous (Updated the item)
  • -email is unavailable- added by bens (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 15 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-03-28 dolorous Attached File- Added 0001-display-don-t-draw-more-edittop-chunks-than-the-scre.patch, #40181
    2017-03-28 bens StatusIn Progress Fixed
        Assigned toNone dolorous
        Open/ClosedOpen Closed
    2017-03-27 dolorous Attached File- Added partial-fix-4a.patch, #40176
    2017-03-27 dolorous Attached File- Added partial-fix-4.patch, #40175
    2017-03-27 dolorous Attached File- Added paging-fix.patch, #40173
    2017-03-27 bens Summaryin current git, an extremely long line can cause a Badness warning in current git, an overlong line can cause a Badness warning
    2017-03-26 dolorous Attached File- Added partial-fix-3.patch, #40153
    2017-03-26 bens StatusNone In Progress
    2017-03-26 dolorous Attached File- Added partial-fix-2.patch, #40148
        Attached File- Added paragraph.txt, #40149
    2017-03-25 dolorous Attached File- Added partial-badness-fix.patch, #40124
    2017-03-23 dolorous Attached File- Added 0001-tweaks-improve-comment-in-edit_scroll.patch, #40103
    2017-03-23 dolorous Attached File- Added 0001-display-don-t-handle-horizontally-scrolled-lines-in-.patch, #40102

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code