- User Since
- Jan 25 2016, 10:25 AM (146 w, 2 d)
Sorry, I don't see this going in in this or a similar form.
Sun, Nov 4
A solution might be to extend KXmlGui to somehow support such situations,
and decide who owns the shortcut by looking at the focus.
Wed, Oct 31
Tue, Oct 30
Mon, Oct 29
I'd have to use this for a while to see whether it works well in practice, but in general it seems like a cool idea.
Thu, Oct 25
Any help on the Windows version is greatly appreciated, whenever you find the time for it. :)
Tue, Oct 23
Well, I guess adding line offsets is just the level of abstraction our code gen works at, anyways. Which is good enough. In my opinion, go for it ;)
Is the opening brace always on its own line, independent of the formatter selected?
Mon, Oct 22
I'm inclined to say this looks innocent enough to go into 5.3 ...
Sun, Oct 21
Can go to 5.3 in my opinion. Thank you!
Wed, Oct 17
I'm not a huge fan of tuples either, for the reason you mentioned; I usually just create a two-element struct instead. In this case I think it's fairly obvious what the elements do from the types, though; the QPair<int, int> is worse ;)
Up to you how you do this, I just wanted to point out that it is done differently in two similar places.
I think the change conceptually makes sense, and I am subconsciously aware of the bug it is trying to fix. If you say it works for you, it should be fine.
Oct 15 2018
If there is no solution which does not require two different code paths for handling this problem, I'm against merging this, non-conformant old behaviour or not, sorry.
Oct 12 2018
I also find this reasonably simple to read. The "type your compiler arch" thing is extremely 80s of course, but the whole concept is a hack :/
Oct 11 2018
Code-wise this looks fine to me otherwise.
I can try it out.
I think the main performance issue here was fixed in 90f51f5830e32998d41a710c448212e49e1be04a, in my profiles that took >95% of the time. It's still worth optimizing, but I think no matter what you do there should not be complaints like the one linked in the bug report any more ...
Oct 9 2018
Otherwise looks good! If you feel super ambitious you could add a test, should be very simple ;)
Oct 5 2018
Looks good to me, and with little risk ... on Linux nothing changes, on Windows we test it anyways before shipping.
Oct 3 2018
On Arch Linux, the latest version of the patch seems to work fine.
Sep 26 2018
Sep 23 2018
Sep 19 2018
Sep 5 2018
I'm not really capable of doing an in-depth review here, since I don't know enough details of PHP nowadays. Style-wise I think it could still profit from reducing the block size of un-named (i.e. not a function with a name), uncommented complex code a bit, since it often requires quite some thinking to grasp what a certain conditional actually checks for. A simple comment like "// not a function" or whatever can do wonders there.
Feel free to merge this as-is or with a bit more comments, better get it in before the beta and still have a fix window before the release than merge it after the beta. Thanks for keeping kdev-php alive!
I'm super confused by the diff phabricator shows below but the commited changes are correct (and different from the diff here) ... what's going on?
For the record, I tried writing a test for this but didn't succeed and eventually put it aside, although the difference is easily visible in a test application. There must be a reason why the naive test case behaves differently from an interactive application ... I could take another look, I guess.
Aug 21 2018
Aug 20 2018
Aug 19 2018
Aug 17 2018
Sorry, never mind -- that code I removed yesterday. All should be fine.
Looks ok to me, except one thing: the operator== is used to compare a note from the list to the "currently active" note in the view. If this compares also the "under mouse" state, this code might be broken now ...?
Good amount of tests :) For the actual implementation, I would suggest trying to make it a bit more readable by a) splitting it up into several functions and b) trying to reduce indent depth a bit by using continue instead of nested ifs.
Maybe you make the the visitAssignment code a bit more readable by breaking it up into 2-3 functions?
As I said at Akademy already, I am neither familiar with the code of the PHP plugin nor with the later developments in the language itself, so I'm not a good reviewer for these patches. But since you want to get them in and nobody else seems to have time, I have read through this and I can't spot anything obviously wrong or stupid. We'll have a beta to try it out :)
Thanks for the patch, the approach looks reasonable at a first glance. You might want to unite the up/down functions ...
It is too much of a WiP to merge it like this, though.
Aug 16 2018
Author: Sven Brauch <firstname.lastname@example.org>
Date: Tue Aug 14 12:31:31 2018 +0200
update license text
Implement Dominik's suggestions
The patches you submitted recently are all very nice, if you feel think you are going to contribute more things in the future, I'm sure we can give you the permission bit to write to the KDE repos yourself :)
Just let us know at any time if you want that.
Of course arc is too stupid for the special characters in your name, sorry about that ...
No, you need special karma to push to the KDE repos. If you are not aware you have this, you have not ;)
I will submit this for you later.
This change looks very reasonable to me. Thank you!
Aug 15 2018
I'm actually fine with this approach, I just don't understand why the environment is set once on the process and once on the job. Why is this necessary, or do I misunderstand what it's doing ...?
address Dominik's suggestion and split focus handling and click handling
I have infinite patience in watching other people do the work, so by all means, go ahead! :-)
Thanks for the patch!
I added the rest of the interaction interface (click, mouseover)
and reduced the API a bit by moving a few hints into the InlineNote
Aug 14 2018
Can't we simply update our shipped schemas, and expect users with custom schemas to fix them?
Somehow I find the code rather strange, with the destructor removing items from the item model ...
But ok, if you tried it out and it fixes the problem, I guess it's fine.
add noteActivated notifier function
Looks good to me, even with unit test! :)
If nobody objects within the next few days, I'll merge this.
Thanks for the feedback! I will try doing a few more things with this interace and then maybe discuss again with the other kate people here at Akademy about which one they like better.
add missing files
Sample patch for KDevelop's problem highlighter plus screenshot:
Aug 13 2018
Ok, fine with me, I don't care much about the type. I just stumbled upon the comment and then noticed the type as well. Thanks for the patch, all fine from my side :)
Submitted with f30ef7c6f1376 to 5.2, sorry, I forgot to set the diff in the commit message.
Good find, the previous code (remove item -> call beginRemoveRows() -> call endRemoveRows()) definitely looks wrong. Can you submit this yourself?
Otherwise, I'd need an email address to set as the commit author.
Now I would compute the fileInfo's canonicalPath only once (it might require a stat) and then this looks okay :)
I can't accept it apparently because the discussion is still open ...
Yes, that looks reasonable.
I'd like to play with this a bit wrt what can be done in KDevelop with it (I want the problem popups gone). Would you mind if I do some changes along the way? I would post an updated patch here, in case I actually come up with useful changes ...
By the way, other people around here are very impressed by this patch as well, and we'd really like to get this merged :)
Moving e.g. KDevelop's warning markers into an inline note instead of the annoying popup would make a real difference for usability ...
Wow, that looks amazing! Really impressive.
Aug 12 2018
The "overly sensitive touchpad" issue seems to be missing accumulation of scroll events, so this patch to my understanding should not have it.