-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
registry: Parse “popularity” attribute #406
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side-note: the reason I skipped this in the registry loading is because at the time xkeyboard-config didn't always set it (iirc) and it's somewhat superfluous since xk-c also guarantees that there's no exotic in the default base.xml file, see the test_rules.py
there.
A few style nitpicks but this LGTM to me otherwise. The struct config_item
change would be nice if you find the time for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed review comments.
4cb6e5f
to
ee9be77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only had a cursory look (I defer to @whot), but LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this LGTM!
l->popularity = popularity; | ||
l->description = config.description; | ||
l->brief = config.brief; | ||
l->popularity = config.popularity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we totally need a g_steal_pointer or copy libei's steal macro for the same effect. would allow us to just use an unconditional cleanup path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will have a look, but let’s do this in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it a try in #407.
Previously the attribute “popularity” was completely ignored. It also did not respect the modified DTD, because its default value depends if we are currently parsing an “extras” rules file. Fixed: - Always parse the popularity attribute. - Change the DTD to reflect that the default value is implied.
ee9be77
to
3189816
Compare
Previously the attribute “popularity” was completely ignored. It also did not respect the modified DTD, because its default value depends if we are loading an “extras” rules file.
Fixed:
Fixes #278