Add VcsAnnotationItemDelegate, for control of rendering and tooltip
Needs ReviewPublic

Authored by kossebau on Nov 8 2017, 12:37 AM.

Details

Reviewers
kfunk
Summary

Uses upcoming KTextEditor::AbstractAnnotationItemDelegate

While staying in the boundaries of line-based rendering,
the delegate takes advantage of knowledge about grouped
lines for the same annotation, and does not repeat the main
info, but writes a secondary info on a second line (taking into
account visibility) and keeps further ones blank.

It also applies an ActiveToolTip for the tooltip on the annotation,
to allow copying data and in the future clicking links in the shown
commit message.

Diff Detail

Repository
R32 KDevelop
Branch
addAnnotationItemDelegate
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2613
Build 2631: arc lint + arc unit
kossebau created this revision.Nov 8 2017, 12:37 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptNov 8 2017, 12:37 AM
apol added a subscriber: apol.Nov 8 2017, 11:30 AM

LGTM. I assume the hover information stays the same?

kdevplatform/vcs/CMakeLists.txt
49

I wouldn't compile it conditionally. Let's just bump the frameworks dependency once this is in...

Thanks for first feedback. Still need to write some more detailed introduction notes, though, hopefully get to it tonight.

In D8709#165678, @apol wrote:

LGTM. I assume the hover information stays the same?

Yes, currently tooltip and annotation group highlighting on hovering are as before, the latter even fixed a little :)

My plan (and initial motivation for the delegate) though is to get the tooltip to show the complete commit message, not just the first line.
Ideally even as active tooltip and with contained links or keywords available as active links.

So the use-case of "hm, why was this change done, what has been discussed about it at the review" no longer needs thousands of UI interactions, but just a hover and clicking on the links in the tooltip. Still some way to go for this, but the delegate is a first step to that goal. Sadly the delegate needs to be done properly, due to the ABI needs in KTextEditor. But such is life. And as side-effect we already get some slightly more useful annotation sidebar, at least I already started to like the current UI proposal over the existing.

kdevplatform/vcs/CMakeLists.txt
49

Sure about this? Developers on rolling releases might have no problems with such a dependency bump, anyone else though would, no? :)

brauch added a subscriber: brauch.Nov 8 2017, 11:53 AM

Looks cool! The panel is extremely wide, maybe we want it to be a bit less wide ...?

Re. KF5 version, please don't hard-bump the requirement ... requiring KTextEditor from tomorrow is really a burden. This feature isn't essential enough to make that necessary. All the windows builds will break, and it will be ages of work to get them properly updated to current KF5.

Looks cool! The panel is extremely wide, maybe we want it to be a bit less wide ...?

Which width to use seems a challenge. No good idea yet, see first item in "Yet to be solved:" and the comments in VcsAnnotationItemDelegate::sizeHint().

There are surely different use-cases where different width are useful:
So far my main testing use-case has been: understanding history of some code, and there the more I could see from the first line, the better, no need to go hunting for the tooltip. (And yes, I have a tiny laptop screen, 1366 px wide :) ). So goal of the width is: no need for tooltip, thus wide is better.
But if using the annotation bar together with tool views on both sides, too little source code text is seen (with my screen), so there it fails.

Sometimes one might be only interested in the grouping of committed lines, where the content on the annotation bar is more used as id to see if the lines belong to the same. There a short width, with unique enough content would be sufficient.

To be discussed. At least the delegate will allow to adjust to our needs, without having to go back to change KTextEditor API (ideally).

Random thought, not sure if useful/technically feasible: the border could also behave like a splitter, allowing to resize it.

Random thought, not sure if useful/technically feasible: the border could also behave like a splitter, allowing to resize it.

Sounds useful to me, given the annotation content has no preknown sizes, so personal customization like in similar situations would be handy (I know I had the impulse to resize things a few times already).
When doing the delegate API, I had such a potential feature in mind. The delegate/KDevelop code here is prepared for it, it just renders into the rect it is told to :)

Should be implementable by adding the needed code to the KateView helper elements. Not on my personal TODO plan though.

Still does not solve the challenge for a sane default width. But would at least allow to tune things to one's need (ideally the width being remembered per view, also across session reload).

40 characters or 25% of the view's width, whichever is smaller?

apol added inline comments.Nov 8 2017, 3:27 PM
kdevplatform/vcs/CMakeLists.txt
49

The rest can use the appimage or the flatpak or snap or docker or create their own packaging system...

IMHO

brauch added inline comments.Nov 8 2017, 4:30 PM
kdevplatform/vcs/CMakeLists.txt
49

Sorry Aleix, I'm strongly against this. Requiring KF5 5.40 effectively means nobody using ubuntu will be able to self-compile KDevelop easily before mid-next-year, it means we have to rebuild our windows dependency chain (which in the past always turned out to be at least a day's worth of fiddling), and so on. It's simply not worth it. Let's keep the ifdef and wait for more things depending on later KF5 to come up before we bump the requirement.

kossebau updated this revision to Diff 22230.Nov 12 2017, 2:01 PM

Update to latest API of KTextEditor patch

kossebau updated this revision to Diff 40991.Tue, Sep 4, 3:45 PM

update to latest master

kfunk requested changes to this revision.Tue, Sep 4, 4:53 PM
kfunk added a subscriber: kfunk.
kfunk added inline comments.
kdevplatform/vcs/CMakeLists.txt
49

+1 on Sven's remarks.

@kossebau Why's that still commented?

kdevplatform/vcs/vcspluginhelper.cpp
401

Or simply:

#if KTEXTEDITOR_VERSION > QT_VERSION_CHECK(5,40,0)
...

in this file. /me doesn't like #ifdefs, #ifs are easier to track since you're getting compiler warnings when the define gets lost for whatever reason.

This revision now requires changes to proceed.Tue, Sep 4, 4:53 PM
apol added inline comments.Tue, Sep 4, 4:58 PM
kdevplatform/vcs/CMakeLists.txt
49

xD in fairness, v5.40.0 nowadays is older than the last Ubuntu LTS release and it should be fine to have it included there unconditionally.

kossebau added a comment.EditedTue, Sep 4, 5:54 PM

Oops, thanks for your new comments, developer mates :), but did not consider an update would trigger you to look at it again. This was not the intention, but to save the current state, while I give further work a try.

re: version. The needed changes are not yet in KTextEditor, so any if version will be in some future KDE Frameworks one, 5.51 or later.

Still some nuts to crack before I can remove the [WIP]...

So please give the Summary text a look and add some comments to those items, what you think can be done about them.

kossebau updated this revision to Diff 41113.Thu, Sep 6, 3:18 PM
  • limit size of annotation border to 25 % of view (other values)

Needs latest version of D8708. Version checks commented out for simplicity
of usage (as they would return false for now, being a future version).

Would be happy to have people apply the patches and give them a try,
screenshot only tells so little.

kossebau updated this revision to Diff 41228.EditedSat, Sep 8, 9:20 PM

Reaching review candidate state, please start reviewing the code.

I am still a bit unsure about the API, and given this is 5.4 material I
propose to give things some more time and only push the base work to
KTextEditor in October, once 5.51 is tagged.
So we have 4 weeks of API/code review now, and then 4 weeks of testing by
those people running master of KTextEditor and KDevelop.

For some more verbose presentation of this patch see https://frinring.wordpress.com/2018/09/09/from-code-to-related-bug-report-or-review-in-just-a-hover-click/

kossebau retitled this revision from [WIP] Add VcsAnnotationItemDelegate, for control of rendering, menus and tooltip to Add VcsAnnotationItemDelegate, for control of rendering and tooltip.Sat, Sep 8, 9:29 PM
kossebau edited the summary of this revision. (Show Details)
croick added a subscriber: croick.Fri, Sep 14, 10:32 PM

I like it so far. The additional information is certainly helpful when trying to understand the history of some piece of code.

  • The behaviour of the ToolTip is better now. However, if the commit message is too short, it's sometimes not possible to reach "Select All" in the context menu of the ToolTip. But that's rather an issue with the ActiveToolTip vanishing immediatly once it's left.
  • If I try to "Show Diff..." of any annotation, KDevelop will crash:
#6  0x00007f1e74ff3d37 in KTextEditor::ViewPrivate::uniformAnnotationItemSizes() const (this=0x562425e5c750) at /home/christoph/Software/kde/frameworks/ktexteditor/src/view/kateviewhelpers.h:299
#7  0x00007f1e75016b63 in KateIconBorder::calcAnnotationBorderWidth() (this=this@entry=0x56242e3eb9f0) at /home/christoph/Software/kde/frameworks/ktexteditor/src/view/kateviewhelpers.cpp:2722
#8  0x00007f1e75016ca0 in KateIconBorder::updateFont() (this=0x56242e3eb9f0) at /home/christoph/Software/kde/frameworks/ktexteditor/src/view/kateviewhelpers.cpp:1631
#9  0x00007f1e7501b092 in KateIconBorder::KateIconBorder(KateViewInternal*, QWidget*) (this=0x56242e3eb9f0, internalView=<optimized out>, parent=<optimized out>) at /home/christoph/Software/kde/frameworks/ktexteditor/src/view/kateviewhelpers.cpp:1458
#10 0x00007f1e75005f00 in KateViewInternal::KateViewInternal(KTextEditor::ViewPrivate*) (this=0x56242a732190, view=<optimized out>) at /home/christoph/Software/kde/frameworks/ktexteditor/src/view/kateviewinternal.cpp:233
#11 0x00007f1e74ffe904 in KTextEditor::ViewPrivate::ViewPrivate(KTextEditor::DocumentPrivate*, QWidget*, KTextEditor::MainWindow*) (this=0x562425e5c750, doc=0x562424247cc0, parent=<optimized out>, mainWindow=0x562423ba63f0) at /home/christoph/Software/kde/frameworks/ktexteditor/src/document/katedocument.h:1049
#12 0x00007f1e74f77f58 in KTextEditor::DocumentPrivate::createView(QWidget*, KTextEditor::MainWindow*) (this=this@entry=0x562424247cc0, parent=parent@entry=0x56242dc32310, mainWindow=0x562423ba63f0) at /home/christoph/Software/kde/frameworks/ktexteditor/src/document/katedocument.cpp:417
#13 0x00007f1e764f5207 in KDevelop::TextDocument::createViewWidget(QWidget*) (this=0x5624259b14c0, parent=0x56242dc32310) at /usr/include/c++/8.2.1/bits/atomic_base.h:390
#14 0x00007f1e764f4225 in KDevelop::TextView::createWidget(QWidget*) (this=0x56242635b410, parent=0x56242dc32310) at /home/christoph/Software/kde/extragear/kdevelop/kdevelop/kdevplatform/shell/textdocument.cpp:565
kdevplatform/vcs/widgets/vcsannotationitemdelegate.cpp
388

On a small screen, that's taking a little too much space in my opinion. I preferred 20%.

What would be necessary to make it resizable?

I like it so far. The additional information is certainly helpful when trying to understand the history of some piece of code.

Thanks for testing :)

  • The behaviour of the ToolTip is better now. However, if the commit message is too short, it's sometimes not possible to reach "Select All" in the context menu of the ToolTip. But that's rather an issue with the ActiveToolTip vanishing immediatly once it's left.

Yes, seen that as well. Not yet sure what to do here.

  • If I try to "Show Diff..." of any annotation, KDevelop will crash:

Eek, I will have a look tomorrow, never experienced that.

kdevplatform/vcs/widgets/vcsannotationitemdelegate.cpp
388

Myself I would actually prefer 30 % :) Guess that should be a value users want to customize in the general kdevelop settings. Will extend the patch to propose to add that.

For making it interactively resizeable, that will need some extension of KateView in KTextEditor. Have not yet thought further about this and how it should be as user experience and how it could be implemented

  • If I try to "Show Diff..." of any annotation, KDevelop will crash:

Eek, I will have a look tomorrow, never experienced that.

Confirmed, should be fixed now with D8708#326397