Differential D4538

[KTextEditor] consistent conversion from/to cursor to/from coordinates
ClosedPublic
Actions

Authored by jsalatas on Feb 9 2017, 10:07 PM.

Details

Reviewers
cullmann
Group Reviewers
Frameworks
KTextEditor
Plasma
Commits
R39:1858d6fafb58: Consistent conversion from/to cursor to/from coordinates
Summary

This patch addresses 3 issues

  1. Currently if you try to convert the cursor to coordinates and then convert these coordinates back to a cursor again, it produces an invalid cursor (-1, -1) when the original cursor is located at the end of line
  1. Converting back to cursor coordinates that have been produced by cursorToCoordinate(view->cursorPosition()) and cursorPositionCoordinates() doesn't always produce the same results as the includeBorder option doesn't seem to be used consistently.
  1. Although I cannot find a test case for this, it doen't seem to make any sense the line scrollPos(max, max.column(), calledExternally); in KateViewInternal::makeVisible function. Was it supposed to be that way (ie force when the last line is not empty and not force when it is), or is it just a typo? :\ I would appreciate any reasoning about this!
Test Plan
  1. Create a new app with a single KTextEditor::View widget (similar to kwrite).
  1. connect the view's cursorPositionChanged signal with a slot in the window which contains the following code
qDebug() << " original cursor" << view->cursorPosition();
QPoint p1 = view->cursorToCoordinate(view->cursorPosition());
KTextEditor::Cursor c1 = view->coordinatesToCursor(p1);
qDebug() << "converted cursor" << c1;
qDebug() << "converted cursor 2" << view->coordinatesToCursor(view->cursorPositionCoordinates());
  1. Run the application and start typing (I typed 52 characters in the first line) and then pressed the <Enter> key sometimes)
  1. Press Back Arrow key until you are in the beginning of the document.

In steps 3 and 4 the three cursors are the same (this wasn't the case before).

Before you get something like the following

original cursor (0, 1)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 2)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 3)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 4)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 5)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 6)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 7)
converted cursor (-1, -1)
......
 original cursor (0, 48)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 49)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 50)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 51)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 52)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (1, 0)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (2, 0)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (3, 0)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
.....
 original cursor (0, 52)
converted cursor (-1, -1)
converted cursor 2 (-1, -1)
 original cursor (0, 51)
converted cursor (0, 51)
converted cursor 2 (-1, -1)
 original cursor (0, 50)
converted cursor (0, 50)
converted cursor 2 (-1, -1)
 original cursor (0, 49)
converted cursor (0, 49)
converted cursor 2 (-1, -1)
 original cursor (0, 48)
converted cursor (0, 48)
converted cursor 2 (-1, -1)
 original cursor (0, 47)
converted cursor (0, 47)
converted cursor 2 (0, 51)
 original cursor (0, 46)
converted cursor (0, 46)
converted cursor 2 (0, 50)
 original cursor (0, 45)
converted cursor (0, 45)
converted cursor 2 (0, 49)
 original cursor (0, 44)
converted cursor (0, 44)
converted cursor 2 (0, 48)
 original cursor (0, 43)
converted cursor (0, 43)
converted cursor 2 (0, 47)
 original cursor (0, 42)
converted cursor (0, 42)
converted cursor 2 (0, 46)
 original cursor (0, 41)
converted cursor (0, 41)
converted cursor 2 (0, 45)
.....
 original cursor (0, 6)
converted cursor (0, 6)
converted cursor 2 (0, 10)
 original cursor (0, 5)
converted cursor (0, 5)
converted cursor 2 (0, 9)
 original cursor (0, 4)
converted cursor (0, 4)
converted cursor 2 (0, 8)
 original cursor (0, 3)
converted cursor (0, 3)
converted cursor 2 (0, 7)
 original cursor (0, 2)
converted cursor (0, 2)
converted cursor 2 (0, 6)
 original cursor (0, 1)
converted cursor (0, 1)
converted cursor 2 (0, 5)
 original cursor (0, 0)
