Replace system-wide iMON setting with per-device setting #1536

Merged
merged 1 commit into from Dec 13, 2012

Conversation

Projects
None yet
3 participants
@Karlson2k
Member

Karlson2k commented Oct 4, 2012

Addition for #1189
Allow user to set each iMON device as compatible/incompatible with direct input/joystick (this depends on firmware and can't be detected).
When joystick is disabled by iMON presence, disable manual on/off switch in GUI as non-functional (automatic joystick disable have priority over global joystick switch so disabled switch directly indicate disable of joystick).
This way should be clearer for user as user will not try to on/off joystick without any effect.

@opdenkamp

This comment has been minimized.

Show comment Hide comment
@opdenkamp

opdenkamp Oct 4, 2012

Member

yeah missed one thing there. that imon setting can be moved out of guisettings and into the imon class's settings. could you change the PR so it does that instead.

we don't need to bother people with this in guisettings.

Member

opdenkamp commented Oct 4, 2012

yeah missed one thing there. that imon setting can be moved out of guisettings and into the imon class's settings. could you change the PR so it does that instead.

we don't need to bother people with this in guisettings.

@Karlson2k

This comment has been minimized.

Show comment Hide comment
@Karlson2k

Karlson2k Oct 4, 2012

Member

Did you mean move to per-device setting?

Member

Karlson2k commented Oct 4, 2012

Did you mean move to per-device setting?

@opdenkamp

This comment has been minimized.

Show comment Hide comment
@opdenkamp

opdenkamp Oct 4, 2012

Member

yes please

Member

opdenkamp commented Oct 4, 2012

yes please

@ghost ghost assigned opdenkamp Oct 7, 2012

@opdenkamp

This comment has been minimized.

Show comment Hide comment
@opdenkamp

opdenkamp Oct 10, 2012

Member

the we better make the peripheral have an option to disable joystick support, and make it disable the guisetting if it's not doing that yet, so the values won't be conflicting.

Member

opdenkamp commented Oct 10, 2012

the we better make the peripheral have an option to disable joystick support, and make it disable the guisetting if it's not doing that yet, so the values won't be conflicting.

@Karlson2k

This comment has been minimized.

Show comment Hide comment
@Karlson2k

Karlson2k Oct 10, 2012

Member

@opdenkamp Perfect! This is exactly what this PR is doing.

  1. Removed global "iMON device" setting
  2. Added setting to device to disable joystick (default is ON)
  3. If device is present and device setting is ON, than GUI setting for joystick is disabled.
Member

Karlson2k commented Oct 10, 2012

@opdenkamp Perfect! This is exactly what this PR is doing.

  1. Removed global "iMON device" setting
  2. Added setting to device to disable joystick (default is ON)
  3. If device is present and device setting is ON, than GUI setting for joystick is disabled.
@Karlson2k

This comment has been minimized.

Show comment Hide comment
@Karlson2k

Karlson2k Oct 10, 2012

Member

And thanks to flexible peripheral framework, device setting will appear only on win32 platform.

Member

Karlson2k commented Oct 10, 2012

And thanks to flexible peripheral framework, device setting will appear only on win32 platform.

@Karlson2k

This comment has been minimized.

Show comment Hide comment
@Karlson2k

Karlson2k Oct 11, 2012

Member

@opdenkamp Can I improve it somehow before close of merge window?

Member

Karlson2k commented Oct 11, 2012

@opdenkamp Can I improve it somehow before close of merge window?

@opdenkamp

This comment has been minimized.

Show comment Hide comment
@opdenkamp

opdenkamp Oct 11, 2012

Member

i consider it a fix, but @cptspiff needs to give his ok as the merge window is already closed.
and i'd like to review it properly another time first.

Member

opdenkamp commented Oct 11, 2012

i consider it a fix, but @cptspiff needs to give his ok as the merge window is already closed.
and i'd like to review it properly another time first.

@opdenkamp

View changes

xbmc/peripherals/devices/PeripheralImon.cpp
+{
+ if (strChangedSetting.compare("disable_winjoystick")==0)
+ {
+#if defined(TARGET_WINDOWS)

This comment has been minimized.

Show comment Hide comment
@opdenkamp

opdenkamp Oct 30, 2012

Member

do we need this ifdef? m_ImonConflictsWithDInput should never be true on other platforms?
also, please name it m_bImonConflictsWithDInput

@opdenkamp

opdenkamp Oct 30, 2012

Member

do we need this ifdef? m_ImonConflictsWithDInput should never be true on other platforms?
also, please name it m_bImonConflictsWithDInput

This comment has been minimized.

Show comment Hide comment
@Karlson2k

Karlson2k Dec 4, 2012

Member

I'll remove this ifdef.
Moreover, there are no "disable_winjoystick" on other platforms.

@Karlson2k

Karlson2k Dec 4, 2012

Member

I'll remove this ifdef.
Moreover, there are no "disable_winjoystick" on other platforms.

@Karlson2k

This comment has been minimized.

Show comment Hide comment
@Karlson2k

Karlson2k Dec 4, 2012

Member

@opdenkamp Done + rebased.

Member

Karlson2k commented Dec 4, 2012

@opdenkamp Done + rebased.

@Karlson2k

This comment has been minimized.

Show comment Hide comment
@Karlson2k

Karlson2k Dec 5, 2012

Member

@opdenkamp As strings are already translated, should I pick other string ID?

Member

Karlson2k commented Dec 5, 2012

@opdenkamp As strings are already translated, should I pick other string ID?

@opdenkamp

This comment has been minimized.

Show comment Hide comment
@opdenkamp

opdenkamp Dec 6, 2012

Member

@alanwww1 does transifex handle changed strings?

Member

opdenkamp commented Dec 6, 2012

@alanwww1 does transifex handle changed strings?

@Karlson2k

This comment has been minimized.

Show comment Hide comment
@Karlson2k

Karlson2k Dec 7, 2012

Member

@opdenkamp Besides translated strings, is all other code OK?

Member

Karlson2k commented Dec 7, 2012

@opdenkamp Besides translated strings, is all other code OK?

@opdenkamp

This comment has been minimized.

Show comment Hide comment
@opdenkamp

opdenkamp Dec 10, 2012

Member

yeah fine with me

Member

opdenkamp commented Dec 10, 2012

yeah fine with me

@Karlson2k

This comment has been minimized.

Show comment Hide comment
@Karlson2k

Karlson2k Dec 10, 2012

Member

@opdenkamp Nice!
May be it's faster and better to pick other ID for string?

Member

Karlson2k commented Dec 10, 2012

@opdenkamp Nice!
May be it's faster and better to pick other ID for string?

@opdenkamp

This comment has been minimized.

Show comment Hide comment
@opdenkamp

opdenkamp Dec 10, 2012

Member

yeah please do

Member

opdenkamp commented Dec 10, 2012

yeah please do

@Karlson2k

This comment has been minimized.

Show comment Hide comment
@Karlson2k

Karlson2k Dec 10, 2012

Member

@opdenkamp Done.

Member

Karlson2k commented Dec 10, 2012

@opdenkamp Done.

@opdenkamp

This comment has been minimized.

Show comment Hide comment
@opdenkamp

opdenkamp Dec 13, 2012

Member

ok with me so over to @davilla: removes the change in guisettings that was done for this, but i fine with me too if it needs to wait until .1

Member

opdenkamp commented Dec 13, 2012

ok with me so over to @davilla: removes the change in guisettings that was done for this, but i fine with me too if it needs to wait until .1

@ghost ghost assigned davilla Dec 13, 2012

@davilla

This comment has been minimized.

Show comment Hide comment
@davilla

davilla Dec 13, 2012

Contributor

does this fix an existing problem or issue ?

Contributor

davilla commented Dec 13, 2012

does this fix an existing problem or issue ?

@opdenkamp

This comment has been minimized.

Show comment Hide comment
@opdenkamp

opdenkamp Dec 13, 2012

Member

the settings changes between eden and now for this thing could be confusing for users. this make it a setting per device (if you're using an imon device on windows). so yeah, it fixes an issue

Member

opdenkamp commented Dec 13, 2012

the settings changes between eden and now for this thing could be confusing for users. this make it a setting per device (if you're using an imon device on windows). so yeah, it fixes an issue

@Karlson2k

This comment has been minimized.

Show comment Hide comment
@Karlson2k

Karlson2k Dec 13, 2012

Member

@davilla This is a fix for iMON/joystick fix (PR #1189).
If someone have two sets of iMON devices: one with buggy firmware and another with proper firmware, than with this fix it's possible to run single installation of XBMC on both hardware sets. But it's definitely rare situation.
And, as @opdenkamp pointed, settings introduced in #1189 (post-Eden) could confuse users.

Member

Karlson2k commented Dec 13, 2012

@davilla This is a fix for iMON/joystick fix (PR #1189).
If someone have two sets of iMON devices: one with buggy firmware and another with proper firmware, than with this fix it's possible to run single installation of XBMC on both hardware sets. But it's definitely rare situation.
And, as @opdenkamp pointed, settings introduced in #1189 (post-Eden) could confuse users.

davilla added a commit that referenced this pull request Dec 13, 2012

Merge pull request #1536 from Karlson2k/Imon_addition_01
Replace system-wide iMON setting with per-device setting

@davilla davilla merged commit 6671312 into xbmc:master Dec 13, 2012

@Karlson2k Karlson2k deleted the Karlson2k:Imon_addition_01 branch Oct 15, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment