Skip to content
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

Peripheral add-ons #2706

Closed
wants to merge 7 commits into from
Closed

Peripheral add-ons #2706

wants to merge 7 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 7, 2013

I have always found system/keymaps and system/Lircmap.xml to be a little messy.
they are shipped with the core application and thus cannot be updated between releases. furthermore everything is one huge monolith so over time we hardly know where half the data comes from, which platform(s) is is for and so on.

This moves all input devices to a peripheral add-on extension point. the add-ons themself have been done in a hurry, without putting a lot of thought into cosmetics and such.

a peripheral is basically just a collection of keymaps, lircmaps and irssmaps in any combination wrapped up in the usual add-on fluff.

they can be marked as 'reference' add-ons. these will be loaded first, and is to be used with stuff like the general keyboard mappings, general mouse mappings etc.

they can also be marked as 'conditional' add-ons. this is to be used with the peripheral manager stuff, and are not auto-injected into the button translator. rather, it's done when the peripheral is detected, as exemplified for the nyxboard. it can probably be done cleaner than this simple approach, but most important is that @opdenkamp believes this is enough to add better code later.

cleaning up our code in the keymapping and keymap parsing department is quite the task. i have started it with this work, but there is lots left to do. hopefully you'll agree that this can be done in incremental steps. it's just too much to do in one go, and it's a shame to delay (what i consider) nice features waiting for all the code to be cleaned up.

i have chosen to go for some (temporary) code duplication. in parts, since this changes the XML format slightly (to ease device aliasing) there would be close, but not quite identical code in any case (you know xml..) in parts because we can do cleaner code (no need to worry about legacy) and then simply drop the old once we are satisfied.

spiff added 7 commits May 7, 2013 22:58
required for an upcoming template
a peripheral is currently defined as a device
having a keymap, a lircmap or a irss map (in
any combination).

this adds a new keymap format that is less verbose
and easier to work with. the code for handling this
is put in the peripheral add-on class with the ultimate
goal of moving all loading there.
@theuni
Copy link
Contributor

theuni commented May 7, 2013

big +1 on the premise, this is long overdue imo.

@Montellese
Copy link
Member

Very nice. You're at least as crazy as I am ;-)

}

bool result=false;
for (int i=0;i<files.Size();++i)

This comment was marked as spam.

This comment was marked as spam.

@Montellese
Copy link
Member

I haven't mentioned all the cosmetics there are because I figured that some of the code in CPeripheral is more or less copy & paste from CButtonTranslator right?

@ghost
Copy link
Author

ghost commented May 8, 2013

yes it is cnp. jut call out anything you see. i dont care who arsed up an issue is an issue..

}

// add our map to our table
if (map.size() > 0)

This comment was marked as spam.

@jmarshallnz
Copy link
Contributor

I like.

@garbear
Copy link
Member

garbear commented May 8, 2013

from the PR description i see a small problem, if a user customizes a keymap it'll be overwritten when we bump the addon version.

a solution might be to also scan the userdata system/keymaps folder, and override the keymap xml data on a per-device and per-window basis

@akva2
Copy link
Contributor

akva2 commented May 13, 2013

@garbear hmm, that's what is done? don't follow. in the per-device load code we append the addon_data folder. it will be loaded last, overriding the system maps. then we inject into the button translator.

@garbear
Copy link
Member

garbear commented May 13, 2013

So users can customize keymaps using addon_data/id/keymaps (vs system/keymaps) now? that's what I was wondering, I missed the code here.

Doesn't addon_data/id only exist under certain conditions, like when XBMC creates an xml for user settings? It might be a little confusing if the user has to manually create the addon_data/id/keymaps folder for each add-on. Maybe the add-on could create the addon_data keymaps folder on installation, and remove empty dirs on uninstillation (if this applies)?

Another problem I see with using addon_data/id/keymaps vs a universal system/keymaps is that it might be difficult for the user to determine which add-on a peripheral belongs to. For example, should a custom keymap for the Logitech Gamepad F710 go in addon_data/peripheral.xbox.360.controller/keymaps or addon_data/peripheral.xbox.controller.s/keymaps?

EDIT: I meant userdata/keymaps instead of system/keymaps

@akva2
Copy link
Contributor

akva2 commented May 14, 2013

users were never supposed to stick their dirty hands and noses in system/keymaps. they were supposed to put override files in userdata/keymaps. now they stick them in the relevant add-on's folder. but they should NEVER be changing the default add-on mappings. just like they should NOT be changing the system/keymaps files in the current "system". in any case, if they "miss" and choose the wrong folder, it will still work as such.

the old chaos is intact as well. but i cba to do the old mess for the new keymap code. if that is required, somebody else will have to do the honors. i want to get away from that, not reinstate it.

@opdenkamp
Copy link
Member

awesome work, big +1
some questions / remarks:

  • i've added some conditionals for the nyx, since some people like the search popup when you flip the remote, and some don't: https://github.com/xbmc/xbmc/blob/master/xbmc/peripherals/devices/PeripheralNyxboard.cpp#L39 . Am i correct that this is not possible after these changes?
  • what i had in mind when we talked about this earlier was adding two fields to the add-on descriptor xml if possible, with a vendor id and product id in it, and have peripheralbususb query addonmanager whether it has any xbmc.peripheral add-ons that match. that way we can remove the mappings in peripherals.xml and only use code in core to scan for devices. but maybe that's something for a follow up pr

@MartijnKaijser
Copy link
Member

@cptspiff
small nudge to get this in this going :)

@ghost
Copy link
Author

ghost commented Aug 6, 2013

then someone else has to spearhead it. all my time goes into binaddons atm.

@opdenkamp
Copy link
Member

i don't have time for this right now either

@MartijnKaijser
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants