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.

For me it is exactly the opposite. I hover the range that is underlined red

There can be scrollbars in problem reports, see the attached screenshot:

This seems like a big productivity progress (can't test it yet because using the 5.3 branch) but I have a concern that it is only a sufficient solution for those of us with (very) big screens. Esp. problem reports can be long, and I've never seen a scrollbar in them, AFAICR. Another concern is that the combined popup might be so big it's going to obscure parts of the code you'd want to keep visible for context.

use lambda as suggested by Milian

Feel free to push, I can't.

"Hi Friedirch, thanks for your pointer and the question. Though your quick post-commit read missed the fact that the new implementation of openProjectConfig(), while looking shorter, now reuses the new code introduced for the actual purpose of the commit. I am sorry that I missed to properly document this in the commit message that I not only added a new action, but also rewrote as side effect some existing otherwise unrelated code for the purpose of code reuse. I also understand that reasons for code changes should not be buried in long discussions on review boards, but be with the final code result, as no future code reader has time to find and rescan complete review discussions."

This patch also changed the logic of openProjectConfig().

Done, keyboard navigation should work now - at least it does according to my testing! Please test this as well and report back if you spot any issues.

I'm looking into the keyboard navigation now

In D18567#411767, @thomassc wrote:

Regarding waiting for the background parser on TestFile destruction, I didn't find a way to query whether a parse job is running or to wait for it, if it was started externally, using existing code (but I'm also not familiar with the codebase). As an alternative, one can do Q_ASSERT(ICore::self()->languageController()->backgroundParser()->isIdle()); in the test cleanup function to ensure that the tests don't leave the background parser running. This might also be a good idea to ensure that the tests don't influence each other in that way.

Yeah, the keyboard navigation needs some work anyways - it's buggy even without combined tooltips :) Something for the future to work on!

In D11934#410143, @rjvbb wrote:

auto* action = new QAction(tr("Reparse the Entire Project"), this);

Last minute check: tr instead of i18n, really?

using byte arrays is OK imo, but please cleanup the code overall by using a lambda instead of repeating the same thing over and over again

In D18551#411201, @aaronpuchert wrote:

Currently we don't reparse (always?) when an included file changes. That's probably way more annoying than the first code completion having some delay.

In D18551#410223, @aaronpuchert wrote:

In D18551#409551, @mwolff wrote:

When we didn't create the preamble, we will have to reparse the file completely. And this can easily take 1-2s per file, which is *really* annoying.

It only happens on the very first edit though, and will only be noticeable if code completion is required immediately after starting to edit the file. All following parses and code completions can use the precompiled preamble then. I don't think that's too bad. It's also not unusual, many applications have something like a "warmup phase" where not everything is loaded yet.

Sometimes it works, sometimes it doesn't. Often I have to mock-edit a file to force reparsing when some header has changed.

In D18551#411905, @rjvbb wrote:

Indeed, and in that light clang's solution is maybe not that smart, at least not for use as a parser for an entire project. From what I understand that preamble is just a precompiled header file, which in my experience can be tricky to set up (typically require some sort of central header that imports everything that'll be needed everywhere). Having one such preamble per file cannot but duplicate lots of data.

You're right, it's a precompiled header file. I think it works very well for the intended purpose: quick reparsing on most user edits. The additional option that we use here was added in https://reviews.llvm.org/D15490 for pure code completion workloads. I don't think it's meant for us, even though we do completion as well: we also build an index, and have possibly many more files open.

To some extent this resembles the classic latency/throughput discussion.

Regarding keyboard navigation, it does not work for combined tooltips unfortunately. However, I wasn't aware of the existence of that feature at all before you asked about it ...

Regarding waiting for the background parser on TestFile destruction, I didn't find a way to query whether a parse job is running or to wait for it, if it was started externally, using existing code (but I'm also not familiar with the codebase). As an alternative, one can do Q_ASSERT(ICore::self()->languageController()->backgroundParser()->isIdle()); in the test cleanup function to ensure that the tests don't leave the background parser running. This might also be a good idea to ensure that the tests don't influence each other in that way.

In D18551#411309, @rjvbb wrote:

Well, if among developers you cannot find a solution that covers all use cases a configurable setting (applying only to setting the preamble-on-first parse flag or not) could well be the only compromise.

