Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

Hey Thomas, please don't remove the cache. See f2a6941e086cdf506c8fb1798c52982bff43792d for why this was introduced. Your tests don't include other files, so probably that's why you didn't see any effect of the cache?

thanks, lgtm!

Probably not - this is my first commit. Where do I need to apply :-D

Good stuff, do you have push rights?

Upload correct patch

Yeah, you're right.

I think you uploaded the patch against the last version, not against master.

Thanks for the info. I didn't spot it at first since it is hidden in a dropdown menu.

Address Milian's comments; remove m_diagnosticsCache.

@thomassc it is because I commandeered your revision to be able to update it. If you want, you do the same. First select "Commandeer Revision" at the bottom of this page, and then use arc diff (possibly with --update D18567) to update it.

Thanks for the update. Seems good to me.

Work through feedback:

Simplify code.

After finishing initial implementation I found out that QtPromise requires exceptions enabled. KDevelop seem to have exceptions disabled right now.

I will do that.

As long as it integrates well with the rest of the KDevelop build I'm fine. Would probably just copy the QtPromise source tree into maybe kdevelop.git:3rdparty/ and create a CMakeLists.txt which creates a STATIC or OBJECT library for it? QtPromise looks easy enough to build.

yes, QtPromise or AsyncFuture (https://github.com/benlau/asyncfuture) could be used - I wouldn't be opposed to introducing it as a thirdparty dependency (or git submodule)

KAsync looks a bit unmaintained. On the other hand, QtPromise seems to be an active project. Should I start looking into the latter?

if it applies cleanly, you can also push to 5.3, otherwise master is fine - it's not a really urgent bug fix after all (imo)

Onto master?

Looks good to me overall, Let's clean it up so we can get it in.

How it looks.

I don't remember details of this revision, but at least it seems to do like how we agreed.

Remove API changes and just sort plugins list.

Of course, forgot to tell. Now we are in gitlab:
https://invent.kde.org/kde/kdevelop

In D19861#434135, @apol wrote:

Yes, sorry :)

Yes, sorry :)

In D19861#433859, @apol wrote:

Can you send the patch?

Can you send the patch?

Not really: it says that a temporary directory for every kdevelop instance is created

Then the best should be to file a bug on bugzilla.opensuse.org asking for that to be packaged.

In D17760#432630, @lbeltrame wrote:

Is this part of the astyle tarball, or a separate project?

Is this part of the astyle tarball, or a separate project?

In D17289#432577, @rjvbb wrote:

no need for making it "deterministic" in any way. There is no benefit in doing that.

I think there is so, it's the whole point of this PR.

no need for making it "deterministic" in any way. There is no benefit in doing that.

In D17289#432537, @rjvbb wrote:

OK, I stand corrected on this. OTOH, the rest of my notes about this being wrong anyway still stand.

I'm not convinced about those arguments but have no objection either to making the temp dir user-exclusive - it's a detail that should be easy enough. There's probably a reason I'm not using QTemporaryDir though

OK, I stand corrected on this. OTOH, the rest of my notes about this being wrong anyway still stand.

In D17289#432517, @rjvbb wrote:

using the user ID is definitely wrong here: with this change, opening a second kdevelop will erase the temporary directory of the first...

Maybe test-drive the patch (like I have been doing) before advancing hypotheses - don't you think I'd have noticed this kind of astronomically stupid error?
The tmp.dir name also includes the session ID, and you can only open a given session once.

using the user ID is definitely wrong here: with this change, opening a second kdevelop will erase the temporary directory of the first...

In D17760#428926, @arrowd wrote:

In D17760#428860, @cullmann wrote:

Question: does any distro package that?

At least my openSUSE doesn't nor any other distro I know of, only thing I found was:

https://rpmfind.net/linux/rpm2html/search.php?query=libastyle-devel

I skimmed through https://repology.org/project/astyle/packages and found none.

• add some rudimental version detection of the astyle library, since it provides no pkg-config file nor version macros/variables...
• request libastyle >= 3.1

Milian Wolff wrote on 20190312::20:02:54 re: "D17289: KDevelop/Shell: set dedicated TMPDIR"

That's right, thanks a lot!

Re: reparsing reliably each time a headerfile is changed: wouldn't the use of forwarding headers increase the chance of missing a change?

Note: It's Thibault North <thibault.north@gmail.com>

thanks, lgtm - I'll amend the last nits and apply it for you - if you give me full name and email address such that I can set you as the main author of this patch

