Differential D4947

[KTextEditor] Expose additional internal View's functionality to the public API
ClosedPublic
Actions

Authored by jsalatas on Mar 5 2017, 8:57 PM.

Details

Reviewers
mwolff
tfry
cullmann
Group Reviewers
Frameworks
KTextEditor
Commits
R39:c9cb3e9761f3: Expose additional internal View's functionality to the public API
Summary

I'm creating a diff/patch frontend similar to Kompare but using KTextEditor to display/edit source/destination files. In order to be able to sync the scrolling positions of the two Views and also to overlay diff indicators (see screenshot), I'm exposing the following:

  • setScrollPosition: scrolls the View to a cursor position
  • setHorizontalScrollPosition: scrolls the View to a column
  • maxScrollPosition: returns the cursor position of the maximum vertical scroll position
  • firstDisplayedLine: returns the first visible line in the View
  • lastDisplayedLine: returns the last visible line in the View
  • textAreaRect: returns the View's text area rectangle excluding border, scrollbars, etc.

Test Plan

Already tested in my app. It works :)

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 created this revision.Mar 5 2017, 8:57 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 5 2017, 8:57 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald Transcript
mwolff requested changes to this revision.Mar 5 2017, 10:24 PM
mwolff added a subscriber: mwolff.

now that this API becomes public, it must be improved to make it better understandable to the public

and note that you cannot add new virtual methods to this interface, as it would break the ABI: https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B
You will have to create a separate interface for this, similar to what we have done in the past, with a TODO note that this should be merged in time for KF6.

I'm very much interested in your app btw - will you support embedding it into e.g. KDevelop as a patch viewer? Kompare is pretty much unmaintained, and I like what I see in your screenshot!

src/include/ktexteditor/view.h
464

should be scrollToPos, c -> cursor, and make the cursor const

471

scrollToColumn, x -> column

479

why is this not the document end pos? because of virtual cursors? if so, please rename this to viewEndCursor() or similar, I don't see why this should be called startPos

487

firstVisibleLine

495

lastVisibleLine

502

textAreaRect

src/view/kateview.cpp
2557

here and below: put the { on its own line, like you did above

2575
const auto sourceRect = m_viewInternal->rect();
const auto topLeft = m_viewInternal->mapTo(this, sourceRect.topLeft());
const auto bottomRight = m_viewInternal->mapTo(this, sourceRect.bottomRight());
return {topLeft, bottomRight};
This revision now requires changes to proceed.Mar 5 2017, 10:24 PM
jsalatas added a comment.Mar 5 2017, 11:06 PM
In D4947#92889, @mwolff wrote:

You will have to create a separate interface for this, similar to what we have done in the past, with a TODO note that this should be merged in time for KF6.

You mean an interface similar to TextHintInterface, CodeCompletionInterface, ConfigInterface, AnnotationViewInterface, etc or something else? If something else, could you please provide an example?

I'm very much interested in your app btw - will you support embedding it into e.g. KDevelop as a patch viewer? Kompare is pretty much unmaintained, and I like what I see in your screenshot!

Thanks! Actually it is (obvioulsy) still in a very early stage. I hope initially to finalize it as a standalone app and release its code to the public. After that we will see where it goes, hopefully it will get embedding support at some time.

I'll come back soon regarding the rest of the issues you mentioned.

anthonyfieroni added a subscriber: anthonyfieroni.Mar 6 2017, 5:19 AM

Milian had in mind that you can take charge of Kompare and implement your ideas rather than release new app. :)

jsalatas added a comment.Mar 6 2017, 6:11 AM
In D4947#92995, @anthonyfieroni wrote:

Milian had in mind that you can take charge of Kompare and implement your ideas rather than release new app. :)

lol! OK! Got it! I guess I can do that, although my initial motivation is that I don't like many parts of kompare's GUI, so if I implement my ideas there, it will change drastically and I'm not sure if this would be acceptable by anyone :\

dhaumann added a subscriber: dhaumann.Mar 6 2017, 9:06 AM
dhaumann added inline comments.
src/include/ktexteditor/view.h
464

I would even favour setScrollPosition(). Avoid abbreviations whenever possible. And we already have many other API that contains the full word position.

And should it not be const KTextEditor : : Cursor &?

495