I wouldn't want to give up on a consensus just yet. I think the Clang developers have actually chosen a pretty smart default, and this additional flag incurs costs that are hard to control for hardly any benefit.

Well, if among developers you cannot find a solution that covers all use cases a configurable setting (applying only to setting the preamble-on-first parse flag or not) could well be the only compromise. And this one doesn't have to be hard to understand, which doesn't mean it has to (probably means that it should not) indicate exactly what it does.

This is not something that should be configurable. The follow-up to hard-to-understand options shouldn't be to add more of them.

In D18551#410396, @rjvbb wrote:

Or, you add a flag to one of the relevant settings panels. There are already a number of options that aren't exactly easy to understand for everyone, this one could be something like "Make code-completion results available faster (can require significant resources)".

Or, you add a flag to one of the relevant settings panels. There are already a number of options that aren't exactly easy to understand for everyone, this one could be something like "Make code-completion results available faster (can require significant resources)".

In D18551#409551, @mwolff wrote:

When we didn't create the preamble, we will have to reparse the file completely. And this can easily take 1-2s per file, which is *really* annoying.

It only happens on the very first edit though, and will only be noticeable if code completion is required immediately after starting to edit the file. All following parses and code completions can use the precompiled preamble then. I don't think that's too bad. It's also not unusual, many applications have something like a "warmup phase" where not everything is loaded yet.

auto* action = new QAction(tr("Reparse the Entire Project"), this);

could you fixup the two minor nits please? then you can push directly

In D18551#403490, @aaronpuchert wrote:

I'm fine with that, but would prefer never having it. Not all files that are opened will be edited, and the delay caused by creating the preamble after the first edit is probably negligible over the lifetime of a session. It would also allow us to just drop affected preambles when a header is edited — otherwise we might have to recreate all preambles that include the header.

ping?

ping? I'd really like to see this getting integrated, could you please fix the small issues pointed out by pino and me and resubmit?

The problem with nested event loops are that they introduce execution flows that are very hard to reason about and can easily cause issues like the one you see here. Quite often, you can run into issues where an object spawns a nested eventloop, which then somehow handles a deleteLater event for the object itself, thereby destroying the object. When the nested eventloop exits, this was destroyed and anything can happen. I believe something like that happens here too.

the proper way would be to have Project::open return void, and instead report failure via a new 'failedToOpen' signal. And then internally handle the async code without a nested event loop. We'll probably need this in more places, and I wonder what the current best-practice is for that. Eventually async/await can be used, but we want to have something that works with the compilers we support today. Hm. I'd like to prevent us from trying to handle async code in an adhoc fashion. Rather, we should try to adopt a proper framework like e.g. KAsync? QtPromise? Something similar?

I have little Qt experience, why nested event loops are bad? What are options to avoid them?

I'm afraid to say that this is just a workaround, not a proper fix... I ran into this issue recently too - the real problem - imo - is that we synchronously execute the stat jobs in KDevelop::ProjectPrivate::initProjectFiles... if you look at the ASAN report you'll see that TestCore::shutdown isn't referenced there at all!

No, I was just looking at it. I could remove the QByteArray change and reduce it to online the x.remove() and "else if" parts.

in exchange of much harder to read code.
Was it ever a bottleneck for you?

The QByteArray was already created the same way, now the intermediate object is just cached. But even if that did not change anything it reduces the number of utf8 conversion as only those lines that actually match anything will be converted.

I wonder if that's actually faster. Here we are creating intermediary copies of the QByteArray only to convert to QString.

Question to @apol: from what I can grasp CMakeTargetItem::m_builtUrl is cmake-internal cuisine so there shouldn't be any need to store it in canonical fashion but rather reason(s) to store it exactly as cmake knows it?

In D18551#405494, @rjvbb wrote:

I can't say that it happens systematically but neither that it does NOT happen systematically, and I mean that in general: re-opening a file you just closed.

Isn't this unavoidable BTW, given that we don't end up with preambles of all files?

Commit message ideally. Truncate the report where applicable.

Is the parser triggered again if the entire project has been parsed already?

Oh, sorry, I forgot to press "Submit" on Phab and thought I posted ASan log here.