- User Since
- Apr 16 2015, 7:53 PM (200 w, 6 d)
Tue, Feb 19
we override execution in our own completion models, so this patch will only change the behavior for the builtin word and keyword completion models in ktexteditor I believe
Mon, Feb 18
Fri, Feb 15
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
Yeah, the keyboard navigation needs some work anyways - it's buggy even without combined tooltips :) Something for the future to work on!
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
Mon, Feb 11
could you fixup the two minor nits please? then you can push directly
Sun, Feb 10
ping? I think it would still make sense to merge this - Aleix, are you still running this? Should we revive the patch, rebase it and get it in?
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'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!
Fri, Feb 8
pushed this now with a proper benchmark too, shows a ~10x performance win when a non-empty PATH is set
Tue, Feb 5
please still attach the full ASAN report to your commit - i.e. put it into your commit message
I'm in favor!
Fri, Feb 1
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.
Wed, Jan 30
I see that it's faster when I profile kate/kdev, but I cannot easily write a benchmark for this. It's only noticeable when the KDE_COLOR_SCHEME_PATH variable is set, otherwise the global application config will be used afte rall, which is going to be shared most probably. Any idea how I could construct a valid KDE_COLOR_SCHEME_PATH path to e.g. the breeze color scheme from within a kcolor scheme auto test?
Tue, Jan 29
you can pick whatever you prefer for the reviews for now.
Mon, Jan 28
do we have custom patches in our libastyle? I hope not, but it's been too long for me to remember. If not, then I guess we should get rid of our copy of lbiastyle and just disable kdevastyle if that dependency wasn't found - it's pretty optional anyways.
yes, push to 5.3 and we'll merge it to master
lgtm now, sorry for the delay - can you push that yourself? if not, please tell us your email address and name to associate with the commit
it was measured back then:
Sun, Jan 27
we use CXTranslationUnit_CreatePreambleOnFirstParse to get code completion results fast. otherwise the first code completion request would create the preamble, which felt much worse
please paste compiler errors instead of showing screenshots
sorry for the long delay. personally, I like what I'm seeing in the video, could you cleanup the patch a little please? and does ensureVisible help maybe?
sorry for the long delay, it sounds like a good idea but the diff is super hard to review since it contains lots of unrelated whitespace changes
great initiative, but could you check if this could be done in a more generic way potentially? if not then I'm all for getting this in as-is otherwise
Sat, Jan 26
lgtm now, note that you now need to push to the new gitlab remote at firstname.lastname@example.org:kde/kdevelop.git
lgtm, thanks. note that you now need to push to the new gitlab remote (email@example.com:kde/kdevelop.git) cf. https://invent.kde.org/kde/kdevelop for future reviews
since noone else is using this plugin, I'd say let's push this! @wcancino do you have a developer account? if not, then we can commit this for you. Please then give us your email address to associate with the commit then.
@thomassc: A separate context could help, yes. But if you find alternative ways to handle it, like proposed here, then not using a separate context could potentially work out too. But it feels a bit hackish to handle the template args specially everywhere instead of putting them into a proper context.
finally got it working, man that old code was broken :D I wrote most of that a really long time ago, and it shows!
sadly my initial attempt doesn't work either ;-) I'm looking at this some more now and will let you know once I've found an alternative approach