Possiblity to change Definition data after loading
ClosedPublic

Authored by sirgienko on Aug 9 2019, 8:37 PM.

Diff Detail

Repository
R216 Syntax Highlighting
Branch
read-write
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15020
Build 15038: arc lint + arc unit
sirgienko created this revision.Aug 9 2019, 8:37 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptAug 9 2019, 8:37 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
sirgienko requested review of this revision.Aug 9 2019, 8:37 PM
sirgienko added inline comments.Aug 9 2019, 8:39 PM
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.

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.

Yes, you right. I will change the pull request to setKeywordList.

sirgienko updated this revision to Diff 63489.Aug 10 2019, 4:35 PM

Use setKeywordList instead previous API

sirgienko marked an inline comment as done.Aug 10 2019, 4:36 PM

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.

sirgienko added a comment.EditedAug 10 2019, 5:47 PM

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.

Yes, I have forgotten, that reserve don't clear std::vector.

cullmann requested changes to this revision.Aug 10 2019, 7:24 PM

This looks more correct ;=)
Could you now add some test to the auto-tests? Thanks!

This revision now requires changes to proceed.Aug 10 2019, 7:24 PM

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:

  • Only existing keywordLists() can be changed. For non-existent keyword lists, false is returned.
  • @see keywordList(), keywordLists()

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?

sirgienko updated this revision to Diff 63599.Aug 12 2019, 11:50 AM

Add test and change the function description

sirgienko added inline comments.Aug 12 2019, 11:56 AM
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.

Mm, any changes?

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.

dhaumann added inline comments.Sat, Aug 24, 7:08 PM
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).

sirgienko updated this revision to Diff 64595.Sun, Aug 25, 6:06 PM

Add explanation, how setKeywordList can be used

sirgienko added inline comments.Sun, Aug 25, 6:11 PM
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.

cullmann accepted this revision.Sun, Aug 25, 6:30 PM

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!

This revision is now accepted and ready to land.Sun, Aug 25, 6:30 PM
This revision was automatically updated to reflect the committed changes.
dhaumann added inline comments.Mon, Aug 26, 9:16 AM
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 :)