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

[peripherals] replace peripherals manager with dialogselect #7872

Merged
merged 4 commits into from Sep 1, 2015

Conversation

Projects
None yet
8 participants
@mkortstiege
Copy link
Member

mkortstiege commented Aug 23, 2015

This nukes the peripherals manager dialog and uses generic dialog select instead.

  • New dialog uses a simple list with just the device name.
  • The peripheral settings dialog header now shows the actual device name rather than just Settings.
  • Peripheral properties (vendor, product, bus, location, class and version) are passed to the settings dialog and can be used as window properties if the skin authors want to show more detailed information.

Before:
screenshot from 2015-08-23 17 57 40

After:
screenshot from 2015-08-23 18 00 16

/cc @Montellese, @phil65, @ronie

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Aug 23, 2015

jenkins build this please.

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Aug 23, 2015

ping @opdenkamp as it's his baby.

@phil65

This comment has been minimized.

Copy link
Member

phil65 commented Aug 23, 2015

+1. If we also achieve to get rid of PeripheralsSettings.xml in the longer term then all skins would provide peripherals support out-of-the-box.

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Aug 23, 2015

@mkortstiege would it be possible to use the textbox in the select dialog to display those device properties?

@opdenkamp

This comment has been minimized.

Copy link
Member

opdenkamp commented Aug 23, 2015

Can't review right now, on my phone and still busy with building the house. The description in the title looks good ;)

-------- Oorspronkelijk bericht --------
Van: Philipp Temminghoff notifications@github.com
Datum:23-08-2015 12:49 (GMT+01:00)
Aan: xbmc/xbmc xbmc@noreply.github.com
Cc: Lars Op den Kamp opdenkamp@gmail.com
Onderwerp: Re: [xbmc] [peripherals] replace peripherals manager with
dialogselect (#7872)

+1. If we also achieve to get rid of PeripheralsSettings.xml in the longer term then all skins would provide peripherals support out-of-the-box.


Reply to this email directly or view it on GitHub.

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Aug 23, 2015

@mkortstiege would it be possible to use the textbox in the select dialog to display those device properties?

Yes, that's possible when using the detailed view, but i think those information are pretty useless and just clutter the list. Not sure if it's worth tbh.

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Aug 23, 2015

Added before/after screenshots to the PR description.

@ronie

This comment has been minimized.

Copy link
Member

ronie commented Aug 23, 2015

i think those information are pretty useless

yeah, it probably is.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Aug 23, 2015

only @opdenkamp knows i guess :) I'm sure those things are also logged?

@opdenkamp

This comment has been minimized.

Copy link
Member

opdenkamp commented Aug 23, 2015

Hmm the version should not be removed. The rest is optional I guess, but the version is important

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Aug 23, 2015

Hmm the version should not be removed. The rest is optional I guess, but the version is important

You're ok with "Devicename (version)" ?

@HitcherUK

This comment has been minimized.

Copy link
Contributor

HitcherUK commented Aug 23, 2015

Version as label2 ;)

@opdenkamp

This comment has been minimized.

Copy link
Member

opdenkamp commented Aug 24, 2015

You're ok with "Devicename (version)" ?

no sorry, the version string is a bit too long for that. e.g. for CEC it's currently "libCEC 3.0.1 - firmware v5 (2015-01-27)" as version string and I don't want that kind of information to be removed

@Paxxi

This comment has been minimized.

Copy link
Member

Paxxi commented Aug 24, 2015

What use does a regular user have for the version information?

@opdenkamp

This comment has been minimized.

Copy link
Member

opdenkamp commented Aug 24, 2015

this is especially for regular users who you don't want to ask to look up these version numbers in a debug log.

@opdenkamp

This comment has been minimized.

Copy link
Member

opdenkamp commented Aug 24, 2015

long term plan is to turn the implementations for these peripherals into proper (binary) add-ons btw, so if you want to re-use existing dialogs, then you can probably use the ones that we use for add-ons, which have a version number and description etc.

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Aug 24, 2015

Latest commit adds the detailed list layout.

screenshot from 2015-08-24 13 43 45

@opdenkamp

This comment has been minimized.

Copy link
Member

opdenkamp commented Aug 24, 2015

that looks ok, perhaps omit the unknown versions. guess this is with mouse support disabled and that's why the icons in the top are missing to close the dialog?

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Aug 24, 2015

that looks ok, perhaps omit the unknown versions.

Tested and that is just looking wrong.

guess this is with mouse support disabled and that's why the icons in the top are missing to close the dialog?

Jep.

@mkortstiege mkortstiege added this to the Jarvis 16.0-alpha3 milestone Aug 25, 2015

@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented Aug 25, 2015

dialog with CEC plugged in
https://db.tt/4o3veDkp

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Aug 25, 2015

dialog with CEC plugged in
https://db.tt/4o3veDkp

Thanks for testing. Looks like it should.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Aug 26, 2015

Testing the latest version of this PR and a disabled CEC adapter (presumably, any disabled input device) will show "Version: Unknown"

screenshot-disabled

Would it be better to indicate that the device is disabled, eg. "Status: Disabled" in place of "Version: Unknown" (or even, though perhaps a little confusing, "Version: Disabled")?

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Aug 26, 2015

How were disabled devices handled before this PR?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Aug 26, 2015

Enabled:
enabled

Disabled:
disabled

So no change in terms of behaviour, but with version now being the only label it's more obvious than before. It's just cosmetic, really.

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Aug 26, 2015

@MilhouseVH just pushed another commit. Mind testing again?

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Aug 26, 2015

Looks good!

Enabled:
enabled

Disabled:
disabled

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Aug 26, 2015

Great. Thanks for testing.

@MilhouseVH

This comment has been minimized.

Copy link
Contributor

MilhouseVH commented Aug 26, 2015

The only remaining comment/observation I have is that once you enable CEC, the Peripheral list continues to show "Status: Disabled" until Kodi is restarted - not sure if this is a peculiarity of CEC or all input devices behave this way. It's the same when disabling CEC - it will continue to show the version information for CEC until you restart Kodi. This behaviour is not new, just more obvious now with this PR.

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Aug 26, 2015

Unfortunately i can't find my CEC Adapter right now. Will search for it again and test it.

@mkortstiege mkortstiege force-pushed the mkortstiege:nuke-peripheralsmanager branch from b19b141 to eacf371 Sep 1, 2015

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Sep 1, 2015

What's the status here? I'd prefer to merge as is. The remaining issues pointed out by @MilhouseVH are out of scope of this PR and should be handled separately.

@mkortstiege

This comment has been minimized.

Copy link
Member Author

mkortstiege commented Sep 1, 2015

jenkins build this please.

MartijnKaijser added a commit that referenced this pull request Sep 1, 2015

Merge pull request #7872 from mkortstiege/nuke-peripheralsmanager
[peripherals] replace peripherals manager with dialogselect

@MartijnKaijser MartijnKaijser merged commit 21ea90b into xbmc:master Sep 1, 2015

1 check passed

default Merged build finished.
Details

@mkortstiege mkortstiege deleted the mkortstiege:nuke-peripheralsmanager branch Oct 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.