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

[chg] add guisetting to configure number of volume control steps #9617

Merged
merged 1 commit into from Apr 15, 2016

Conversation

Projects
None yet
6 participants
@stefansaraev
Copy link
Contributor

commented Apr 13, 2016

lets try again.

@zag2me

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2016

Thanks for this. A huge improvement to customization of Kodi.

@zag2me

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2016

Sorry if I am bike-shedding and you go round in circles ;) but it seems a GUI setting would be nice.
We already have settings for skip steps so I think this could be added under expert mode?
Again apologies if this is already discussed, I get the feeling you guys already went through this. I'm just speaking from the users point of view here.

@stefansaraev stefansaraev force-pushed the stefansaraev:volumesteps branch from d5d22bf to e0029d9 Apr 13, 2016

@stefansaraev

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2016

@zag2me done. may need better help hint. I am not a native speaker so suggestions welcome.

@Montellese could you please check for stupids?

@Montellese side question: if I understand right, settings system ensures that a min/max value is guaranteed. but as distros may override via appliance.xml and pass invalid values (<10 || > 90), should I do some sanity check? (EDIT: yep. added..)

@zag2me

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2016

English string sounds good to me 👍

@stefansaraev stefansaraev force-pushed the stefansaraev:volumesteps branch from f9e7b30 to 0279b24 Apr 13, 2016

@stefansaraev stefansaraev changed the title [chg] add advancedsetting to configure number of volume control steps [chg] add guisetting to configure number of volume control steps Apr 13, 2016

@stefansaraev stefansaraev force-pushed the stefansaraev:volumesteps branch from 0279b24 to c76f0c7 Apr 13, 2016

@@ -2296,11 +2296,18 @@ bool CApplication::OnAction(const CAction &action)
if (m_muted)
UnMute();
float volume = m_volumeLevel;
int volumesteps = CSettings::GetInstance().GetInt(CSettings::SETTING_AUDIOOUTPUT_VOLUMESTEPS);
// appliance.xml may contain invalid values. sanity check

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Apr 14, 2016

Member

this should not be required. the settings systems takes care for limits.

This comment has been minimized.

Copy link
@stefansaraev

stefansaraev Apr 14, 2016

Author Contributor

not really. hence the appliance.xml comment.

This comment has been minimized.

Copy link
@stefansaraev

stefansaraev Apr 14, 2016

Author Contributor

try this appliance.xml and see what happens.

<settings>
  <section id="system">
    <category id="audiooutput">
      <group id="3">
        <setting id="audiooutput.volumesteps">
          <default>0</default>
          <constraints>
            <minimum>-10</minimum>
            <step>1</step>
            <maximum>200</maximum>
          </constraints>
        </setting>
      </group>
    </category>
  </section>
</settings>

This comment has been minimized.

Copy link
@Montellese

Montellese Apr 14, 2016

Member

Do we really need to guard against that? An appliance doing something like this basically criples its own system which is certainly not in their interest. The only check really necessary would be for volumesteps to not be 0 to avoid a division by zero problem.

<constraints>
<minimum>10</minimum>
<step>5</step>
<maximum>90</maximum>

This comment has been minimized.

Copy link
@Montellese

Montellese Apr 14, 2016

Member

Is 90 the current default or is it 100? If it's 100 I would allow 100 as well.

This comment has been minimized.

Copy link
@stefansaraev

stefansaraev Apr 14, 2016

Author Contributor

current is 90.

@Montellese

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

settings stuff looks ok.

@stefansaraev stefansaraev force-pushed the stefansaraev:volumesteps branch from cd15cb4 to 524c2ba Apr 14, 2016

@stefansaraev

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2016

updated.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Apr 14, 2016

+1

@stefansaraev

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2016

jenkins build this please

@fritsch

This comment has been minimized.

Copy link
Member

commented Apr 15, 2016

WIN32 machine seems to have hung ... builds since 10 hours. Good to go :-)

@stefansaraev stefansaraev merged commit 74020b2 into xbmc:master Apr 15, 2016

0 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build cancelled
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
default Merged build finished.
Details

@stefansaraev stefansaraev deleted the stefansaraev:volumesteps branch Apr 15, 2016

@Razzeee Razzeee added this to the Krypton 17.0-alpha1 milestone Apr 15, 2016

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.