- User Since
- Apr 20 2015, 7:20 AM (165 w, 5 d)
Thu, Jun 21
@jpoelen Can you push yourself, or do you still not have a KDE contributor account?
Fri, Jun 15
Lgtm - thanks! Please only in master (as this review request shows).
Thu, Jun 14
const auto would be even nicer, if possible. Besides that, lgtm.
Tue, Jun 12
To be honest, I cannot claim I can follow all changes, since this is a big update. But in general the usage of the rules look good.
Nice update, will integrate. And thanks for the unit test!
Sat, Jun 9
Indeed, question is if wrapping helps: If you see 5 entries, and you see the last one is correct, then moving up would be ok.
But if the completion contains >> 10 entries, this will not work. And then, there is still the "End" key on the keyboard as dedicated action.
See arguments above: If we change it, another one will come over time and submit yet another patch to revert again, and again. This patch cannot go in as is.
I remember that @cullmann intentionally implemented it this way: Going upwards will hide the completion list and not wrap around.
Looks good to me, will integrate.
@mwolff To me this looks ok - do you see an issue with this? E.g. that KTextEditor will require much more memory for almost no gain?
The same holds true for other actions such as: Tools > Highlighting, Mode, Indentation, Script.
Works - will integrate. Thanks!
Thu, May 31
If it also was decided for Dolphin that this is wanted, I am fine with it.
Wed, May 30
I would give a ship-it - but maybe another review would be good? @ngraham maybe?
Tue, May 29
Will integrate, thanks.
@vkrause @cullmann Could you please comment on the above? Should we change KTextEditor such that dynamic is automatically derived based on whether there is a dynamic rule? Indeed, I agree with @jpoelen that this is redundant.
@jan.hajer Thanks for the patch, updates like this are very much appreciated, so please keep it coming :-) The change will be in KDE Frameworks 5.47, released early in June.
Ok, then please commit.
@rempt Is Digikams code the same as this?
This patch is correct, except that the version needs to be increased. Will integrate later.
Looks good to me.
If the format is really such that a term must appear before any Suffix, then this patch is already better that before.
@rempt Does Krita also allow changing the application color scheme? Would something like this be useful?
Mon, May 28
@tfjellstrom Would you give this a try?
Sun, May 27
Ping. Any updates on this matter?
Btw, did anyone test with dynamic word wrap enabled? Does it still work, when a line spans multiple view lines?
- make a branch: git checkout -b MyNiceFix origin/master
- commit in this branch
- arc diff # now you have a phabricator review D12345
- reviewers review
- you change your branch, and again: arc diff, to update the online review
- you get your OK: arc land
@turbov ping again :-) After adding the reference file in autotest/input, run make test, and then call ./autotests/update-reference-data.sh in your build folder. Then git status will tell you which files are new and need to be added updated.
Looks good, will integrate!
Looks pretty good to me already. Is there anything that needs to be addressed? What do you think about i18nc?
May 21 2018
Oh, I messed up the direction. Makes sense!
May 20 2018
What is the reason for this move? I am asking since I thought KWrite, Kate, and all the other applications using KTextEditor all implement command line parsing on their own. And Kate for instance has different options such as sessions that KWrite does not have.
May 14 2018
@jpoelen And why is it a problem with Kate? This means the C++ implementation of the highlighting rules is different in KSyntaxHighlighting and KTextEditor? If so, this would be an important catch. Can you elaborate why this is going wrong?
May 13 2018
I as someone being around for 15 years now also immediately thought "huh? Why this forth and back and forth..."?
May 12 2018
BTW, Akademy is nearing, registration is open. Could we send a mail to all (!) contributors who have a KDE commit account? I think this does make a lot of sense to trigger everyone (especially new contributors) to come to this event.
May 10 2018
@cullmann ping? :-)
Ah, found my comments in a different browser tab :-P
I think the setIcon() is also buggy, since if you call setIcon() and then setMessageType(), the icon is lost. So yes, I would be in favor of a better solution.
Somehow my other comments were lost, here we go:
- could you also update the screenshot in the doxygen documentation?
- setIcon() is behavior incompatible, and in fact, the referenced bugs did not complain about icons. So why the change? In my opinion this is not good enough in case of Kate/KWrite.
- setIcon(): Now the API documentation in the header file is wrong, since it says by default no icon is set.
Can we revert the setIcon() part? It changes application behavior, an in the case of Kate/KWrite, this is note wanted, see my comments.
May 6 2018
Looks good, will integrate.
Looks good to me. Please push.
This is already an excellent patch. The API documentation is already really nice.
Apr 29 2018
@devillemereuil If I remember correctly, you do not have to run all tests. It is enough to start the indenter test (look into the bin folder) and add as parameter you indent test folder (relative, e.g. cstyle). And, what Thomas says is correct. Typically a test case only contains very few lines to test a specific case. I think once you get the first one running, it will be easy for you to add more.
I think this is very cool. But there is one thing we require for new indenters: unit tests.
Apr 17 2018
@sraizada I am just seeing that this is your first KDE patch, nice! I hope that you do not get discouraged by my comments, I am just trying to find good solutions that work for everyone, and in this case, the solution this patch proposed did not work out in the past...
The behavior that is proposed here was like Kate behaved before, and as result we got bug reports that not all trailing spaces were removed. So we removed this.
Apr 16 2018
Out of curiosity: why are you putting multiple items in the same line on the keyword lists? This bloats up the diff and makes a real review so much harder... :/
Apr 15 2018
Ok, then thanks & please go ahead in master branch.
I think this is nice, but is this patch a problem if you have new Kate with KTextEditor < 5.46?
Sure? It was reverted a month ago once. Was it applied again?
@ngraham So what is the current state of a proper fix in KFontRequester?
They all are closed as a general KFontChooser issue. Maybe we should even blog about this :p
Btw there are bug reports for this. Could you link the Kate related ones, if existing?
A simple fix for a severe issue - please commit!
I am ok with this: +1
Apr 14 2018
Indeed, maybe this class should be part of the KConfigWidgets framework? See https://api.kde.org/frameworks/kconfigwidgets/html/annotated.html for current list of classes - it already contains the color scheme stuff and is a tier3 framework (so many dependencies allowed).
@cullmann This patch is unrelated to KTextEditor settings itself. It only adds an action that allows to change the KDE color scheme per application. The name "KateColorScheme..." is misleading here, since it has nothing to do with Kate color schemes...
Looks mostly good, but there are some minor issues we first need to address:
- dsControlFlow and similar default styles were added with KDE Frameworks 5, so kateversion="5.0" is required in the header
- WordDetect for things like "end if" is not sensitive to multiple spaces anymore. Are you sure this is correct? If not, please keep RegExpr.
Apr 2 2018
Just as more background: no, you are definitely not expected to push to the branch. You only should push to the branch if the bug justifies a mostly untested patch (untested in the sense of that it's probably only you who did proper testing). That said, it usually is up to you to decide. Personally, I believe crashes and severe bug fixes should be backported, but given the risk of introducing regressions in stable versions, commits to the stable branch are often not necessary. It all boils down to risk, and the fact that you take responsibility for what you do :-)
As background: in KF5 world, the KTextEditor settings are shared among applications: enabling line numbers in Kate will enable line numbers in KDevelop, Kile, KWrite, ...
Apr 1 2018
This looks good from my side.
Mar 31 2018
Not yet good enough, let's have another revision.
Thanks for the additional info. Please add this comment to the commit log when you push, so that we have it in the history. Thanks!
Would it also be possible to call setRestartHint() directly, or why is the connect required?