D14451: Add Repo::formatFromId(), Definition::formatFromId() and ::formats()
dhaumann updated this revision to Diff 38721. dhaumann added a comment. Update REPOSITORY R216 Syntax Highlighting CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14451?vs=38720=38721 BRANCH addFormatGetters (branched from master) REVISION DETAIL https://phabricator.kde.org/D14451 AFFECTED FILES autotests/syntaxrepository_test.cpp src/lib/definition.cpp src/lib/definition.h To: dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14451: Add Repo::formatFromId(), Definition::formatFromId() and ::formats()
dhaumann updated this revision to Diff 38720. dhaumann added a comment. - Update REPOSITORY R216 Syntax Highlighting CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D14451?vs=38695=38720 BRANCH addFormatGetters (branched from master) REVISION DETAIL https://phabricator.kde.org/D14451 AFFECTED FILES autotests/syntaxrepository_test.cpp src/lib/definition.cpp src/lib/definition.h src/lib/repository.cpp src/lib/repository.h To: dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14451: Add Repo::formatFromId(), Definition::formatFromId() and ::formats()
vkrause added a comment. The whole format id code is indeed not used here, this was purely added based on the requirements I got from you guys :) So feel free to change whatever is necessary. REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D14451 To: dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14451: Add Repo::formatFromId(), Definition::formatFromId() and ::formats()
cullmann added a comment. I think if we have 1. formats() per definition 2. some "includedDefinitions" we can build up the needed format list per highlighting that will allow us to map the syntax-highlighting formats to the KTextEditor::Attribute we need internally and in our external API. A global lookup for id => format might then not even be needed, as we will internally need to lookup our wrapped KTextEditor::Attribute anyways and need to come up with some format->id() => internal attribute mapping after building that up. What is lacking in the Format ATM is an accessor for the underlying default style, I think, which we need, as that is exposed both in Attribute and other API. REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D14451 To: dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14451: Add Repo::formatFromId(), Definition::formatFromId() and ::formats()
dhaumann added a comment. > e.g. for the customization of coloring you want to present the list of all formats a highlighting has, including the included ones. (even if later the color of the included ones is not stored per including highlighting) The included ones behave differently in KSyntaxHighlighting and KTE: In KTE, the colors of included definitions can be different when included in multiple highlighitng files. In KSyntaxHighlighting, this is by design not possible: If you change "Alert" in C++, you also change Alert in Python etc... So this is always shared. And getting a list of all included Definitions sounds doable as well ;) That is probably still missing in the API. So we'd need a Definition::includedDefinitions (that recursively returns all Definitions it uses). Anything else? REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D14451 To: dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14451: Add Repo::formatFromId(), Definition::formatFromId() and ::formats()
cullmann added a comment. e.g. for the customization of coloring you want to present the list of all formats a highlighting has, including the included ones. (even if later the color of the included ones is not stored per including highlighting) Otherwise you are right, a global lookup is all one needs, which could be build on-the-fly in ktexteditor, too, if we don't want to have that in the syntax-highlighting framework, given the id's are unique. REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D14451 To: dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14451: Add Repo::formatFromId(), Definition::formatFromId() and ::formats()
dhaumann added a comment.
Btw, thinking more about it: I believe we do want globally unique IDs for a
very simple reason. Think of e.g. bracket matching where multiple Definitions
are included (e.g.: html { . For the last '}', we want to
find the matching attribute '{'. With globally unique attributes, this is as
easy as compaing IDs. With Definition local IDs, we also have to check the
Definition and the attribute.
So can we at least conclude that the global IDs are what we want?
With respect to #includeRules-included a Definition 'X': The IDs from
included 'X' are the same as the IDs from 'X' itself. So the Formats are
reused. Why do you need a list of all included IDs at all? If the IDs are
global, you simply don't care, right?
REPOSITORY
R216 Syntax Highlighting
REVISION DETAIL
https://phabricator.kde.org/D14451
To: dhaumann, cullmann, vkrause
Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns,
demsking, cullmann, sars, dhaumann
D14451: Add Repo::formatFromId(), Definition::formatFromId() and ::formats()
cullmann added a comment. I think an issue is the inclusion of definition in each other. I think ATM the formats are really only per definition, that means if the ids would be definition local, one will have clashs. I think ATM there is no way to get formats for definitions that got included at all. To be able to port our stuff nicely, we would need a way to have that (or to get some global vector of all formats for a definition including the included ones). REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D14451 To: dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14451: Add Repo::formatFromId(), Definition::formatFromId() and ::formats()
dhaumann added a comment. I just had a look into the code - the Format IDs are currently not used at all by KSyntaxHighlighting. Instead, all format lookups are done via the Format name. And that by definition means that the Format looksups are per Definition, since Formats from different Definitions may have the same name. In other words, if the lookup of the IDs is good enough on Definition level, then we can change this entirely. Maybe we can even switch the internal lookup from string-based to id-based, certainly not slower. REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D14451 To: dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14451: Add Repo::formatFromId(), Definition::formatFromId() and ::formats()
cullmann added a comment. I am wondering: Do we really need to have unique IDs over all definitions? Actually I would think it would be much nicer to have all formats for one definition in a vector and having the index as the unique id. The lookup is faster and it is easier to understand. (and the 16-bit limit is no actual limit anymore for any realistic definition). REPOSITORY R216 Syntax Highlighting REVISION DETAIL https://phabricator.kde.org/D14451 To: dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
D14451: Add Repo::formatFromId(), Definition::formatFromId() and ::formats()
dhaumann created this revision. dhaumann added reviewers: cullmann, vkrause. Restricted Application added projects: Kate, Frameworks. Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. dhaumann requested review of this revision. REVISION SUMMARY With Repository::formatFromId() it is now possible to only save the format ids when highlighting a line, which is a very efficient way to store highlighting information per text line and the way it is done in KTextEditor. Definition::formats() can be used to list all Formats from one specific Definition, e.g. for printing a syntax guide. Defintion:formatFromId() only exists for convenience and internally redirects to Repository::formatFromId(). TEST PLAN make test, added unit test REPOSITORY R216 Syntax Highlighting BRANCH addFormatGetters (branched from master) REVISION DETAIL https://phabricator.kde.org/D14451 AFFECTED FILES autotests/syntaxrepository_test.cpp src/lib/definition.cpp src/lib/definition.h src/lib/repository.cpp src/lib/repository.h src/lib/repository_p.h To: dhaumann, cullmann, vkrause Cc: kwrite-devel, kde-frameworks-devel, michaelh, kevinapavew, ngraham, bruns, demsking, cullmann, sars, dhaumann
