Add Definition::::formats()
ClosedPublic

Authored by dhaumann on Jul 29 2018, 9:42 AM.

Details

Summary

Definition::formats() can be used to list all Formats from one
specific Definition, e.g. for printing a syntax guide.

Test Plan

make test, added unit test

Diff Detail

Repository
R216 Syntax Highlighting
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dhaumann created this revision.Jul 29 2018, 9:42 AM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptJul 29 2018, 9:42 AM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
dhaumann requested review of this revision.Jul 29 2018, 9:42 AM

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).

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.

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).

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.: <?php { /> html { <?php } />. 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?

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.

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?

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.

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.

dhaumann retitled this revision from Add Repo::formatFromId(), Definition::formatFromId() and ::formats() to Add Definition::::formats().Jul 29 2018, 6:35 PM
dhaumann edited the summary of this revision. (Show Details)

Together with includedDefinitions() from the other request, this should be sufficient to fake the internal attribute vectors in ktexteditor.

vkrause accepted this revision.Jul 30 2018, 7:14 AM
This revision is now accepted and ready to land.Jul 30 2018, 7:14 AM
This revision was automatically updated to reflect the committed changes.