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.

Sign up for GitHub

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

Jump to bottom

[python] add multi-select dialog #7750

Merged
merged 3 commits into from Aug 18, 2015
Merged

[python] add multi-select dialog #7750

merged 3 commits into from Aug 18, 2015

Conversation

tamland
Copy link
Member

@tamland tamland commented Aug 7, 2015

Exposes the multi-select dialog to python. Uses indices for consistency with the single select api.

@tamland tamland added Type: Feature non-breaking change which adds functionality API change labels Aug 7, 2015
@hudokkow
Copy link
Member

hudokkow commented Aug 8, 2015

jenkins build this please

@tamland
Copy link
Member Author

tamland commented Aug 14, 2015

@mkortstiege /@xhaggi for review please. There are core changes as well here.

@mkortstiege
Copy link
Member

Really not sure about the codegenerator one but changes looking good to me otherwise. Please don't forget to bump the API version after this went in or the merge window closes.

@Montellese
Copy link
Member

I'll try to take a look at the codegenerator and Python API changes over the weekend.

@tamland
Copy link
Member Author

tamland commented Aug 14, 2015

After checking the API I believe Player::getAvailableSubtitleStreams and Player::getAvailableAudioStreams are the only two returning vector pointer. They would change from returning empty list to None on failure.

Either we consider that a bug and change the API, or change the c++ code to return empty vector instead of null (with the generator change we can now do both).

@xhaggi
Copy link
Member

xhaggi commented Aug 14, 2015

apart from the minors this looks good, thanks 👍

@Montellese
Copy link
Member

IMO in case of Player::getAvailableSubtitleStreams and Player::getAvailableAudioStreams I don't see what the advantage of returning None over an empty array/list would be. It doesn't have a different meaning and IMO getting an empty list is more intuitive. I don't even understand why it's implemented to return a pointer-to-vector instead of just vector.

@tamland tamland force-pushed the python_multiselect branch 2 times, most recently from 51137e2 to e01f61a Compare August 14, 2015 15:41
@tamland
Copy link
Member Author

tamland commented Aug 14, 2015

Updated. all good?

@xhaggi
Copy link
Member

xhaggi commented Aug 14, 2015

looks good from my side

jimfcarroll
jimfcarroll reviewed Aug 14, 2015
View reviewed changes
xbmc/interfaces/legacy/Player.cpp
@@ -426,26 +426,26 @@ namespace XBMCAddon
}
}

std::vector<String>* Player::getAvailableSubtitleStreams()
std::vector<String> Player::getAvailableSubtitleStreams()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tamland
Copy link
Member Author

tamland commented Aug 16, 2015

jenkins build this please

@tamland
Copy link
Member Author

tamland commented Aug 17, 2015

jenkins build this please and please make an effort this time

@tamland
[gui] change CGUIDialogSelect::GetSelectedItems to return indices
e002a1b
@tamland
[python] add multiselect dialog
eb6676a
@tamland
[codegenerator] return None for null pointer, not empty vector
29b2a13 
change getAvailableAudioStreams and getAvailableSubtitleStreams to
ensure they return empty vector instead of null (for api compatibility)
@tamland tamland merged commit 29b2a13 into xbmc:master Aug 18, 2015
@hudokkow hudokkow added this to the Jarvis 16.0-alpha2 milestone Aug 18, 2015
@popcornmix
Copy link
Member

There is a bug reported on nightly build:
"There is a bug in latest releases that user can not change groups under TV/group (slides from right) the groups (view options). Even if you select it, it does not change."

I'm just guessing but this looked like the most likely commit that day to cause it.
http://forum.kodi.tv/showthread.php?tid=231092&pid=2084729#pid2084729
http://forum.kodi.tv/showthread.php?tid=231092&pid=2085191#pid2085191

@roee88
Copy link

roee88 commented Feb 10, 2016

An option to specify pre-selected items would make this more useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Type: Feature non-breaking change which adds functionality
Projects
None yet
Milestone
Jarvis 16.0-alpha2
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@tamland @hudokkow @mkortstiege @Montellese @xhaggi @popcornmix @roee88 @jimfcarroll @ace20022