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

Merge settings dialogs xml's #8856

Merged
merged 3 commits into from
Mar 6, 2016
Merged

Merge settings dialogs xml's #8856

merged 3 commits into from
Mar 6, 2016

Conversation

ronie
Copy link
Member

@ronie ronie commented Jan 12, 2016

in the past we've been adding a new settings dialog xml for every new future.
as a result there are now a lot of xml files in the skin that are (near) identical.

all those dialogs could (should) use a common settings dialog: DialogSettings.xml

this merges the following settings dialogs:

  • DialogAudioDSPSettings.xml
  • DialogContentSettings.xml
  • DialogMediaFilter.xml
  • DialogNetworkSetup.xml
  • DialogPeripheralSettings.xml
  • DialogPVRTimerSettings.xml
  • LockSettings.xml
  • ProfileSettings.xml
  • VideoOSDSettings.xml

@ronie ronie added RFC PR submitted for gathering feedback Component: Skin labels Jan 12, 2016
@Montellese
Copy link
Member

Nice. This is part of what I had as a long term goal when I unified the settings dialog's code.

@Hitcher
Copy link
Contributor

Hitcher commented Jan 12, 2016

Great stuff, thanks ronie.

@BigNoid
Copy link
Member

BigNoid commented Jan 13, 2016

Nice cleanup :) I have one remark though. If the ultimate goal is to have one dialog for all setting dialogs, maybe we can use a default template with 3 groups, like addonsettings has, and just use a default label/button named general if the category group is not used. That would future proof this dialog and would get rid of the addonsettings dialog too. I have no idea how viable this is, so feel free to ignore.

@ronie ronie added the WIP PR that is still being worked on label Jan 13, 2016
@ronie
Copy link
Member Author

ronie commented Jan 13, 2016

@BigNoid it's certainly an idea worth considering.

currently only addon settings uses those category buttons.
but it might be useful for the osdaudiosettings dialog as well,
to split it into audio / subtitle related categories.

@Montellese
Copy link
Member

Keep in mind that the addon settings dialog isn't based on the same implementation as all the other setting dialogs. But the functionality for category buttons is also supported by the base implementation for setting dialogs it's just not used anywhere.

@ronie ronie force-pushed the dialogsettings branch 4 times, most recently from f9b1791 to 4c8df51 Compare January 13, 2016 22:22
@ronie
Copy link
Member Author

ronie commented Jan 13, 2016

added a commit which also makes the contentsettings dialog use DialogSettings.xml

needs proper review!

@ronie ronie force-pushed the dialogsettings branch 4 times, most recently from 4d99a3b to 0bad496 Compare January 14, 2016 17:10
@ronie ronie removed the WIP PR that is still being worked on label Jan 14, 2016
@MartijnKaijser
Copy link
Member

jenkins build this please

@ronie
Copy link
Member Author

ronie commented Jan 19, 2016

@mkortstiege get out of the beer spa and review my code please ;-)

@mkortstiege
Copy link
Member

Looks good to me. Btw, Unlimited Beer SPA!

@ronie
Copy link
Member Author

ronie commented Jan 20, 2016

@Montellese is there a way to define parent/sub settings in the settings dialogs?

i noticed there's a GetParent() function here: https://github.com/xbmc/xbmc/blob/master/xbmc/settings/dialogs/GUIDialogSettingsBase.cpp#L583-L608
but i can't find how to set them.

i have a number of settings defined here:
ronie@8cf6dd2#diff-68de0b073df124e07ac952d978e55bd3L47
and i would like to make one of them a subsetting of another.

the reason is, i'd like to prefix the label of the subsetting with "- "

@Montellese
Copy link
Member

If you have the CSetting based object you can use SetParent() to set the parent using its setting ID.

@ronie ronie force-pushed the dialogsettings branch 5 times, most recently from a6cc4d7 to dab97b4 Compare January 23, 2016 01:03
@ronie
Copy link
Member Author

ronie commented Jan 23, 2016

@Montellese thx for the pointer :-)
mind checking ronie@436d9a8 if i didn't screw up?

added two additional commits:

@ronie
Copy link
Member Author

ronie commented Jan 31, 2016

updated as @Montellese suggested: ronie@daa1e51

needs review: ronie@e748cec
@xhaggi perhaps?

xhaggi
xhaggi reviewed Feb 1, 2016
View reviewed changes
xbmc/profiles/dialogs/GUIDialogProfileSettings.h
@@ -57,7 +57,7 @@ class CGUIDialogProfileSettings : public CGUIDialogSettingsManualBase
*/
static bool GetProfilePath(std::string &directory, bool isDefault);

void updateProfileName();
void updateProfileImage();
void updateProfileDirectory();

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@razzeee
Copy link
Member

razzeee commented Feb 18, 2016

Would really like to see this merged.

@ronie
Copy link
Member Author

ronie commented Feb 19, 2016

sure, sure :-)

hopefully i'll have some time (read: motivation) to look into the remaining issues next week.

@ronie ronie force-pushed the dialogsettings branch 3 times, most recently from a83d345 to ff2ff2d Compare March 2, 2016 01:16
@ronie
Copy link
Member Author

ronie commented Mar 3, 2016

jenkins build this please

1 similar comment
@ronie
Copy link
Member Author

ronie commented Mar 5, 2016

jenkins build this please

@ronie ronie removed the RFC PR submitted for gathering feedback label Mar 6, 2016
ronie added a commit that referenced this pull request Mar 6, 2016
@ronie
Merge pull request #8856 from ronie/dialogsettings
9a0feee 
Merge settings dialogs xml's
@ronie ronie merged commit 9a0feee into xbmc:master Mar 6, 2016
cg110
cg110 reviewed Mar 10, 2016
View reviewed changes
xbmc/settings/dialogs/GUIDialogContentSettings.cpp
subsetting->SetParent(SETTING_SCRAPER_LIST);

group = AddGroup(category, 20322);
if (category == NULL)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

un1versal added a commit to un1versal/skin.confluence that referenced this pull request Mar 11, 2016
@un1versal
rename unknown-user.png -> DefaultUser.png
f61779c 
part of xbmc/xbmc#8856
un1versal added a commit to un1versal/revamped.themes that referenced this pull request Mar 11, 2016
@un1versal
rename unknown-user.png -> DefaultUser.png
39efbac 
part of xbmc/xbmc#8856
un1versal added a commit to un1versal/revamped.themes that referenced this pull request Mar 11, 2016
@un1versal
rename unknown-user.png -> DefaultUser.png
1d7474e 
part of xbmc/xbmc#8856
@ronie ronie deleted the dialogsettings branch October 15, 2016 16:33
@Hitcher Hitcher mentioned this pull request Nov 20, 2016
Closed
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
Component: Skin
Projects
None yet
Milestone
Krypton 17.0-alpha1
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@ronie @Montellese @Hitcher @BigNoid @MartijnKaijser @mkortstiege @razzeee @xhaggi @cg110