D23061: Possiblity to change Definition data after loading

sirgienko created this revision. sirgienko added reviewers: dhaumann, cullmann. sirgienko added a project: Framework: Syntax Highlighting. Herald added projects: Kate, Frameworks. Herald added subscribers: kde-frameworks-devel, kwrite-devel. sirgienko requested review of this revision. REPOSITORY

D23061: Possiblity to change Definition data after loading

sirgienko added inline comments. INLINE COMMENTS > keywordlist_p.h:73 > +m_keywords.removeAll(keyword); > +initLookupForCaseSensitivity(m_caseSensitive); > +} Not sure about this realization, I think, we could modify it without lookuping all data. REPOSITORY R216 Syntax H

D23061: Possiblity to change Definition data after loading

cullmann added a comment. 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 l

D23061: Possiblity to change Definition data after loading

sirgienko added a comment. In D23061#509540 , @cullmann wrote: > 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 setKeywordL

D23061: Possiblity to change Definition data after loading

sirgienko updated this revision to Diff 63489. sirgienko added a comment. Use `setKeywordList` instead previous API REPOSITORY R216 Syntax Highlighting CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23061?vs=63448&id=63489 BRANCH read-write REVISION DETAIL https://phabricato

D23061: Possiblity to change Definition data after loading

sirgienko marked an inline comment as done. REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D23061 To: sirgienko, dhaumann, cullmann Cc: kwrite-devel, kde-frameworks-devel, univerz, LeGast00n, gennad, bmortimer, domson, michaelh, genethomas, ngraham, bruns, d

D23061: Possiblity to change Definition data after loading

cullmann added a reviewer: vkrause. REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D23061 To: sirgienko, dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel, univerz, LeGast00n, gennad, bmortimer, domson, michaelh, genethomas, ngraham, bruns,

D23061: Possiblity to change Definition data after loading

cullmann added a comment. 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. REPOSITORY R216 Syntax Highl

D23061: Possiblity to change Definition data after loading

sirgienko added a comment. In D23061#509851 , @cullmann wrote: > 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_keywordsSorted

D23061: Possiblity to change Definition data after loading

sirgienko updated this revision to Diff 63492. sirgienko added a comment. Bugfix REPOSITORY R216 Syntax Highlighting CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23061?vs=63489&id=63492 BRANCH read-write REVISION DETAIL https://phabricator.kde.org/D23061 AFFECTED FILES

D23061: Possiblity to change Definition data after loading

cullmann requested changes to this revision. cullmann added a comment. This revision now requires changes to proceed. This looks more correct ;=) Could you now add some test to the auto-tests? Thanks! REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D23061

D23061: Possiblity to change Definition data after loading

dhaumann added a comment. Imho the API is currently not clear enough, see comment below. Can we somehow resolve this? INLINE COMMENTS > definition.h:339 > + * @since 5.62 > + */ > +bool setKeywordList(const QString& name, const QStringList& content); Please improve the API docum

D23061: Possiblity to change Definition data after loading

sirgienko updated this revision to Diff 63599. sirgienko added a comment. Add test and change the function description REPOSITORY R216 Syntax Highlighting CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23061?vs=63492&id=63599 BRANCH read-write REVISION DETAIL https://phabric

D23061: Possiblity to change Definition data after loading

sirgienko added inline comments. INLINE COMMENTS > dhaumann wrote in 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() > >

D23061: Possiblity to change Definition data after loading

sirgienko added a comment. Mm, any changes? REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D23061 To: sirgienko, dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel, LeGast00n, gennad, GB_2, bmortimer, domson, michaelh, genethomas, ngraham

D23061: Possiblity to change Definition data after loading

cullmann added a comment. 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

D23061: Possiblity to change Definition data after loading

dhaumann added inline comments. INLINE COMMENTS > sirgienko wrote in 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 sessi

D23061: Possiblity to change Definition data after loading

sirgienko updated this revision to Diff 64595. sirgienko added a comment. Add explanation, how setKeywordList can be used REPOSITORY R216 Syntax Highlighting CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D23061?vs=63599&id=64595 BRANCH read-write REVISION DETAIL https://phab

D23061: Possiblity to change Definition data after loading

sirgienko added inline comments. INLINE COMMENTS > dhaumann wrote in 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 r

D23061: Possiblity to change Definition data after loading

cullmann accepted this revision. cullmann added a comment. This revision is now accepted and ready to land. 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

D23061: Possiblity to change Definition data after loading

This revision was automatically updated to reflect the committed changes. Closed by commit R216:21c3f38f6091: Possiblity to change Definition data after loading (authored by sirgienko, committed by cullmann). REPOSITORY R216 Syntax Highlighting CHANGES SINCE LAST UPDATE https://phabricator.k

D23061: Possiblity to change Definition data after loading

dhaumann added inline comments. INLINE COMMENTS > sirgienko wrote in 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 >