- User Since
- Jan 25 2016, 10:25 AM (108 w, 5 d)
Looks good to me, also because null termination sounds better than \n (filenames can easily contain \n although they usually don't).
Jan 13 2018
Jan 2 2018
I'll add some people who have recently been working on kdev-php.
Dec 30 2017
Heh. Fix looks obviously correct to me (good find), and tests are always nice.
Dec 24 2017
Dec 23 2017
After this was submitted master doesn't compile for me, and if I fix the compile in the trivial way the test fails. Can you have another look?
All the changes look sensible to me, thanks!
Dec 21 2017
All three changes look good to me, although the formatting of comments is of course only useful until that clang bug is fixed ... but still, better than nothing and esp. when we already have the implentation in the codebase anyways.
Dec 17 2017
String change is probably ok if there is enough time before the next release (a few weeks) AFAIU. My two cents about where to merge it would be, put it in 5.2 only if you consider it a bug fix -- i.e. if there are projects which do not parse properly because this feature is missing go for it, otherwise put it in master. There's nothing worse than adding regression bugs in patch releases because of minor features like this.
I'm not up-to-date with the PHP standard, but guessing what the language feature does, code-wise this looks fine to me.
Dec 14 2017
Not much is left of the original patch but this change makes sense to me ;) thanks!
Dec 13 2017
Yeah, sorry, I'm also against this. Linking an extra lib we depend on anyways is a problem a compuer has to deal with, extra code is a problem humans have to deal with. The former wins against the latter unless there is a very good reason why not.
Dec 12 2017
Hmm. There's a comment in the line above which states it explicitly uses canonicalFilePath to avoid issues with symlinks. If we resolve symlinks like you suggest, there will be situations like files which are part of a project but for which the project's root directory is not a prefix of the file path. Are we somewhat sure this doesn't break in other places?
Dec 10 2017
Dec 7 2017
Is this actually faster? Why?
Dec 4 2017
Hmm, this will break build of all the plugins, no? Other than that, I'm in favour of this change, thanks for the work!
Dec 3 2017
Dec 1 2017
Nov 30 2017
Yes, cool feature, go for it!
Nov 29 2017
Thanks! I'll submit this as well.
Ah, you don't have write access to KDE repos? Then I will submit this for you. Thanks!
Alright, if you think it makes sense, submit it. It's certainly an improvement over the old situation. Thanks!
Nov 28 2017
Hmpf, right, QList compares its head to its tail which is certainly not atomic. I *think* for QVector this would be safe (it compares its size to zero), but better not take chances. Sorry that this is such a hassle.
Ok, but can we not at least set the state to Ended when the connection breaks, or something like that?
Hm yes, ok, so it makes sense why this fixes the crash. I think we can keep this as a safety guard. However I think the actual problem is that the state is not set to EndedState when the debug session actually ends, which also has other undesirable consequences (e.g. KDevelop doesn't switch back to code view, etc.). Do you have an idea why this happens?
To clarify my comment above, if what I said is correct, I would suggest to call the function "processFailedToStart"or so, and make it a non-slot.
I think I hit this issue yesterday when testing kdev-xdebug, esp. the debugger wouldn't stop properly when stepping over the end of the program. I don't quite understand what issue exactly this change fixes and why, can you write a few lines?
Very nice, thanks.
Nov 27 2017
Adress issues. Inline local statics where the wrapper function is not needed.
Nov 23 2017
Well conceptually, a signal is only a signature. The const has no meaning conceptually (what would be the difference between a const and a non-const signal). So it makes sense to pick a normalized form -- the non-const one.
Nov 22 2017
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 ...
Nov 20 2017
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.
Nov 19 2017
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!
Nov 17 2017
Yes, makes sense to me.
Ok, REVIEW: doesn't close reviews ... submitted with a2712c8d969137 to 5.2. Thanks for the comments.
Nov 16 2017
+1 from me, but let apol review it ;)
Nov 15 2017
I understand what the patch does, but can you explain why this fixes your issue?
Nov 8 2017
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 ...?
Nov 7 2017
Looks good, thanks!
Nov 5 2017
Nov 3 2017
No, no objections, it doesn't really do any harm. Thanks!
Nov 1 2017
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.