Just to make sure this api is well understood: if code is folded, many hundred lines can be between firstVisibleLine and lastVisibleLine. Please add a comment to the api documentation to reflect this...

dhaumann added a reviewer: tfry.Mar 6 2017, 9:08 AM

Add Thomas, since he iirc also was interested in such an api once.

mwolff added a comment.Mar 6 2017, 1:47 PM
In D4947#92995, @anthonyfieroni wrote:

Milian had in mind that you can take charge of Kompare and implement your ideas rather than release new app. :)

Hihi, while not really my intention, that would of course the best way forward. But @jsalatas, prototyping in a standalone application is just fine. Just please keep this in your mind, that people may actually want to use this functionality from within another application like KDevelop.

Looking at the other comments: Yes, the interfaces you mention are what I had in mind - take them as inspiration on what to do. And @dhaumann, I agree! esp. setScrollPosition is good.

tfry edited edge metadata.Mar 6 2017, 4:54 PM

Hi thanks for adding me in CC. My use case was saving and restoring a certain scroll position. Thus, what I am missing, immediately, is a way to retrieve the current scroll position. The proposed startLine() goes some length, here, but does not provide columns (in case of horizontal scrolling), as far as I can see. On the side of the setter functions, I wonder why there is scroll(To)Column(s), but apparently no equivalent for scrolling to a different line at the current horizontal position.

Thus, I would suggest
a) replacing

int startLine() const Q_DECL_OVERRIDE;

with

KTextEditor::Cursor currentScrollPosition() const Q_DECL_OVERRIDE;

for more flexibility.
b) same probably for endLine(), although I'm not quite clear on the use case for this one.
c) either add scrollToLine(), or remove scrollToColumn() (which could be written as

scrollToPosition(KTextEditor::Cursor(x, currentScrollPosition.line())

, where needed).

jsalatas added a comment.Mar 6 2017, 5:55 PM

Thank you all for your feedback. I'll update soon....

src/include/ktexteditor/view.h
479

This is not the document's end position. It is the max position that the view can scroll vertically.

For example suppose a document with 100 lines viewed in a view that has enough height to display 30 lines. In this case maxStartPos is 70 (the view can scroll down to line 70).

I guess my function naming sucks :)

jsalatas updated this revision to Diff 12253.Mar 7 2017, 5:08 AM
jsalatas edited edge metadata.

Diff updated. Hope I addressed all issues :\

See some additional comments/clarifications below:

In D4947#92889, @mwolff wrote:

should be scrollToPos, c -> cursor, and make the cursor const

No! the internal view's scrollPos (line 180 in src/view/kateview.cpp) modifies the cursor if it is out of scrolling limits.

In D4947#93014, @dhaumann wrote:

And should it not be const KTextEditor::Cursor &?

No! See my comment above to a similar question asked by @mwolff.

In D4947#93173, @tfry wrote:

Thus, I would suggest
a) replacing

int startLine() const Q_DECL_OVERRIDE;

with

KTextEditor::Cursor currentScrollPosition() const Q_DECL_OVERRIDE;

