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

[SETTINGS] Add Source settings to call source selection windows... #10199

Merged
merged 5 commits into from
Jul 31, 2016

Conversation

jjd-uk
Copy link
Member

@jjd-uk jjd-uk commented Jul 31, 2016

.... and rename Library settings section to Media to allow creation of Media -> Library category to contain new Source settings and previous Database settings.

This was inspired by @phil65 "Media sources" item in Estuary, so this add similar ability to core in order to be available to everyone and not just Estuary users.

image

image

These changes would allow removal of "Media sources" from Estuary as this function could now be handled by core, however didn't want to be presumptuous about doing that myself, so will leave that decision to the skinners.

I've marked this as [WIP] since I've renamed the Library section to Media, however to be done in a 100% proper manner the underlying window would also need to be renamed, for example:

https://github.com/xbmc/xbmc/blob/master/xbmc/guilib/WindowIDs.h#L46

WINDOW_SETTINGS_LIBRARY to WINDOW_SETTINGS_MEDIA

However this would break skins already updated to use the ActivateWindow(LibrarySettings) call in their skin Settings.xml, so would require changing to ActivateWindow(MediaSettings) if the window was renamed.

Thus at the moment I've played it safe and only included the labelling changes and I've NOT yet renamed the underlying window name in code to avoid breaking skins. If this is to be merged for Krypton a decision is needed on whether we:

  • rename window to WINDOW_SETTINGS_MEDIA now and break skins ActivateWindow(LibrarySettings) call
  • keep WINDOW_SETTINGS_LIBRARY and ActivateWindow(LibrarySettings) for Krypton then rename to WINDOW_SETTINGS_MEDIA in v18

ping @MartijnKaijser @Paxxi and additional pings for opinions on whether to do the window rename at this point of the release cycle @ronie @BigNoid

@Paxxi
Copy link
Member

Paxxi commented Jul 31, 2016

Too late in the release cycle to change the window name in my opinion, tag it with a todo and schedule it for v18 alpha 1

@jjd-uk
Copy link
Member Author

jjd-uk commented Jul 31, 2016

jenkins build this please

@jjd-uk
Copy link
Member Author

jjd-uk commented Jul 31, 2016

Just occured to me, I could do the Window renaming now in a seperate PR then that PR could either be accepted for Krypton (assuming this one goes in), or the merger of that PR is delayed until we've branched Krypton and Master is open for v18. That way no one can raise the concern that it'll be forgotten about if delayed.

@MartijnKaijser
Copy link
Member

I like the change. I'll leave it up to @ronie and skinners as he can tell if skins already use the name. If not I'd see no issue including it.

@ronie
Copy link
Member

ronie commented Jul 31, 2016

i'm fine with the window name change as well.
we're still in alpha, so no problem with skin breakage.

@Hitcher
Copy link
Contributor

Hitcher commented Jul 31, 2016

It's no big deal to change ActivateWindow(LibrarySettings) to ActivateWindow(MediaSettings)

@jjd-uk jjd-uk force-pushed the media branch 2 times, most recently from c8fe5a4 to 7fdd829 Compare July 31, 2016 14:17
@jjd-uk jjd-uk changed the title [WIP] Add Source settings to call source selection windows... [SETTINGS] Add Source settings to call source selection windows... Jul 31, 2016
@jjd-uk
Copy link
Member Author

jjd-uk commented Jul 31, 2016

PR updated to include the underlying code changes necessary to rename the window from LIBRARY to MEDIA.

jenkins build this please

@jjd-uk
Copy link
Member Author

jjd-uk commented Jul 31, 2016

Windows test build if anyone wants to see this change http://mirrors.xbmc.org/test-builds/win32/KodiSetup-20160731-b94a377-media.exe

@MartijnKaijser
Copy link
Member

Looks good but shouldn't the media sources button be removed?

@jjd-uk
Copy link
Member Author

jjd-uk commented Jul 31, 2016

After this is merged the "Media sources" button is indeed not required, but I took that as being a skin decision as to whether @phil65 @ronie @BigNoid want to keep it or not, don't want to intrude too much on other peoples turf ;)

@ronie
Copy link
Member

ronie commented Jul 31, 2016

i'd say ditch it :)

@Hitcher
Copy link
Contributor

Hitcher commented Jul 31, 2016

@jjd-uk Might want to add descriptions for each item as well for the textbox ID(6)

@jjd-uk
Copy link
Member Author

jjd-uk commented Jul 31, 2016

@HitcherUK do you mean the Help descriptions? if so now added.

@MartijnKaijser
Copy link
Member

@jjd-uk you can add the removal of the skin button in this PR or make it a separate one. Up to you.

@jjd-uk
Copy link
Member Author

jjd-uk commented Jul 31, 2016

I'd prefer not to do it without phils ok, so I could PR the button removal to the Estuary repo

@jjd-uk
Copy link
Member Author

jjd-uk commented Jul 31, 2016

Jenkins has gone missing since my last force push, just to be sure

jenkins build this please

@MartijnKaijser MartijnKaijser added this to the Krypton 17.0-alpha3 milestone Jul 31, 2016
@MartijnKaijser MartijnKaijser added the Type: Feature non-breaking change which adds functionality label Jul 31, 2016
@MartijnKaijser MartijnKaijser merged commit 36bb23e into xbmc:master Jul 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature non-breaking change which adds functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants