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

[Dominik Haumann]

dhaumann closed this revision.
dhaumann added a comment.


  Close as it should work now.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D10054

To: dhaumann, cullmann
Cc: rikmills, #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, 
sars, dhaumann

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

[Dominik Haumann]

dhaumann added a comment.


  This is fixed, see commit 
https://phabricator.kde.org/R39:1a38adebb64e6e7d5acb756f68166d56d8ba0b72

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D10054

To: dhaumann, cullmann
Cc: rikmills, #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, 
sars, dhaumann

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

[Christoph Cullmann]

cullmann added a comment.


  No idea ;=)

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D10054

To: dhaumann, cullmann
Cc: rikmills, #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, 
sars, dhaumann

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

[Dominik Haumann]

dhaumann added a comment.


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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D10054

To: dhaumann, cullmann
Cc: rikmills, #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, 
sars, dhaumann

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

[Christoph Cullmann]

cullmann reopened this revision.
cullmann added a comment.
This revision is now accepted and ready to land.


  Then lets reopen it.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D10054

To: dhaumann, cullmann
Cc: rikmills, #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, 
sars, dhaumann

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

[Rik Mills]

rikmills added a comment.


  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: F5707888: testsuite-stdout 

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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D10054

To: dhaumann, cullmann
Cc: rikmills, #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, 
sars, dhaumann

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

[Dominik Haumann]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:2bed94b6d463: Fix: View jumps when Scroll past end of 
document is enabled (authored by dhaumann).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10054?vs=25837&id=25899

REVISION DETAIL
  https://phabricator.kde.org/D10054

AFFECTED FILES
  autotests/src/kateview_test.cpp
  autotests/src/kateview_test.h
  src/view/kateviewinternal.cpp

To: dhaumann, cullmann
Cc: #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann

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

[Christoph Cullmann]

cullmann accepted this revision.
cullmann added a comment.
This revision is now accepted and ready to land.


  Yeah, logic did seem to be flawed.
  To only look at the line makes sense.
  
  +1 for unit test ;=)
  (aka Fleißsternchen)

REPOSITORY
  R39 KTextEditor

BRANCH
  FixScrollPastEnd (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D10054

To: dhaumann, cullmann
Cc: #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann

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

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D10054

To: dhaumann, cullmann
Cc: #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann

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

[Dominik Haumann]

dhaumann updated this revision to Diff 25837.
dhaumann added a comment.


  - Add unit test for scroll past end of document

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10054?vs=25830&id=25837

BRANCH
  FixScrollPastEnd (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D10054

AFFECTED FILES
  autotests/src/kateview_test.cpp
  autotests/src/kateview_test.h
  src/view/kateviewinternal.cpp

To: dhaumann, cullmann
Cc: #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann

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

[Dominik Haumann]

dhaumann edited the summary of this revision.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D10054

To: dhaumann, cullmann
Cc: #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann

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

[Dominik Haumann]

dhaumann created this revision.
dhaumann added a reviewer: cullmann.
Restricted Application added projects: Kate, Frameworks.
Restricted Application added a subscriber: Frameworks.
dhaumann requested review of this revision.

REVISION 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, where the last line
  - make sure the last line is NOT empty
  
  Let's say the last two lines look as follows
  
  yy|yy # '|' denoes the cursor position
  z
  
  Make sure you scrolled past the end of the document (either
  with the mouse or with Ctrl+Down). Note that line 'z' 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.
  
  BIG: 306745
  FIXED-IN: KDE Frameworks 5.43

TEST PLAN
  make test

REPOSITORY
  R39 KTextEditor

BRANCH
  FixScrollPastEnd (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D10054

AFFECTED FILES
  src/view/kateviewinternal.cpp

To: dhaumann, cullmann
Cc: #frameworks, michaelh, kevinapavew, ngraham, demsking, cullmann, sars, 
dhaumann