for more flexibility.
b) same probably for endLine(), although I'm not quite clear on the use case for this one.
c) either add scrollToLine(), or remove scrollToColumn() (which could be written as

scrollToPosition(KTextEditor::Cursor(x, currentScrollPosition.line())

, where needed).

@tfry the "problem" here that horizontal scrolling is based on pixels rather than cursors, which is not the case with vertical scrolling.

cullmann added a subscriber: cullmann.Mar 11 2017, 2:05 PM

Hmm, for the interface thing: You could avoid that entirely if you just add the functions as non-virtual ones.
That is now possible in KF5, e.g. see insertTemplate(...) method.

jsalatas updated this revision to Diff 12396.Mar 11 2017, 9:41 PM

Implemented additional functions as non-virtuals, according to @cullmann's suggestion.

It's much better and cleaner this way!

jsalatas updated this revision to Diff 12412.Mar 12 2017, 3:06 PM

in firstVisibleLine() and lastVisibleLine() provide the option for real lines (in cases of folded text).

cullmann added a comment.Mar 12 2017, 3:52 PM

Perhaps we should introduce some enum for real vs. visible lines.
The bool parameter looks not that understandable (yeah, we did that internally already wrong).

dhaumann added a comment.Mar 12 2017, 6:02 PM

Reading this API, I still have some general thoughts:
This is the first time we expose the concept of "visible lines". So far, this only exists internally in katetextfolding.h/cpp. By itself, this is fine, but given we do not expose folding information so far, I think the API is not complete.
If we already provide infos about visible first / last visible line in a view, the complete API should also contain functions like isLineVisible(int line). But here, we have a problem, since the fistVisibleLine() and lastVisibleLine() are not the text folding lines, instead, its the visually visible drawn line on screen. Here, the term "visible" is used in two different contexts: once for screen visibility, and once for folding information. Internally, KTextEditor uses terms viewLine or displayedLine for this. Maybe displayedLine is the best here?

Any comments? Are we lacking more API to make it feel complete?

jsalatas updated this revision to Diff 12416.Mar 12 2017, 9:25 PM
jsalatas edited the summary of this revision. (Show Details)

According to @cullmann's and @dhaumann's feedback created the enum LineType and also renamed firstVisibleLine() and lastVisibleLine() to firstDisplayedLine() and lastDisplayedLine()

jsalatas added a comment.Mar 12 2017, 9:30 PM
In D4947#94562, @dhaumann wrote:

Reading this API, I still have some general thoughts:
This is the first time we expose the concept of "visible lines". So far, this only exists internally in katetextfolding.h/cpp. By itself, this is fine, but given we do not expose folding information so far, I think the API is not complete.
If we already provide infos about visible first / last visible line in a view, the complete API should also contain functions like isLineVisible(int line). But here, we have a problem, since the fistVisibleLine() and lastVisibleLine() are not the text folding lines, instead, its the visually visible drawn line on screen. Here, the term "visible" is used in two different contexts: once for screen visibility, and once for folding information. Internally, KTextEditor uses terms viewLine or displayedLine for this. Maybe displayedLine is the best here?

Any comments? Are we lacking more API to make it feel complete?

Actually I was looking at it yesterday. It seems to be a draft of a FoldingInterface (https://cgit.kde.org/ktexteditor.git/tree/src/draft/foldinginterface.h) which I guess, when completed, will provide a rather extensive (complete ?) folding API.

@cullmann, I'm not sure if you are still working with it's implementation. If this is not the case, I guess I could implement it.

cullmann added a comment.Mar 18 2017, 2:50 PM

Sorry for late response, thanks for ping!

First about the interface you added: I think that it is ok the way, beside for the enum values I would go for CamelCase like all other things and something like VisibleLine or RealLine, that is more KF/Qt style I would say.
If others don't oppose, I would be in favor to merge that then.

For the folding interface: I actually didn't do any work on that since long (nor did Dominik I guess).
Therefore, if you want to have it exposed, feel free too enhance it (might just be non-virtual function addon to view, too, later).
But I think that should go in a second differential item after this stuff is merged.

Actually looking forward to use a new diff tool for testing :P

jsalatas updated this revision to Diff 12598.Mar 18 2017, 7:42 PM
jsalatas added a comment.Mar 18 2017, 7:44 PM
In D4947#95893, @cullmann wrote:

Sorry for late response, thanks for ping!

No worries :)

First about the interface you added: I think that it is ok the way, beside for the enum values I would go for CamelCase like all other things and something like VisibleLine or RealLine, that is more KF/Qt style I would say.
If others don't oppose, I would be in favor to merge that then.

Done

For the folding interface: I actually didn't do any work on that since long (nor did Dominik I guess).
Therefore, if you want to have it exposed, feel free too enhance it (might just be non-virtual function addon to view, too, later).
But I think that should go in a second differential item after this stuff is merged.

OK, I guess I can do it. Will post another differential review when it's done. Hopefully soon :\

cullmann accepted this revision.Mar 19 2017, 10:31 AM

Then I would say that is OK to go in.

jsalatas marked 9 inline comments as done.Mar 19 2017, 11:09 AM
jsalatas marked 2 inline comments as done.
Closed by commit R39:c9cb3e9761f3: Expose additional internal View's functionality to the public API (authored by jsalatas). · Explain WhyMar 19 2017, 11:48 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 12613

View Options

src/include/ktexteditor/view.h

Loading...
View Options

src/utils/ktexteditor.cpp

Loading...
View Options

src/view/kateview.h

Loading...
View Options

src/view/kateview.cpp

Loading...