ADD: matchesregex() to the visibility expressions #2227

wants to merge 4 commits into

3 participants


This adds the function matchesregex(INFO_LABEL, STRING, [BOOL=false]) to the <visiblity></visibility stuff of the skinning engine.

BOOL= if true, the regex matching will be case insensitive

Befor you ask why? This feature was requested in this PR #1654 by @bstrdsmkr.
This feature seemed standalone enough so I decided to make it a seperate PR

Fice added some commits Jan 22, 2013
Fice Added matches_regex(InfoLabel, string, bool) to the visibility expres…
Fice FIXED: Operators like '+' did not work in the regex expressions becau…
…se xbmc thought they were the normal operators for visibility expressen. Added a quick fix, so you can add quotes around a string and all the symbols like '+' are not treated as operators then
Team Kodi member

First off, thanks for your efforts. However, this will not be added without some sort of caching built in to attempt to limit the impact: Doing regexps 60 times a second (complete with compilation) is not something we wish to encourage if we can possibly avoid it.

This could be done several ways, but one way might be to have an InfoRegexp (derived from InfoBool, there may be some work required to factory-ise it perhaps?) introduced that stores the labels etc. as needed. Then in it's Update() routine you check if the label has changed, and only run the regexp if it has. Note that the regexp can be compiled at parse time so you really only have a string compare then a regexp run (only if the string has changed) which will be far less computation load.

Regarding the string parsing, I'd really prefer that a grammar/parse tree is setup to make this correct once and for all. I have some work on this done up in a (local?) branch that I can clean and push up to github if you want to take a look (it's a somewhat separate issue to the main one of getting regexp support in though).


I tried to take the first step (compile the regex during parsing instead of evaluation). I felt that it was more appropriate to derive from GUIInfo and make a GUIRegexInfo. (The Caching part would be more appropriate in InfoBool, as that one is already responsible for deciding if it should be updatet?!?).

Well, during that I ran into the problem that I would have to downcast in CGUIInfoManager::GetMultiInfoFOO and I wanted to avoid that, so i moved the GetMultiInfoFOO functions into GUIInfo and GUIRegexInfo. Not sure this was the right move, the last commit is quite huge right now (and probably a b*tch to merge later on). But i see a few advantages: We could make 2 versions of GUIInfo depending on if they need a m_data2 or not (and save some space). Also I was thinking about moving GUIInfo and GUIRegexInfo in their own files, to make the 5000+ LOC of GUIInfoManager.cpp more comprehensible?!?

So, I'm not sure it was the right move myself so what do you think?

Team Kodi member

InfoBool still updates once a frame. The problem is it's still dumb - basically it just wraps a call into CGUIInfoManager::GetBool. Ofcourse, the idea is that it becomes less dumb with time as it's subclassed for specific functions - i.e. it effectively starts absorbing the boolean-specific things that are required. e.g. you could anticipate a class for all system booleans, or more generally, a class for all bools that require GUIInfo or GUIMultiInfo. The former could then call directly into the functions required, rather than jumping into a huge switch statement via GetBool(). Further, the updating could be made specific to the cases (e.g. system bools probably never need updating, essentially moving to a push-model).

Essentially you could create the InfoRegexpBool in CGUIInfoManager::Register() by detecting there that it's a regexp in the single bool case. (Later on, a better factory could be produced for this to avoid the issue with a regexp containing + et al.). The parsing function could then be implemented in the InfoRegexpBool constructor rather than calling TranslateSingleString - you'd call SplitInfoString() and then set all members, construct the regexp etc.


Yes, this commit was only to not recompile the regex every time (and it seemed to fit better in GUIInfo than in InfoBool), has nothing to do with caching.

Will take a closer look at how everything works tomorrow ;)


@jmarshallnz my exams are over, so I finally have some time to spare (already done some small things, but nothing to show for). Would be cool if you could push your local parse/syntax tree, so I could have a look and perhabs work on top of that ;)

Team Kodi member

@Fice it's a bit of a mess, but I've rebased it up here:

There's plenty of rambling comments which likely don't mean much to you, but you should get the basic idea from the Parser.cpp/Tokeniser.cpp stuff on how the parsing works and what the grammar is set up for. IIRC I aimed it initially at Builtins, but there's some changes there for info stuff which might be able to be adapted.

The tests use boost, but should be pretty easy to adapt to gtest if you want to work on it.

Team Kodi member

Closing as abandoned. @Fice if you want to continue with it, feel free to do so, and once done we can reopen etc.

@jmarshallnz jmarshallnz closed this May 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment