Differential D16203

Context browser: fix handleRect for non-symbol tooltips
ClosedPublic
Actions

Authored by kossebau on Oct 14 2018, 5:34 PM.

Details

Reviewers

kfunk

Group Reviewers

KDevelop

Commits

R32:27c15b1809f1: Context browser: fix handleRect for non-symbol tooltips

Summary

The ContextBrowserPlugin::showToolTip() tried to calculate the handle
rect for the tooltip in all cases from DUChainUtils::itemUnderCursor(),
which currently in case of no symbol repots a valid empty range at
position 0,0.

This results in bogus active zones for the tooltips for problem tooltips,
but also languages where the language support plugin has many items not
in the DUChain (like the CMake support plugin).

This patches fixes this by
a) changing ILanguageSupport::specialLanguageObjectNavigationWidget()
to report not only the widget but also which document range the object
covers for which the widget was created.
b) collect the ranges from all places from which the tooltip widget is
fetched

Test Plan

Tooltips for problems and cmake symbols behave now similar to those for e.g.
C++ symbols. E.g.. tooltips for items at the bottom of the screen, where the
tooltip is shown above, the tooltip now can be entered by the mouse and does
not disappear on a move.
C++ language tooltips work as before.

Diff Detail

Repository

R32 KDevelop

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

kossebau created this revision.Oct 14 2018, 5:34 PM

Restricted Application added a project: KDevelop. · View Herald TranscriptOct 14 2018, 5:34 PM

Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript

kossebau requested review of this revision.Oct 14 2018, 5:34 PM

Harbormaster completed remote builds in B3870: Diff 43605.Oct 14 2018, 5:34 PM

kossebau mentioned this in D16211: Adapt to ILanguageSupport::specialLanguageObjectNavigationWidget sig change.Oct 14 2018, 7:36 PM

kossebau added a dependent revision: D16211: Adapt to ILanguageSupport::specialLanguageObjectNavigationWidget sig change.Oct 14 2018, 7:36 PM

I think the change conceptually makes sense, and I am subconsciously aware of the bug it is trying to fix. If you say it works for you, it should be fine.

It is a binary- and source-incompatible change for plugins though. Did you check whether the potentially affected plugins need source changes?

It's not great style to do such a change so shortly before the release from the release management point of view, but since realistically there exist no external plugins, we probably shouldn't worry too much about that ...

plugins/contextbrowser/contextbrowser.cpp
556–557	In the API, you return a tuple for this use case, here you use a return argument instead. I think you should return a tuple here as well.

In D16203#344770, @brauch wrote:

I think the change conceptually makes sense, and I am subconsciously aware of the bug it is trying to fix. If you say it works for you, it should be fine.

Used in my work version since the WE and so far saw no issues (with C++ and CMake language plugins).

It is a binary- and source-incompatible change for plugins though. Did you check whether the potentially affected plugins need source changes?

From the currently released ones, kdev-php needs adaption: D16211 is up for that. Then there is kdev-css from playground, which I would patch directly.

It's not great style to do such a change so shortly before the release from the release management point of view, but since realistically there exist no external plugins, we probably shouldn't worry too much about that ...

Agreed. This should be an exception, and I only proposed to do it since the bug sometimes gets so much into the face. Even more now that I know the details and have my work version patched with this fix :)

kossebau marked an inline comment as done.Oct 17 2018, 4:45 PM

kossebau added inline comments.

plugins/contextbrowser/contextbrowser.cpp
556–557	Proposed to do that in the public API to follow the rest of the methods there. Personally not yet a fan of tuples, at least the QPair ones with their semantically poor `first` and `second` member names, so avoided to do that here. But given you asked, I tried a variant using also QPair for this method here, not sure I prefer it. Updating the patch next with that to give you an idea and something to choose from :)

Use QPair also with ContextBrowserPlugin::navigationWidgetForPosition

Harbormaster completed remote builds in B3990: Diff 43803.Oct 17 2018, 4:45 PM

I'm not a huge fan of tuples either, for the reason you mentioned; I usually just create a two-element struct instead. In this case I think it's fairly obvious what the elements do from the types, though; the QPair<int, int> is worse ;)
Up to you how you do this, I just wanted to point out that it is done differently in two similar places.

Looks ok to me, if Kevin doesn't have objections to putting this into 1.3, go ahead ;)

Thank you for your work!

In D16203#344806, @brauch wrote:

I'm not a huge fan of tuples either, for the reason you mentioned; I usually just create a two-element struct instead. In this case I think it's fairly obvious what the elements do from the types, though; the QPair<int, int> is worse ;)

And then there is also the current(?) overhead in C++ to destruct the returned tuple on the caller side, where one would like to reference a tuple element as variable a few more times.
Not sure if it was Rust, but some language I once played with made on the fly-created tuples a very sane thing to use, especially returning them from methods (giving better clue to code reader what values are really returned to the caller). But in C++ I have yet to learn if newer standards improved things syntactically :)

Up to you how you do this, I just wanted to point out that it is done differently in two similar places.

Would then prefer the first version. Also because it is less code.

Looks ok to me, if Kevin doesn't have objections to putting this into 1.3, go ahead ;)

@kfunk Having my big "Push" button polished now and wired up to this patch (and the related kdev-php one), just waiting for your okay then :)

Thank you for your work!

Thank you for review work :)

Go for it. I don't have an opinion on whether to use version 1 or 2 from an API point of view, both dont look entirely clean to me.

A custom struct instead of the QPair<>, returned everywhere, would be best; but I can live with v1.

Let's go for 5.3.

This revision is now accepted and ready to land.Oct 18 2018, 5:35 AM

Closed by commit R32:27c15b1809f1: Context browser: fix handleRect for non-symbol tooltips (authored by kossebau). · Explain WhyOct 18 2018, 10:29 AM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			kdevplatform/language/interfaces/ilanguagesupport.h (5 lines)
M			kdevplatform/language/interfaces/ilanguagesupport.cpp (5 lines)
M			plugins/clang/clangsupport.h (3 lines)
M			plugins/clang/clangsupport.cpp (11 lines)
M			plugins/cmake/cmakemanager.h (8 lines)
M			plugins/cmake/cmakemanager.cpp (42 lines)
M			plugins/contextbrowser/contextbrowser.h (3 lines)
M			plugins/contextbrowser/contextbrowser.cpp (65 lines)
M			plugins/qmljs/kdevqmljsplugin.h (2 lines)
M			plugins/qmljs/kdevqmljsplugin.cpp (10 lines)
M			plugins/quickopen/quickopenplugin.cpp (2 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	43605	29b5f14		Oct 14 2018, 5:34 PM	★	★
Diff 2	43803	27cdd3a	Use QPair also with ContextBrowserPlugin::navigationWidgetForPosition	Oct 17 2018, 4:45 PM	★	★
Diff 3	43856	27eaf33	R32:27c15b1809f15c7029757575ef19283d916e8671	Oct 18 2018, 10:28 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
7498e4757371	c8edb2130e9b	27cdd3acf098	Friedrich W. H. Kossebau	Context browser: fix handleRect for non-symbol tooltips (Show More…)	Oct 14 2018, 5:30 PM

	Status	Author	Revision
	Closed	kossebau	D16211 Adapt to ILanguageSupport::specialLanguageObjectNavigationWidget sig change
	Closed	kossebau	D16203 Context browser: fix handleRect for non-symbol tooltips