I was actually thinking about adding this for the recent KMail template highlighting patch, but decided to install the custom XML file into the default path but mark it as hidden, as that was the immediately available solution. A custom search path would be the cleaner solution for such uses-cases IMHO, so special-purpose syntax definitions don't pollute the global ones. So I agree with the concept.

@dhaumann yes, your guess is correct. I was experimenting with replacing a difficult-to-maintain qscintilla fork with some custom syntax lexers with QTextEdit + KSyntaxHighlighting + easier-to-maintain custom Kate-style syntax definitions, in a non-KDE application. One of the custom syntax rules is arguably more generally useful outside of the application (although it may be misleading since it shares an extension with a more common use), but the others I feel would just pollute the global set since they're unlikely to ever be edited outside the app.

Ok, I guess with your reasoning given this patch indeed makes sense.

To me, this patch along with the unit test already looks very good. I only have minor comments, then this is good to go.

@vkrause Any comments from your side?

Hmm, I think the idea is good.

But instead of adding API, could we not just define that an additional resource dir is searched always?

e.g. like:

loadSyntaxFolder(repo, QStringLiteral(":/org.kde.syntax-highlighting/syntax-extra"));

or something like that and per definition applications (or libraries using the framework) can just add extra syntax files are resources to that directory?

Then you need no API and it is very easy to deploy the stuff, as you just bundle all the extra files in the qrc inside the binary (lib).

(same for themes)

That would then work like other frameworks like kxmlgui and Co. just search in some defined resource dir for extra things.

That would also be an option. Note, though, that the current patch also would allow that: repository.addCustomSearchPath(":/my-app-resource"); So with this API, you have both, folders on disk, and support for custom resource files.

PS: What I like on the version with adding API is that we make this feature very explicitly visible, and not some hidden feature that works through magic ;) But maybe that's just me...

That would also be an option. Note, though, that the current patch also would allow that: repository.addCustomSearchPath(":/my-app-resource"); So with this API, you have both, folders on disk, and support for custom resource files.

That is true, the question is if you need that flexibility.
If we just standardize some resources dirs for that, applications need zero api calls and just can package the wanted files in the right dirs and be done.

@vkrause Would the solution Christoph proposes also work for all the cases in the pim world, or is it not good enough?

@cullmann, I like that suggestion better... It's closer to what I originally tried, but was unable to because of the index cache. Are the resource paths documented anywhere that I should update?

Sorry, I missed some of the later discussion when updating the patch... I'm fine with either solution, I just want to know which is preferred :).

Thinking more about it, perhaps I lean towards the new API more, simply because it's more obvious how to inject custom syntax/theme files. As it was, I had to read the source to know where it was looking in the first place.

Btw, just in case we go for the files-in-a-resource way, maybe we should install the katehighlightindexer for this so everyone can use this. This way, we also could ensure our strict policies we now have (correct RegExps, trimmed keyword lists, possibly other automated checks).

QRC support is definitely preferred, but that should work with the original proposal indeed. Having just one fixed QRC extension path baked in would be problematic for PIM with its hundreds of libs/components for example, AFAIK QRCs don't overlay on the same path, do they?

QRC support is definitely preferred, but that should work with the original proposal indeed. Having just one fixed QRC extension path baked in would be problematic for PIM with its hundreds of libs/components for example, AFAIK QRCs don't overlay on the same path, do they?

IMHO they overlay in the same path, just as they do in their root path. Otherwise it wouldn't work e.g. for kxmlgui the way it (I hope) does ;=)

But you are 100% right, the initial proposal was even more flexible, I am just not sure that we really need that flexibility to solve the issue at hand.

From my side both variants would be ok (and useful for the mentioned usecases) ;=) You can decide.

In any case, even if we use this patch, adding files are resource is the preferred way.

Btw, if qrcs overlay, then you can already now overlay org.kde.syntaxhighlighting/syntax and /theme :-)

Btw, if qrcs overlay, then you can already now overlay org.kde.syntaxhighlighting/syntax and /theme :-)

That true, but for /syntax that doesn't help as the generated index will not allow to find the additional files ;=)

Ah, learned something new then :)
So we can have an additional hard-coded QRC search path, that would only prevent us from having indexed content in there. Probably fine, as we are looking at a handful of custom syntax definitions at most, right?
Installing the indexer might nevertheless be interesting, also thinking about a cleaner approach for cross-compilation than what we do atm.

Installing the indexer might be more useful in the custom path case, since it could be used to index bundled rules. If we just add the /syntax-extra search path and a shared library installs an index there, we'd be right back where we started with no way to bundle new rules in an application via qrc.

Is anything else needed for this? I'm assuming installing the indexer would go as a separate review and that nobody is waiting for me to add that here...

I think you are right, the indexer is an other issue.
We could even just disable that index... files are used by the framework beside for its own internal dirs.

I guess for this review just Volker needs to say which version should go in and we are done.