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 layout changes improvements #9208

Merged
merged 5 commits into from May 1, 2016

Conversation

@jjd-uk
Copy link
Member

commented Feb 25, 2016

This is a refactor of the layout of Settings inspired by the discussions on http://forum.kodi.tv/showthread.php?tid=189031

For Team members I've open a thread in our forums so discussion of the details can be made there to avoid people getting bombarded with too much github spam, as I imagine this will generate a lot of discussion.

To make sure I've not messed up the platform specific xmls (as this has only been tested on Windows) and any other comments @Memphiz @popcornmix @koying @Paxxi @FernetMenta

Also for UI apsects @da-anda @ronie @phil65 @HitcherUK

<onclick>ActivateWindow(AppearanceSettings)</onclick>
<label>14200</label>
<label2>31430</label2>
<onclick>ActivateWindow(PlayerSettings)</onclick>

This comment has been minimized.

Copy link
@phil65

phil65 Feb 25, 2016

Member

tabs vs. spaces.

@phil65

This comment has been minimized.

Copy link
Member

commented Feb 25, 2016

Lot of small formatting issues in those XMLs. I would suggest to use tools to auto-format and verify XMLs after bikeshedding has ended. ;)

@Tolriq

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2016

If there's a rework can I ask something about the Settings level access ?

I find not very logical the navigation to it right now, you can't press up from 1st line to access it, you need to get to bottom then down, then if you press down again you return to last item and not first item. And if pressing down again you return to settings level.

While when pressing up you can do a loop in all categories but the settings level. This breaks a little the navigation pattern.

@phil65

This comment has been minimized.

Copy link
Member

commented Feb 25, 2016

@Tolriq : that's unrelated to this PR. To get that done properly, someone would have to implement SetFocus(id,pos) for grouplists.

@jjd-uk jjd-uk changed the title [RFC] Settings layout chages [RFC] Settings layout changes Feb 25, 2016
@bossanova808

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2016

If you're going to do all this re-arranging, surely a good time to move the crazily undiscoverable

Skin

  • Settings

to somewhere more obvious/sensible. I've seen people using Kodi for years miss that one...

@jjd-uk jjd-uk force-pushed the jjd-uk:settings_layout_pr branch from 3c6617f to c18c3a3 Mar 5, 2016
@jjd-uk jjd-uk closed this Mar 5, 2016
@jjd-uk jjd-uk force-pushed the jjd-uk:settings_layout_pr branch from c18c3a3 to c4bdedf Mar 5, 2016
@jjd-uk jjd-uk reopened this Mar 5, 2016
@jjd-uk jjd-uk force-pushed the jjd-uk:settings_layout_pr branch 3 times, most recently from c68e715 to 1374e24 Mar 13, 2016
@jjd-uk jjd-uk force-pushed the jjd-uk:settings_layout_pr branch 6 times, most recently from 717809b to c177cbb Mar 26, 2016
@Razzeee

This comment has been minimized.

Copy link
Member

commented Mar 27, 2016

Whats the status here?

@jjd-uk jjd-uk force-pushed the jjd-uk:settings_layout_pr branch 2 times, most recently from 71b0cdc to 8e85237 Mar 28, 2016
@jjd-uk

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2016

I'm at a stage now where I need feedback from the Team, also I've not yet tested on any platform other than Windows so help with that is required as I've altered the platform specific xml's blindly (android.xml, darwin.xml etc).

I've rebased on master as it was yesterday and just waiting for jenkins (http://jenkins.kodi.tv/view/Helpers/job/BuildMulti-PR-Manually/213/) to complete so I can post some up to date test builds to the forum.

@jjd-uk jjd-uk force-pushed the jjd-uk:settings_layout_pr branch 2 times, most recently from 2412cf7 to e2e0e16 Mar 31, 2016
@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

Plenty of +1 on the forum for this change. Can we get it rebased and merged?

@jjd-uk jjd-uk closed this Apr 30, 2016
@jjd-uk jjd-uk force-pushed the jjd-uk:settings_layout_pr branch from 1720438 to b6ddce8 Apr 30, 2016
@jjd-uk jjd-uk reopened this Apr 30, 2016
@jjd-uk jjd-uk force-pushed the jjd-uk:settings_layout_pr branch 4 times, most recently from 590019f to 5b5980e Apr 30, 2016
@jjd-uk

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2016

jenkins build this please

@MartijnKaijser MartijnKaijser removed the RFC label May 1, 2016
@MartijnKaijser MartijnKaijser changed the title [RFC] Settings layout changes Settings layout changes improvements May 1, 2016
@jjd-uk jjd-uk force-pushed the jjd-uk:settings_layout_pr branch 4 times, most recently from dfd3099 to 71ee450 May 1, 2016
@jjd-uk jjd-uk force-pushed the jjd-uk:settings_layout_pr branch from 71ee450 to d356178 May 1, 2016
@MartijnKaijser MartijnKaijser merged commit 8203843 into xbmc:master May 1, 2016
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@MartijnKaijser MartijnKaijser added this to the Krypton 17.0-alpha1 milestone May 1, 2016
@jjd-uk

This comment has been minimized.

Copy link
Member Author

commented May 1, 2016

@phil65 Martijn was keen to get this in so it's been merged with some Estuary changes I did to allow testing, so you may want to review them.

  • For the icons I just reused what you already had for the skin, so you may wish to change these.
  • For the ordering I put what I felt were like items together, so top row is the media related items, then second row I wanted to keep Interface (as this is all things related to the GUI) and Skin settings together.
  • For the strings I've added two versions of the strings for each settings category e.g. "Library" and "Library settings" so skins can choose whatever they want. I partly did this since in Estuary we have non-settings stuff like "System Info", so I wanted to clearly distinguish between that and "System" settings, plus since you moved "Skin settings" to be in the main category list I thought it added consistency.

You however may disagree so do whatever you feel is appropriate as I'm no skinner.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented May 2, 2016

well done!

SETTING_VIDEOPLAYER_PAUSEAFTERREFRESHCHANGE is obsolete

@jjd-uk

This comment has been minimized.

Copy link
Member Author

commented May 2, 2016

Thanks FernetMenta.

I did try and follow everything that was getting changed in Master each time I was rebasing. Let me know if there's anything else that needs to be addressed.

@phil65

This comment has been minimized.

Copy link
Member

commented May 2, 2016

I will take a look at skin-related stuff, thx for info.

@AlwinEsch

This comment has been minimized.

Copy link
Member

commented May 14, 2016

Hi @MartijnKaijser was a bit hard to find :) but why you have changed DSP enable/disable to level 3
has a bit confused me ;)

        <setting id="audiooutput.dspaddonsenabled" type="boolean" label="36441" help="36438">
          <level>3</level>
          <default>false</default>
          <dependencies>
            <dependency type="visible">
              <or>
                <condition on="property" name="aesettingvisible" setting="audiooutput.passthrough">audiooutput.dspaddonsenabled</condition>
                <condition on="property" name="aesettingvisible" setting="audiooutput.ac3passthrough">audiooutput.dspaddonsenabled</condition>
              </or>
            </dependency>
          </dependencies>
@jjd-uk jjd-uk deleted the jjd-uk:settings_layout_pr branch Jun 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.