- User Since
- Apr 20 2015, 7:20 AM (182 w, 3 d)
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?
Mon, Oct 15
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.
Sat, Oct 13
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.
Wed, Oct 10
Tue, Oct 9
I like it, definitely an improvement.
Fri, Oct 5
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.
Wed, Oct 3
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?
Mon, Oct 1
Sun, Sep 30
Fixed, see 9e039a669f8f34591c12e433cba1c97a11645c5d
Sat, Sep 29
We have Repository::definitionForMimetype() since KF 5.50:
Is this task done?
Fri, Sep 28
Indeed I just ported a QStandardItemModel to a QAbstractItemModel. The speed gain was huge since many QString -> QVariant conversions are not needed anymore. Do you see this in perf as well?
Wed, Sep 26
@cullmann Can you make a decision? I trust it will be a good one ;)
I cannot really say much about it, since I never use YAML. Still, if the unit test works, then I am fine with this.
Tue, Sep 18
@cullmann If the other solution is easier to maintain then I am fine with this. Still, I would like to avoid that includedDefinitions() for sass returns css just because of keywords. In fact, that was my main motivation for this solution.
Sep 16 2018
Yes, and calling loadKeywords would only do work the first time.
To be clear, currently we have two states:
- load metadata only
- lazy load full file
This was also my interpretation: only include the Definitions that use itemDatas / colors.
Sep 15 2018
FYI: This now landed with 88be459559448d9d30b33f33b3ffd31fc41327c7 (cf. https://commits.kde.org/kinit/88be459559448d9d30b33f33b3ffd31fc41327c7)
Btw, a sophisticated list of features is available on https://kate-editor.org/about-kate/
I commented on some things - I did not try this, though.
Sep 13 2018
Almost good enough, can you remove the forward declaration again? Just another one-line change. I then don't have any objections anymore.
Correct patch as discussed
Btw, this is a regression from the switch to the KSyntaxHighlighting framework. Maybe there is a better fix?
Sep 11 2018
Ctrl+shift+0 is much better than the +r variants. +r typically is used for reloading/refreshing. If we cannot find a better solution, then having no default shortcut would also be an option.
Sep 10 2018
Sep 9 2018
I think the idea in general is good. +1 from my side, things can still be improved later if it turns out that it does not work.
To me, Ctral+0 would be the correct choice. So +1 from my side.
Sure, please use kdesrc-build as described here:
Sep 8 2018
Btw, we forgot to add the themes to the Qt resource file, see also D15358
- Merge branch 'master' into check-themes
- Include Solarized color themes
See D15358 :-)
Looks good to me.
Sep 7 2018
Looks good to me. Can you commit yourself?
Solarized dark still only has 28 colors, right? Can you add the missing ones?
Thanks for working on this. Looks good to me - just a minor question about the section for Solarized light.
Sep 5 2018
Sep 4 2018
Is this due to drop of own hl implementation? In any case, lgtm.
Sep 3 2018
Btw, did you try using QRegExp just for testing the speed difference? If the behavior is the same, we could even consider using that as long as it still exists...
Sep 2 2018
@cullmann Can you update the docbook in kate.git/doc/ so that the documentation about writing syntax highlighting files keeps up to date also for the dynamic changes?
Aug 30 2018
BTW: did you grep for other locations in kio for "_AIX"? Might make sense.
I guess this makes sense. One could add a #error in case of someone trying to build on aix, but it's probably overkill.
Aug 28 2018
Aug 27 2018
@broulik Could we clarify the copyright?
Aug 26 2018
Do you need the public methods in Definition outside of KSyntaxHighlighting?
@rkflx can you please reopen so we can accept this?
Aug 25 2018
The old wildcard matcher existed for performance reasons. Did you check?
@rkflx I think this patch is definitely good enough to not let it go.
Was this resolved, or did you give up?
Aug 24 2018
- Verify Definition::formats() contain all formats from 1...N
Aug 22 2018
True, agreed. And it was an ugly workaround in the first place.
BTW, slightly related as idea: I think it would be nice to add a function Definition Downloader::setDownloadLocation() or similar that allows to have a custom download location. This would match nicely with Repository::addCustomSearchPath().
I am fine with this change. I want a review from Volker, through .