- User Since
- Apr 20 2015, 7:20 AM (190 w, 6 d)
I integrated this now, see a16d082370a44fcbae3a204bfede1db6e6dffe86 - the change also includes the rename of autoHide to autoHideDelay. The function names cannot be changed of course, since it's public API.
Now you are mixing documentation changes with code changes. I would have preferred to have different review requests, especially since the documentation part was already reviewed. Further, I would prefer autoHideDelay instead of just delay. If we use "delay" only, then the question immediately arises "the delay of what"? With this in mind, the naming of "autoHide" as better, since at least the context was clear. I will take care of the documentation now, but will not take care of the renaming for the reason given.
Looks almost good, except for "optional" and a missing "be". Could you update again?
I am not 100% convinced of this feature, but my attitude is: let's try this, and if someone complains, we can still change it. So I am fine with the change.
Sat, Dec 15
What happens, if you have multiple mainwindows? Do you get the message multiple times then? Please try with View > New Window.
Not sure about binary compatibility...
Wed, Dec 12
Well, if it's copy & pasted, it must be correct ;)
Reviewing correct context popping is always not so easy - I trust this part is correct?! ;)
Thanks, good patch again.
Not an issue but for the future: your initial summary for the review request is non-existent :) could you in future elaborate? E.g.: did you observe memleaks? Did it crash? Does perf or valgrind proof this? Etc... :)
Do we really need this? Or is this a reject?
Ok with me as well.
Looks still good to me.
Tue, Dec 11
Mon, Dec 10
Could you also extend the unit test in autotest/input/highlight.d?
I guess it's ok to have this in. But please use this version at your daily work ;) I think we don't have many testers...
Looks good, please commit. With these kind of changes, we soon support up to 2^64 lines in a document? ;)
Sun, Dec 9
I dislike the fact that we have two times almost the same thing. Then again I can see that if C++ is set explicitly, the cppcheck plugin will be unusable for C. Then again, cppcheck contains 'cpp' in its name. So should we care about C at all here? If the answer is 'no', I would prefer to simply add the command line option and be done with it :-) or add an option in the Projects config page.
Sat, Dec 8
Hm indeed. I remember when the patch was added that added the editing positions. At that time I was not convinced about this, and now even performance problems show up.
Just curious: Do the editing positions really show up in perf?
Curent state: This patch is only almost good to go in... :-P
- increase version number only by one :-)
Maybe file_save_alternatives or as Gregor once suggested file_save_variants. Or file_save_extended.
Is it correct that highlighting in the document (i.e. KSyntaxHighlighting) still takes place for the entire line, and this change simply only changes the fact that KateRenderer stops using the highlighting info after 100000 columns?
@cullmann You did not seem to have strong objections to this. Can you comment on this here again?
Looks good to me, but please consider using i18np to enable correct plural translations.
I agree this should go in (possibly with minor improvements I added as comments): The patch is already big enough, so I'd rather prefer continuation work on followup patches.
As background: inserting the '*' automatically is done by the indenter in cstyle.js. Theorwtically, we could even add a joinLines function to the indenters as well and call it if it exists, and if not use the default implementation. Just an idea... Usually, the motivation for moving something into an indenter boils down to whether a specific behavior is related only to a specific programming language, i.e. when a general purpose solution cannot be done.
Fri, Dec 7
Yes, looks good to me, thanks! These little things do mske a difference :)
I think is an improvement - sorry for the delay :( will integrate tomorrow.
I guess this is fine. Could you use your real name please?
Wed, Dec 5
I think this is fine. A minor comment from my side would be: I am not sure adding these kind of labels are alwas a good solution. I can see that it's useful to know which files are affected. Then again, once you know this, the label is just visual noise.
Mon, Dec 3
Hm, is this really a good idea? Right now, the File menu is rather flat, which is a good thing imo. Do we really want to move Save operations into a "Save Variants" sub menu?
I think Kevin's suggestion is a good one. One more iteration?
Mon, Nov 26
Btw, I think in this case the fix is correct.
In general I have no objections to this patch. I'd welcone another review, though.
Wed, Nov 21
Tue, Nov 20
I would have preferred to only have one #indef WIN32. Preprocess statements should be reduced to the minimum, imho.
Mon, Nov 19
Hm, isn't it that in all 4 cases Ctrl+L focuses the line edit and selects all? /Independent/ on the context? That's what you just wrote, right? :-)
Sun, Nov 18
I kind of am with @elvisangelaccio here and would argue that the file dialog is inconsistent :-)
I am not voting against this feature, but I use CTRL+L also in browsers. There, CTRL+L does two things:
Nov 12 2018
Unfortunately, someone started with v2 only many years ago, and other reused this license simply due to copy & paste. Nowadays, we have a hard time getting rid of v2 only. Please use v2+.
Nov 9 2018
Unless you have a KDE commit account you can not commit yourself. Unfortunately we cannot see on Phabricator whether you have one or not. We can integrate this patch and then it will be in the next KDE Frameworks release, i.e. early next month.
Nov 4 2018
I am with Kare here: I think we should care to not enable plugins by default that are known to be problematic on windows.
Can you #ifdef this or at least add a runtime check depending on the OS?
Has already 2x +1 --> Let's try this.
Good idea to unify this. Can we have one more iteration? :)
Oct 28 2018
As I see we have the following situation
- the current implementation does work reasonably well
- just because we cannot match a theme 100% does not imply that we have to adapt our implementation (there will always be a color in theme xy that does not properly exist in KTextEditor)
- KTextEditor does not use color themes from KSyntaxHighlighting yet, so the transition is not complete
Oct 26 2018
Please add a small test file and explain what this highlighting language is used for :)
Could you explain what CLIST is used for? Please add a small test file that is also MIT licensed.
Oct 22 2018
Oct 21 2018
@gregormi In my ~/kde/kf5/src/kdesrc-buildrc file I have the line:
Yes, in KDE Frameworks, there is only a master branch.
Oct 20 2018
Well, make test works for me, and if this fixes the issue for you, I'm fine with that.
Did you followup on this with @skelly ?
In yet another patch, I would change the public members of FilenameListItem to getter functions. Then we are free to either cache the data, or only calculate all stuff on-the-fly from the KTextEditor::Document.
Looks good to me, please commit.
Oct 18 2018
Qt decided against #pragma once. I wouldn't want to use it in KDE unless it is decided on kde-frameworks-devel.
Oct 17 2018
Hi Gregor, I just did a post-review again, and noticed several issues I think we should address. I have a patch ready for most of the stuff, but did not yet post it on Phabricator, since I indeed use KTextEditor::Document as member pointer in the FileListItem, and its constructor now reads the icon etc from the document, which destroys the tsttestapp demo. So the comments are just here to make you aware of the issues. We can have the real discussion when I post the patch.
Feel free to push this for 5.52 (you'd have to adapt the @since 5.53 again).
@slenz Thanks for this nice patch, please keep it coming :-)
Do you have to rebase your patch now that the other patch is in?
Oct 15 2018
The title "Cleanup" is a bit too generic imho. Better eould be something along the lines "Ceanup: Merge main() into unit test tsttestapp.cpp" along with a detailed description :) Maybe something for future commits? Good enough this time, but for commits in KDE Frameworks this would not be good enough, since "Cleanup" would then end up in David's changelog, which is too generic (cleanup of what?).
I would prefer that the contents of main.cpp goes to tstestapp.cpp, since this would allow to have multiple tests over time. But this can be done in a separate patch, please push.
Looks good, please push.
Oct 13 2018
Yes, please push. In a separate patch, could you possibly look at KateTabSwitcherTest::testLongestCommonPrefix() again? It looks as if you could use QFETCH along eith a function KateTabSwitcherTest::testLongestCommonPrefix_data()? QFETCH is typically used when testing a data set.
Oct 10 2018
Oct 9 2018
I like it, definitely an improvement.
Oct 5 2018
If Christoph accepts, I am fine with this.
Hm, given the size of the patch, and given it introduces new public API we cannot easily change, I think a silent +1 since noone reacts is not good enough.
Oct 3 2018
Btw, can you commit yourself, or shall we push this? If so, can someone take care of this, since I am not available for the next 10 days.
Hm, two separator lines, even touching each other sounds like a hack. Is there no better way?
Oct 1 2018
Sep 30 2018
Fixed, see 9e039a669f8f34591c12e433cba1c97a11645c5d