Fix: View jumps when Scroll past end of document is enabled
ClosedPublic

Authored by dhaumann on Jan 23 2018, 4:31 PM.

Details

Summary

This patch fixes a corner case for the following setup:

  • enable "[x] Scroll past end of document"
  • disable dynamic word wrap (i.e. you see a horizontal scrollbar for long lines)
  • open a document with several lines
  • make sure the last line is NOT empty

Let's say the last two lines look as follows

yy|yy # '|' denoes the cursor position
zzzzz

Make sure you scrolled past the end of the document (either
with the mouse or with Ctrl+Down). Note that line 'zzzzz' is
completely visible.

Now press 'cursor down'.

What happens is that the view contents jumps and the scrolling
behavior acts as if "Scroll past end of document" is not enabled.

Expected behavior is that the cursor position goes one line down,
but the scroll position remains completely unchanged.

The bug here is the following if clause:

} else if (c > viewLineOffset(startPos(), linesDisplayed() - m_minLinesVisible - 1)) {
    KTextEditor::Cursor scroll = viewLineOffset(c, -(linesDisplayed() - m_minLinesVisible - 1));
    scrollPos(scroll, false, calledExternally);
}

In the buggy case, c==(28, 1), and viewLineOffset()==(28,0).
This triggers the bug that in the last line of the document for
columns > 0 the scroll position is adapted.

The proposed fix here is to not compare cursor positions, but only
the lines. Clearly, 28 < 28 is not true, leading to no change in
the scroll position.

BUG: 306745
FIXED-IN: KDE Frameworks 5.43

Test Plan

make test

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dhaumann created this revision.Jan 23 2018, 4:31 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptJan 23 2018, 4:31 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
dhaumann requested review of this revision.Jan 23 2018, 4:31 PM
dhaumann edited the summary of this revision. (Show Details)Jan 23 2018, 4:34 PM
dhaumann updated this revision to Diff 25837.Jan 23 2018, 5:45 PM
  • Add unit test for scroll past end of document
ngraham edited the summary of this revision. (Show Details)Jan 23 2018, 6:13 PM
cullmann accepted this revision.Jan 23 2018, 10:56 PM

Yeah, logic did seem to be flawed.
To only look at the line makes sense.

+1 for unit test ;=)
(aka Fleißsternchen)

This revision is now accepted and ready to land.Jan 23 2018, 10:56 PM
This revision was automatically updated to reflect the committed changes.

This change appears to cause ktexteditor to fail it's vimode_completion autotests.

From KDE CI: https://build.kde.org/job/Frameworks%20ktexteditor%20kf5-qt5%20SUSEQt5.10/35/testReport/junit/(root)/TestSuite/vimode_completion/

Ubuntu autotests:

Reported as bug https://bugs.kde.org/show_bug.cgi?id=390333 as this currently blocks 5.43 from getting into Ubuntu.

cullmann reopened this revision.Feb 13 2018, 7:58 AM

Then lets reopen it.

This revision is now accepted and ready to land.Feb 13 2018, 7:58 AM

Question is: what happens in the vi test? Did we really introduce a regression or is the test wrong?

dhaumann closed this revision.Feb 13 2018, 9:58 PM

Close as it should work now.