In D17289#429526, @rjvbb wrote:

And there are probably places where the simpler QProcess API was used
instead.

But the KDevelop env. profiles are already based on or compatible with QProcess::setProcessEnvironment(), no?

Regarding the unit-test, I've seen a similar one at plugins/cmake/tests/manual/parentheses_in_test_arguments/, one could do something like that, but I don't really see here arguments passed to the test are checked... (only had a quick look).

And there are probably places where the simpler QProcess API was used
instead.

Include suggested fixes

ah, then let's get this in as-is

In D17760#428860, @cullmann wrote:

Question: does any distro package that?

At least my openSUSE doesn't nor any other distro I know of, only thing I found was:

https://rpmfind.net/linux/rpm2html/search.php?query=libastyle-devel

great! can we get a unit test for this? though I'm unsure if we ever revived the unit tests from our old cmake integration

Question: does any distro package that?

yes I would be fine with that personally!

In D17289#401264, @rjvbb wrote:

we use CXTranslationUnit_CreatePreambleOnFirstParse to get code completion results fast. otherwise the first code completion request would create the preamble, which felt much worse

Shall we keep that discussion to D18551?

ok cool, then please get rid of our internal copy!

nice, this is getting better! some suggestions on how to improve the code quality, and then some potential issues I can think of - please fix or document why they aren't an issue

cool, this is great! and it fixes the issue you originally found in testActiveDocumentsGetBestPriority?

Rene, instead of thinking about what-ifs, maybe try it out first? Most notably, the tooltips only show up when you press ALT and keep it pressed. Otherwise, the tooltips don't show up - unless you hover code with your mouse cursor. Moving the keyboard edit cursor won't ever trigger tooltips. Or are you somehow moving your mouse cursor while typing?! Don't do that :)

ehhh, damn arc land, I didn't knew it will squash commits, oh well...

FWIW kdev-java was to my knowledge never really working. I think at some point a lot of the code started as effectively a fork of the old C++ plugin, which was then gradually (but not thoroughly) adapted to java ...

Thanks, certainly better than bitrot. I can't really imagine it won't crash with any non-trivial project though :/

Only had a brief look, but this looks good to me! Well, better than letting it bit-rot.

Em ter, 5 de mar de 2019 às 18:36, Dāvis Mosāns <noreply@phabricator.kde.org>
escreveu:

In D19457#422812, @davispuh wrote:

Only I had to disable tests, because they depend on kdevplatform/tests/*.h which doesn't get installed with KDevelop

These headers should normally be installed.

Some distro packages of KDevelop omit them, because of explicitly setting -DBUILD_TESTING=OFF during the build process.

You should be able to copy them manually, or add the source dir as an extra include path.

In D19457#424138, @apol wrote:

Are you planning to maintain it?

Make tests buildable

Nice work!

Are you planning to maintain it?

Only I had to disable tests, because they depend on kdevplatform/tests/*.h which doesn't get installed with KDevelop

Even in your screenshot it looks fine to me.

Personally, I don't think that I share the concerns about large tooltips. Even in your screenshot it looks fine to me. Since as written before, it can be very easily hidden if desired (e.g. just by clicking in the editor text area). I'd much rather have as much relevant information at once instead of having to click another "expand" button each time to see more. But that's just my personal opinion. Maybe others can comment too.

Here's a very good example of an overly large problem report:

Address comments. Now, problems are created for each "requested here" child diagnostic that refers to the current document. This may be more than just the last child diagnostic, since there may be multiple ones referring to the current document.

About the combined popups: could you downsize the 2 components so they always have the same (reasonable) size which could be coupled to screen height?

Omitting the problem tooltip if it's large makes it a bit unpredictable to the user. He/she then won't know in advance whether hovering over a location that might show a combined tooltip will actually show it or not.

Thanks for pointing me to the right places.

Interesting, I didn't know about the mode which highlights problematic lines. Thanks a lot for pointing it out. I had to look for it, and it turned out that this setting is at: Settings dialog -> Language Support -> Semantic Code Highlighting block -> Highlight problematic lines. I can't remember deactivating this, so it's probably off by default? It seems potentially very useful though, given that the underlines are sometimes easy to overlook, and the red line highlighting also shows up in the scrollbar minimap as you write. When this setting is active, the region where the problem tooltip shows up indeed makes more sense to me. It still doesn't match up though, for example if there are some whitespace lines before the line with the error, then the tooltip will also show when hovering somewhere over these whitespace lines.