- User Since
- Jan 25 2016, 10:25 AM (95 w, 2 d)
Hmm, I'm not sure. The thing is, while it is of course an extremely niche feature in 2017, maybe we should look if it is easily fixable. Because *if* you are in the unfortunate situation that you have to interact with a CVS repo, I think you are all the happier if your IDE takes the pain of reading the CVS manual away from you ...
Mon, Nov 20
Sorry, forgot to mention the review in the commit message ...
Do we want this in 5.2?
No, Queued means they are queued in the receiving object's thread's event loop, i.e. the slots are executed in that thread when it re-enters its event loop. Direct means the slots are called immediately, in the sender's thread. There are no other connection types.
It's not "asynchronous and complex", it's just a queued connection. It posts an event to the receiving thread, and when that thread next runs its event loop, the slot gets invoked. As Milian says, it's not rocket science.
Sun, Nov 19
Ah yes, good point about Q_SIGNALS, makes sense.
I'll accept this too so milian is happy about the green tickmark :)
Makes sense to me. Thanks for working on this issue!
Can it happen that resume() is called before ThreadWeaver has finished suspending, causing it not to resume? Otherwise LGTM
I'd propose something like "attempting to resume background parser, but it is not suspended" as the warning text, if I see "not suspended" in the output in a year I won't know what it's about
don't see anything wrong
Technically it can fail, I think e.g. if ICore::self()->projectController() is null, but that doesn't happen of course. LGTM
Ah, alright. Thanks!
Very good to have somebody working on those dialogs, they really need some love. Thanks!
Fri, Nov 17
Yes, makes sense to me.
Ok, REVIEW: doesn't close reviews ... submitted with a2712c8d969137 to 5.2. Thanks for the comments.
Thu, Nov 16
+1 from me, but let apol review it ;)
Wed, Nov 15
I understand what the patch does, but can you explain why this fixes your issue?
Wed, Nov 8
Good find, please submit to 5.2 :)
40 characters or 25% of the view's width, whichever is smaller?
Random thought, not sure if useful/technically feasible: the border could also behave like a splitter, allowing to resize it.
Looks cool! The panel is extremely wide, maybe we want it to be a bit less wide ...?
Tue, Nov 7
Looks good, thanks!
Sun, Nov 5
Fri, Nov 3
No, no objections, it doesn't really do any harm. Thanks!
Wed, Nov 1
In general looks like a good idea and I admire your persistence, but do we really want 3k lines of code of tests testing copy constructors? Does that add value? It seems like it just makes any future changes harder ... It only tests that the compiler-generated code actually does what the compiler claims it will do, no? ;)
Oct 22 2017
Yeah, the problem with kate plugins is that we have no way to distribute them. We don't have infrastructure for distributing binary plugins, and we don't really support script plugins, and nobody installs your plugin if you put it on github with compile instructions. I'm not sure what that means for this case, I'll leave that decision to the maintainers I think.
Hmm, I'm not sure -- this is quite a bit of code for a feature I personally wouldn't know how to make use of. Yes, I sometimes want to perform this action, but really, I just press Ctrl+C, Ctr+N, Ctrl+V and I have achieved the same thing -- and it's much easier to remember than another shortcut for specifically this ...
Oct 17 2017
Oct 16 2017
Yes, at least the ones I'm aware of. Thanks for the review, I'll push it in a moment.
Oct 12 2017
Oct 6 2017
Future tags won't drop the v, because all KDE apps always have the v there.
- This only works on UNIX
- This adds a hard build-time-dependency on git, doesn't it?
Oct 4 2017
I grepped around and I a) can see why not setting this key breaks things and b) don't see why it would be wrong. I think it's just an oversight that it wasn't. Thank you!
Sep 30 2017
It's a hack but meh, why not. It's not like the "sort selection" script wouldn't have the same problem. While you're at it, probably #ifdef that one as well ...
Sep 29 2017
Sep 28 2017
I also think by the way that this shouldn't go to 5.2, sorry ... 5.2 is fragile enough as it is on the platforms we already claim to be stable on. Let's put it in master and instead try to keep the time to 5.3 short-ish.
Sep 26 2017
Looks good to me, thanks! Maybe wait for Milian to approve as well.
My position stays the same: are we really in a situation in 2017 where, from the kernel POV, we cannot get notified of changed files in a directory tree without 30+ seconds setup cost? Only if that is the case (and I'm quite sceptical) I'd say this approach is a good idea.
Otherwise, looks good to me, thanks a lot!
Sep 25 2017
I was experimenting with a similar patch locally and yes, I think this should be done on the higher layer as well. Otherwise, you need the same patch for e.g. qmake.
Sep 23 2017
Sep 22 2017
Looks good to me! The issue it (I think?) fixes is quite annoying, so go please submit and thanks :)
Sep 21 2017
Looks good to me, thanks!
Sep 18 2017
The deferred starting of the parsing. The dirwatches, sure -- if they are that slow on mac, another solution is needed.
Rene, did you actually read the comments I made on the other RR? I'm fairly sure that is the result an in-depth performance analysis will show too, and I'm faily sure your workaround is not needed if the performance *bugs* this code has are fixed ...
Sep 16 2017
Or rather, let me rephrase: The first point is IMO the much worse bug, since that is what will cause large project imports (think clang) be very slow in either case. For completely no reason whatsoever.
Ok, here's the two things that are broken and need fixing:
I just poked around a bit myself. It indeed takes ages to import the project, and I couldn't figure out why ... it is certainly not CPU load. I looked in perf, the CPU time spent in importing the project is quite low.
In fact, the code model generation (which seems to be the last step in the import AFAIU) finishes relatively quickly here (after something like 5 seconds), but then it still takes like 30 seconds for the import to finish. I don't know where that time is spent.
Sep 15 2017
Ok, you're right ... I looked some more. Try "-pipe", qmake passes that here, that seems to be what breaks things if I'm not mistaken.
There are some arguments which we shouldn't pass on, for example I have a qmake project here which does QMAKE_CXXFLAGS += -Werror=return-type and that completely breaks everything (no highlighting any more whatsoever, parsing of all files fails, ...), Not even sure why, but I guess -Werror arguments should never be passed on in our case.
Sep 14 2017
Re. IRC: Yes, to 5.2 I would say. We didn't release our beta yet, and I even think this counts as a bug fix ...
Thanks a lot!
Sep 13 2017
Looks sensible, thanks for investigating!
Sep 8 2017
Do you have some timing on how much loading time improves with this patch?
range-based for would detach ...?
If you had a reason not to use that, LGTM, thanks!
Sep 5 2017
Releasing with applications makes sense to me, I feel like it should be released in sync with kate.
If there is a hard dependency anyways then I agree with Dominik that we should move all plugins over. All of them are possibly useful for an embedding application, and with moving just some I see us moving plugins back and forth all the time for no real reason.
Sep 1 2017
Aug 29 2017
Aug 28 2017
Good solution I think, better than the hack from before (because it actually does the right thing, in a way). Feel free to put it into 5.2, we still have a few weeks to catch issues it causes. Thanks!