For me it is exactly the opposite. I hover the range that is underlined red
- Queries
- All Stories
- Search
- Advanced Search
Advanced Search
Feb 23 2019
There can be scrollbars in problem reports, see the attached screenshot:
Feb 22 2019
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.
Feb 21 2019
Feb 20 2019
Feb 19 2019
use lambda as suggested by Milian
Feel free to push, I can't.
Feb 17 2019
"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().
Feb 16 2019
Feb 15 2019
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.
Feb 14 2019
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.
Feb 13 2019
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)".
Feb 12 2019
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)".
Feb 11 2019
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
Feb 10 2019
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.
Feb 9 2019
Feb 8 2019
Feb 7 2019
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?
Feb 6 2019
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?
Feb 5 2019
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.
please still attach the full ASAN report to your commit - i.e. put it into your commit message
In D18551#404528, @rjvbb wrote:preambles are not created during the initial import except possibly for the files that opened with the session if the parser is triggered twice for those.
Feb 4 2019
Comment removed.
Feb 3 2019
No, sorry, I am a bit confused by your notation and the fact you seem to consider only files that are open when you start a session and then probably confused myself a bit more while answering.
Remove code used for description.
In D18551#404077, @rjvbb wrote:If line 3 means "files opened after (1)" then yes, that's probably correct.
Feb 2 2019
where (1) = entire project is parsed on import. Is that correct?
The way I understand it, the proposals are slightly different. Maybe we can summarize them as follows:
Is Milian's suggestion different from mine of setting the flag only when projects are not parsed entirely on import (which may be easier to test for where the flag is set, I haven't checked)?
Feb 1 2019
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.
never seen this, please investigate and fix it
But for some reason I'm not getting any tooltips in that reporter at all at the moment.
What I meant is that tooltips in the problem reporter appear when you hover the mouse over an entry, and disappear again when you move the mouse back to the text editor (and certainly when you need to scroll the editor window because the tooltip covers the area where you need to be). But for some reason I'm not getting any tooltips in that reporter at all at the moment.
again: why don't we keep this flag when the file is opened in the editor?
please show the full ASAN report, but the second change looks fine to me - if we get an event during destruction of the tool view action, m_dock may be gone already and thus accessing it may be broken already
tooltips don't take keyboard focus, so I don't understand your first note. also the tooltip only shows up (in the editor) while you keep ALT pressed.
Jan 31 2019
In D18551#402186, @brauch wrote:To clarify, the patch back then was less about the first-time delay, and more about the delay being there *every* time because of wrong usage of the API. Whether the delay occurs once is arguably a bit less important.
Jan 30 2019
So there should indeed be some real-world benchmarking data to do a cost/benefit analysis that includes the human-in-the-loop aspect and huge projects like the ones Aaron uses. We already have an estimated 10% gain for a full reparse of a small project after its initial import.
Jan 29 2019
To clarify, the patch back then was less about the first-time delay, and more about the delay being there *every* time because of wrong usage of the API. Whether the delay occurs once is arguably a bit less important.
In D18551#401594, @rjvbb wrote:Aaron, don't your arguments apply to parsing an entire project on startup as well? I can easily imagine that opening and parsing entire projects of the size you mention must have a non-negligible cost regardless of all optimisation and caching that is currently implemented
It definitely does. Sometimes I just cancel it, but sometimes I need the entire index to understand some code. (“Find uses” is very useful for that.)