- User Since
- Apr 20 2015, 7:20 AM (156 w, 4 d)
Tue, Apr 17
@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.
Mon, Apr 16
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... :/
Sun, Apr 15
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
Sat, Apr 14
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.
Mon, Apr 2
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, ...
Sun, Apr 1
This looks good from my side.
Sat, Mar 31
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?
Change is correct imo.
Makes sense, thanks.
Looks good to me. Thanks for this update!
Btw, I messed up the dates. Tag is next Saturday after today...
Sat, Mar 24
The patch looks good. Please commit this weekend, or wait until next Saturday's v5.45 branch.
Fri, Mar 23
I like this a lot, but have not done a review yet. Will do later.
Wed, Mar 21
An obvious typo. Thanks for this fix! PS: A Screenshot would have been nice, though :-)
Cool! The patch already looks pretty good. Please address or comment on my questions.
Mar 20 2018
Looks good to me.
Mar 13 2018
@jpoelen What would be interesting is to check which optimizations are really an improvement. Because we should either get a significant speed boost (e.g. RegExpr -> WordDetect), or at least reduce memory allocations (possibly StringDetect -> Detect2Chars and DetectChar).
Mar 10 2018
Just for info: https://kate-editor.org/2018/03/10/improving-syntax-highlighting-files/ tries to reach a broader audience for getting patches.
Btw, current generated list finds 1658 issues: https://paste.kde.org/p7iareuxc
Oh, and btw. sorry that this review took essentially 7 months. Sometimes we simply suck and things go badly wrong :-)
So everyone seems to agree that in general it's a good idea.
As quick comment: I like the idea of programmatically suggesting better usage of rules. I did not yet fine the time to look into this, so I do not know how much output this currently generates.
Mar 3 2018
Looks ok to me now.
Calling make test creates the reference unit test data, this was missing. I did this now, adding:
new file: autotests/folding/test.mib.fold new file: autotests/html/test.mib.html new file: autotests/reference/test.mib.ref
Compiling results in:
Feb 25 2018
Mark comments as done.
Good point. Question is what git does in this case. We should be able to test this with a fat USB stick... Todo :)
Hm, did you update with arc diff, or did you do this differently?
Could be, but Windows XP is not supported anymore, so we do not care.
Feb 24 2018
- Add and use contains()
- Use QLaint1String
@vkrause Do you already have something in mind, when you say this does not scale? ...
- Use QString::fromUtf8(ByteArray) for raw string interpretation
Ok, good catch. Given the uninitialized variables in the other classes, this is good. Will integrate.
I'll remove the selColor attributes, since these will not work with dark color schemes.
Similar, I will remove all underline, italic, and bold attributes where not needed by default.
Sounds good to me.
Looks good to me, will integrate.
@julianstirling Can you profile the full xml file again? This patch does not contain the .xml file anymore, so in the current form I cannot commit this as is.
Feb 21 2018
Thanks for the contribution. Before we integrate this, would you agree to change the license to MIT? This license is preferred for new highlighting files.
Feb 19 2018
In general looks good, and it even revealed a bug as I understand.
Feb 15 2018
One small suggestion to improve the clipboard code.
Thinking about it, what about this: The dialog already has all the QLabels. What you could do is something along the lines:
I can see the reason for English text only. Then again, I think that @rkflx has a very valid argument as well. I do not know any other place in KDE that intentionally uses non-translatable text. That's why I agree with @rkflx to better translate this.
Yes, looks ok, will integrate.
Feb 13 2018
Close as it should work now.
This is fixed, see commit https://phabricator.kde.org/R39:1a38adebb64e6e7d5acb756f68166d56d8ba0b72
Question is: what happens in the vi test? Did we really introduce a regression or is the test wrong?
Feb 6 2018
This fixes bug https://bugs.kde.org/show_bug.cgi?id=389307 at least partially. More languages such as Ruby etc can still be added.
Feb 5 2018
To be honest, I do not know. The risk of introducing a regression definitely exists. Also, I do not know whether this works on Windows. What do you suggest to test / do?
Feb 4 2018
Translation is not yet working correctly, see above comment.
All the keys in collectedData are not properly translated. As such, creating a final string out of this is not translation friendly.
- Use reference
@kfunk Do you need such a patch in the KDevelop document switcher plugin as well? This code comes from your code after all...