Details
Diff Detail
- Repository
- R216 Syntax Highlighting
- Branch
- read-write
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 14929 Build 14947: arc lint + arc unit
src/lib/keywordlist_p.h | ||
---|---|---|
73 | Not sure about this realization, I think, we could modify it without lookuping all data. |
Actually, wouldn't it make more sense to just provide a way to set the full keyword list?
You can get the QStringlist will all keywords already with keywordList, a setKeywordList(...) would make more sense to me than this modifiers.
That would allow to alter it like you want and then write it back in one go with just one re-init of the mapping.
Hmm, I think we want an unit test for this, too.
And I think it won't work like it is, before the call to initLookupForCaseSensitivity one needs to clear both m_keywordsSortedCaseSensitive and m_keywordsSortedCaseInsensitive vectors.
Imho the API is currently not clear enough, see comment below. Can we somehow resolve this?
src/lib/definition.h | ||
---|---|---|
339 | Please improve the API documentation and add, for instance:
What I am currently also missing is a section about highlight invalidation: Does one have to trigger a document rehighlight oneself, and if so, how? My point is: Assume a doc is highlighted up to line 100. Now you add keywords to a list that are used in these 100 lines, i.e. the highlighting may change arbitrarily. How is the KSyntaxHighlighting user supposed to trigger a rehighlight? Or should one only change keyword lists before highlighting any document? |
src/lib/definition.h | ||
---|---|---|
339 | I more thinking in variant, when the highlighting more dynamic, and is done more, that one time. For example, in Cantor, we rehighlight whole worksheet, when a new variable created in curresponding session. So, if you write "my_var = 3" in Python session (and run command cell with this command), "my_var" starts to highlight by Cantor as variable after that. |
I think this features fits what you need.
Not sure if it is super useful in a lot other cases, as e.g. reloading the repository will reset all changes and one does need to trigger rehighlight oneself if one changes keywords.
But I think the added API is small and won't hurt further development.
Perhaps Volker could say if he is OK with this, too.
src/lib/definition.h | ||
---|---|---|
339 | Fair enough :-) What you just described here is missing in the API documentation. Can you add this to setKeywordList()? Then it's clear to the user of KSyntaxHighlighting (e.g. that you have to force a rehighlight of your document, best even explain how). |
src/lib/definition.h | ||
---|---|---|
339 | Is it enough? I mean, I can add explanation, that rehiglighting whole document (after keyword list modification) works via QSyntaxHighlighter::rehighlight(), but i think, it is pretty obivously for a KSyntaxHiglighting user. |
I think this is ok.
One can fine-tune the API docs still afterwards.
I see no "this is all evil" comments, therefore I approve this now!
Thanks for the contribution and I hope this is of use for your application!
src/lib/definition.h | ||
---|---|---|
339 | I think it's better. I'll take a look later and try to improve this and add you as reviewer :) |