Differential D17857

DocumentPrivate: Don't jump view when edit using 'scroll past last line'
ClosedPublic
Actions

Authored by loh.tar on Dec 29 2018, 6:42 PM.

Details

Reviewers
cullmann
dhaumann
Group Reviewers
KTextEditor
Commits
R39:a9f97f1be115: avoid view jump to bottom on folding changes
R39:65e3b5462f1c: DocumentPrivate: Don't scroll view when add auto-bracket at end of file
R39:cfb0af25bdfa: ViewInternal: Fix makeVisible(..)
Summary

...when 'Allow scrolling past the end of the document' is set

BUG: 399014
BUG: 306745
FIXED-IN: 5.57

Diff Detail

Repository
R39 KTextEditor
Lint
Lint Skipped
Unit
Unit Tests Skipped
loh.tar created this revision.Dec 29 2018, 6:42 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptDec 29 2018, 6:42 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Dec 29 2018, 6:42 PM
loh.tar retitled this revision from DocumentPrivate: Don't scroll view when add auto-bracket at aned of file to DocumentPrivate: Don't scroll view when add auto-bracket at end of file.Dec 30 2018, 8:59 AM
loh.tar edited the test plan for this revision. (Show Details)
loh.tar added a comment.Dec 30 2018, 9:03 AM

As always poor tested and only in "normal mode", not vi

src/document/katedocument.cpp
3073

Perhaps should that line not removed but out-commentend with a note why

dhaumann added a subscriber: dhaumann.Dec 30 2018, 9:49 AM

Is this related to bug https://bugs.kde.org/show_bug.cgi?id=306745 ? I once tried to fix 306745 but had to revert due to a regression, see D10054.

loh.tar added a comment.Dec 30 2018, 10:17 AM
In D17857#383865, @dhaumann wrote:

Is this related to bug https://bugs.kde.org/show_bug.cgi?id=306745 ?

I don't think so. Here is it auto-bracket stuff, there general "is visible" stuff :-) (Yeah, my English rocks!)

cullmann added a subscriber: cullmann.Jan 20 2019, 12:21 PM

Extra setCursor calls are always fishy.

But on the other side: how does one reproduce this?

For me, if I scroll behind the last line, close to any editing action triggers scrolling, with or without this patch.

loh.tar added a comment.Jan 21 2019, 5:28 AM

But on the other side: how does one reproduce this?

Hm, I had just follow the descriptions in the report.

cullmann accepted this revision.Mar 4 2019, 7:06 PM

I can not reproduce this, but I think one setCursorPosition less is always good :P

This revision is now accepted and ready to land.Mar 4 2019, 7:06 PM
Closed by commit R39:65e3b5462f1c: DocumentPrivate: Don't scroll view when add auto-bracket at end of file (authored by loh.tar, committed by cullmann). · Explain WhyMar 4 2019, 7:07 PM
This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Mar 4 2019, 7:15 PM
cullmann requested changes to this revision.Mar 4 2019, 7:16 PM
This revision now requires changes to proceed.Mar 4 2019, 7:16 PM
cullmann added a comment.Mar 4 2019, 7:16 PM

Diffs like:

  • /usr/home/jenkins/workspace/Frameworks/ktexteditor/kf5-qt5 FreeBSDQt5.12/autotests/input/indent/cstyle/openpar12/expected 2019-03-04 16:49:58.559736000 +0000

+++ /usr/home/jenkins/workspace/Frameworks/ktexteditor/kf5-qt5 FreeBSDQt5.12/autotests/input/indent/cstyle/openpar12/actual 2019-03-04 16:51:44.182941000 +0000
@@ -1,7 +1,6 @@
class X
{

void foo()
  • {
  • }

+ {}
+
};

loh.tar added a comment.Mar 5 2019, 6:09 AM

Um, this patch sucks, sorry! Will try again or re-open bug report :-/

loh.tar updated this revision to Diff 53213.Mar 5 2019, 4:50 PM
loh.tar retitled this revision from DocumentPrivate: Don't scroll view when add auto-bracket at end of file to DocumentPrivate: Don't jump view when edit using 'scroll past last line'.
loh.tar edited the summary of this revision. (Show Details)
loh.tar edited the test plan for this revision. (Show Details)
  • Fix 399014 in a way that should be reviewed!
  • Fix also 306745
  • Autotest looks good
