Changed by andrew_b (@aborodin) on Apr 27, 2017 at 12:55 UTC

Replying to lastique (#3817):

4. At some point mcview stops scrolling as if the list of symbols ended. Typically this happens when the last symbol is very long (maybe longer than the console width; I don't know the exact criteria).

The exact criteria is 8K bytes. You can see that in the top-right corner as 8192/8192+.

Changed by andrew_b (@aborodin) on Apr 28, 2017 at 6:57 UTC (comment 2)

• Cc set to egmont

This behavior is not occured in text wrap mode.

I think the problem is in mcview_display_line (src/viewer/ascii.c:593) which is called in mcview_ascii_move_down (src/viewer/ascii.c:936). Probably, we should call mcview_may_still_grow() in ascii.c:593.

Egmont, do you have any idea?

Changed by egmont (@egmontkob) on Apr 28, 2017 at 13:54 UTC (comment 3)

Will take a look soon, hopefully tonight. Thanks for looping me in! I could reproduce the problem. Do we know if it was my big rewrite that broke it? I'll check it.

I sometimes had a feeling that search was not reading the entire output, but, my bad, never cared enough to track down and file a bugreport. Could be caused by the same issue.

Changed by egmont (@egmontkob) on Apr 28, 2017 at 22:28 UTC (comment 4)

It was indeed broken by me with the big viewer rewrite (#3250, bcb09f6).

Changed by egmont (@egmontkob) on Apr 28, 2017 at 23:18 UTC (comment 5)

Trying to understand what's going on, to come up with the right fix:

mcview_display_line() contains two branches commented as "Optimization: Fast forward to the end of the line, rather than carefully parsing and then not actually displaying it."

This method does not only display the requested row of the screen, but also updates the state from where the next visible row can be painted. The state contains file offset, plus other attributes (e.g. nroff color) that we might need to carry.

In wrap mode, we need to stop right after displaying the visible row, there we have the state from where displaying the next visible row will continue. There's nothing else to do.

In nowrap mode, we need to skip to the end of the file's line. This is what the optimization branches do. This optimization could be cut off, in that case the "normal" body of the method would walk through the characters one by one, adjusting the state machine (e.g. tracking nroff changes etc.), just to drop this whole thing at the end (since no formatting state is carried across newlines).

A workaround for the current problem is to disable the ~10 lines of code at the top of this method commented as "Optimization....".

The core of the problem is that the "normal" path calls mcview_next_combining_char_sequence() which eventually uses mcview_get_byte() or mcview_get_utf() which take care of extending the growbuf if necessary. The shortcut optimized path calls mcview_eol() instead which does not. In fact, eventually it also calls mcview_get_byte(), however, beforehand it explicitly bails out if the end of the current buffer is reached, without trying to grow it.

Hence I'm wondering if the correct solution would be to fix mcview_eol() to grow if necessary.

This might fix some other weird corner cases as well. E.g. when the actual viewer area is only a single row high and you press Ctrl+E to get to the end of that line. (The bug here would be more prominent if Ctrl+E right-aligned the last visible row rather than the first. Then I guess we could spot a bug with sane window sizes as well.)

andrew_b, what do you think, does this approach sound reasonable to you?

Changed by andrew_b (@aborodin) on Apr 29, 2017 at 16:00 UTC (comment 5.6)

Replying to egmont:

andrew_b, what do you think, does this approach sound reasonable to you?

Yes, it is reasonable for me, but I'm not so deeply in the viewer code.

Changed by egmont (@egmontkob) on Apr 29, 2017 at 19:36 UTC (comment 7)

(Forget the paragraph about other bugs/corner cases, it was stupid.)

Changed by egmont (@egmontkob) on Apr 30, 2017 at 19:56 UTC

• Attachment mc-3817-mcview-eol.patch (raw) added

Fix

Changed by egmont (@egmontkob) on Apr 30, 2017 at 19:58 UTC (comment 8)

Could you guys please test this patch?

I haven't paid too much attention to possible side effects and haven't tested it thoroughly.

Also note that this patch does _not_ fix the search issue I've referred to, that seems to be a different bug. I've reported that as #3819.

Changed by lastique (andrey.semashev@….com) on May 3, 2017 at 19:25 UTC (comment 9)

I patched the 4.8.18 that is shipped with my Ubuntu 17.04 and a quick test shows the problem is fixed. I'll keep using the patched version and see if there are any problems.

Thanks guys.

Changed by andrew_b (@aborodin) on May 4, 2017 at 7:21 UTC (comment 10)

• Milestone changed from Future Releases to 4.8.20
• Owner set to andrew_b
• Votes set to andrew_b
• Branch state changed from no branch to on review
• Status changed from new to accepted

Branch: 3817_mcview_grow_buffer
[8d25e94bd59cde4b534ce6136e2eeb37c26dbe7d]

(I have some problem with correct commit comment.)

Changed by andrew_b (@aborodin) on May 17, 2017 at 10:22 UTC (comment 11)

• Branch state changed from on review to approved

Changed by andrew_b (@aborodin) on May 17, 2017 at 10:27 UTC (comment 12)

• Branch state changed from approved to merged
• Resolution set to fixed
• Status changed from accepted to testing
• Votes changed from andrew_b to committed-master

Merged to master: [71c8f5b].

Changed by andrew_b (@aborodin) on May 17, 2017 at 10:28 UTC (comment 13)

• Status changed from testing to closed