converted cursor (0, 0)
converted cursor 2 (0, 4)

after this patch is applied the converted cursor are consistent with the original (correct) cursor.

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.
jsalatas updated this revision to Diff 11137.Feb 9 2017, 10:07 PM
jsalatas retitled this revision from to [KTextEditor] consistent conversion from/to cursor to/from coordinates.
jsalatas updated this object.
jsalatas edited the test plan for this revision. (Show Details)
jsalatas added reviewers: Frameworks, Plasma.
jsalatas set the repository for this revision to R39 KTextEditor.
Restricted Application added projects: Plasma, Frameworks. · View Herald TranscriptFeb 9 2017, 10:07 PM
Restricted Application added subscribers: kwrite-devel, plasma-devel. · View Herald Transcript
jsalatas updated this object.Feb 10 2017, 12:48 AM
cullmann added a subscriber: cullmann.Feb 18 2017, 4:45 PM

Currently if you try to convert the cursor to coordinates and then convert these coordinates back to a cursor again, it produces an invalid cursor (-1, -1) when the original cursor is located at the end of line
Converting back to cursor coordinates that have been produced by cursorToCoordinate(view->cursorPosition()) and cursorPositionCoordinates() doesn't always produce the same results as the includeBorder option doesn't seem to be used consistently.

Yeah, that seems strange

Although I cannot find a test case for this, it doen't seem to make any sense the line scrollPos(max, max.column(), calledExternally); in KateViewInternal::makeVisible function. Was it supposed to be that way (ie force when the last line is not empty and not force when it is), or is it just a typo? :\ I would appreciate any reasoning about this!

Hmm, I doubt anybody can remember why that is that way ;=)

Could your create some minimal unit test to ensure this stays consistent?

brauch added a subscriber: brauch.Feb 18 2017, 6:51 PM

Looks sensible to me, but can you check that it doesn't break KDevelop's navigation widget? I think that is the most visible use case for that interface right now.

jsalatas updated this revision to Diff 11494.Feb 19 2017, 4:17 AM
  1. I abandoned the 3rd issue I mention in the summary about the line scrollPos(max, max.column(), calledExternally); as I could neither verify nor disprove if this was intended or not.
  1. In cursorToCoordinate() I added an extra check to see if we are behind end of line and return and invalid point (-1, -1):
if (cursor.column() > layout.lineLayout().textLength()) {
    return QPoint(-1, -1);
}

as according to QT's documentation (http://doc.qt.io/qt-5/qtextline.html#cursorToX)

If cursorPos is not a valid cursor position, the nearest valid cursor position will be used instead, and cursorPos will be modified to point to this valid cursor position.

and without this extra check, the "behind end of line should give an invalid cursor" tests in kateview_test.cpp would fail.

  1. I also added the relevant tests to kateview_test.cpp, as per cullmann's feedback.
  1. KDevelop seems to work.

I have checked it and I didn't see any issues to the Navigation Widget popup tooltips which indeed seems to extensively use coversions between cursors and coordinates, as mentioned by brauch. Furthermore:

  • cursorPositionCoordinates isn't used in kdevelop and kdevplatform, so the 2nd issue described in the summary shouldn't affect kdevelop and kdevplatform in any way.
  • 1st issue in the summary is related to cursors/positions at the end of line, so my intuition suggests that it wouldn't be related to any context related to kdevelop. :)
cullmann accepted this revision.Feb 19 2017, 12:48 PM

Thanks for extending the test case + trying the KDevelop navigation!

This revision is now accepted and ready to land.Feb 19 2017, 12:48 PM
Closed by commit R39:1858d6fafb58: Consistent conversion from/to cursor to/from coordinates (authored by jsalatas). · Explain WhyFeb 19 2017, 7:16 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 11507

View Options

autotests/src/kateview_test.cpp

Loading...
View Options

src/view/kateview.cpp

Loading...
View Options

src/view/kateviewinternal.cpp

Loading...