loh.tar added inline comments.Mar 5 2019, 4:52 PM
src/view/kateview.cpp
1463 ↗(On Diff #53213)

That was needed to fix the auto bracket bug, looks somehow dangerous

cullmann added a reviewer: dhaumann.Mar 5 2019, 8:37 PM

Perhaps Dominik has some idea if that is right or not ;=)

loh.tar updated this revision to Diff 53252.Mar 6 2019, 5:07 AM
loh.tar set the repository for this revision to R39 KTextEditor.
  • Enable ViewTest: ScrollPastEndOfDocument
********* Start testing of KateViewTest *********
Config: Using QtTest library 5.12.1, Qt 5.12.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 8.2.1 20181127)
PASS   : KateViewTest::initTestCase()
PASS   : KateViewTest::testScrollPastEndOfDocument()
PASS   : KateViewTest::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 162ms
********* Finished testing of KateViewTest *********

This test could be improved to by doing some text input and also with enabled dyn wrap

dhaumann added a comment.Mar 12 2019, 9:22 PM

This needs a more detailed review, and currently I find only very few time :( in general, I once tried to fix this bug, but couldn't find a fix. Now you found one - but given this is a very central place I think this needs to be thought through complete prior to committing.

Also, this needs a lot of manual testing with dynamic word wrap on and off...

loh.tar added a comment.Mar 13 2019, 5:54 AM

currently I find only very few time :(

I see, sorry to bother you...once more

Perhaps Dominik has some idea if that is right or not ;=)

The biggest question here is these forced center call around 1463. Can you remember why/when this is needed/useful?

cullmann added a comment.Mar 31 2019, 10:21 AM

I think nobody has any idea if that is the right fix.
My proposal would be: we wait until 5.57 is out in ~one week.
Afterwards we apply this and give it some testing in master.
If the world doesn't break, this can stay in, else we can revert it before 5.58.
Would that be acceptable?

cullmann accepted this revision.EditedApr 13 2019, 10:04 AM

5.57 released, should be tried now.

This revision is now accepted and ready to land.Apr 13 2019, 10:04 AM
Closed by commit R39:cfb0af25bdfa: ViewInternal: Fix makeVisible(..) (authored by loh.tar). · Explain WhyApr 13 2019, 2:20 PM
This revision was automatically updated to reflect the committed changes.
loh.tar added a comment.Apr 14 2019, 7:13 AM

Did anyone tried this patch?
While coding my focus was only on behavior while enter text. Now I notice that the folding is "broken" in a way that the view jumps to the bottom :-S

First digging indicate that makeVisible(..) is called twice, and the second time reported cache()->displayViewLine(..) "out of view" which is pretty surprising. The given cursor position is the same.
Will look at this later this day.

cullmann added a comment.Apr 14 2019, 7:45 AM

Didn't try folding :)

loh.tar added a comment.Apr 14 2019, 11:53 AM

Looks to me a call to KateLayoutCache::updateViewCache may help at some point.
But have no idea where to place and with which parms.
In KateLayoutCache::displayViewLine is "limit=0" on second call.

cullmann added a comment.Apr 14 2019, 12:18 PM

Ok, I can reproduce the issue you mention.
I will take a look.

loh.tar added a comment.Apr 14 2019, 1:11 PM

Yes! A short test looked very promising.
Will play a little more, because had done some changes to this patch here :-/

cullmann added a comment.Apr 14 2019, 1:14 PM

:=) Nice that it helps.

cullmann added a comment.Jul 7 2019, 12:07 PM

Git commit 49b8add426a252afc1976188baebde194d75ced2 by Christoph Cullmann.
Committed on 07/07/2019 at 12:04.
Pushed by cullmann into branch 'master'.

fix goto line centering
force center for any external set cursor position call

M +1 -1 src/view/kateview.cpp

https://commits.kde.org/ktexteditor/49b8add426a252afc1976188baebde194d75ced2

Revision Contents
Changeset List

PathPackages
Msrc/document/katedocument.cpp (1 line)

Diff 48366

View Options

src/document/katedocument.cpp

Loading...