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

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

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.
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)

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

src/document/katedocument.cpp
3073 ↗(On Diff #48366)

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

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.

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

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.

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
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

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()
  • {
  • }

+ {}
+
};

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

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

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

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...

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?

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.EditedSat, Apr 13, 10:04 AM

5.57 released, should be tried now.

This revision is now accepted and ready to land.Sat, Apr 13, 10:04 AM
This revision was automatically updated to reflect the committed changes.

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.

Didn't try folding :)

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.

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

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

:=) Nice that it helps.