Fix Screen::scrollUp() behavior

Review Request #130133 - Created May 16, 2017 and updated

Information
Octoploid Octoploid
konsole
master
379318
Reviewers
konsole
hindenburg

Patch from Nicholas Marriott (of tmux fame).
See: https://github.com/tmux/tmux/issues/931

Currently Screen::scrollUp() is broken and cannot be used by tmux.
("set -as terminal-overrides ',*:indn@'" in .tmux.conf is a workaround)

Fix the issue by copying the checks in Screen::scrollDown().

Tested with tmux trunk.

Issues

  • 0
  • 0
  • 1
  • 1
Description From Last Updated
Octoploid Octoploid
James Pike
Kurt Hindenburg
Octoploid Octoploid
Review request changed

Change Summary:

Fix off by one issue

Diff:

Revision 2 (+8 -3)

Show changes

Anthony Fieroni

   
src/Screen.cpp (Diff revision 2)
 
 

Does this should be _columns not _columns-1 ?

  1. Has anyone had time to work on this more? The upcoming 17.08 is near and I'd like to get this fixed by than.

  2. The current patch works fine for me (I use konsole with tmux daily at work).
    So I'm not sure what more you are asking for.
    The patch just needs to be reviewed and then applied.

  3. I'm not sure where lastScrolledRegion is used for, but _bottomMargin is +1, look 2 lines above, so logically here should be without -1, no ?

  4. Yes, you are right. But the problem is that I see no change in behavior, when I change _lastScrolledRegion as you have suggested. So either I don't trigger this code path in my testing, or it is neutral.

  5. Please investigate, if lastScrolledRegion is unused, it should be removed or corrected, it does not need to leave untouch incomplete features.

  6. OK, lastScrolledRegion is actually used (in TerminalDisplay::updateImage()). 
    But the width of _lastScrolledRegion doesn't matter.
    It could be any value >=1. (It cannot be 0, because otherwise region.isValid() would be false in TerminalDisplay::scrollImage)
    So changing _columns-1 to _columns make absolutely no difference.
Loading...