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: improve migration of audio output settings from (pre-)Eden to Frodo #1828

Merged
merged 4 commits into from Nov 22, 2012

Conversation

Montellese
Copy link
Member

These commits improve the migration of the audiooutput settings from (pre-)Eden to Frodo by replacing audiooutput.channellayout with audiooutput.channels (the meaning of the values have changed) and uses an existance check on audiooutput.channellayout to realize that we are performing an update to Frodo. That way we can actually migrate the value from audiooutput.channellayout to audiooutput.channels and also reset the value of audiooutput.audiodevice to its default value because that value is not migrateable either (at least not on win32).

There's also a fix in CGUIWindowSettingsCategory which currently shows the old value in the list of choices for audiooutput.audiodevice. IMO presenting the user with a non-existing and wrong option to choose from is a very bad idea as there's no indication in the GUI that this value is invalid.

@jmarshallnz
Copy link
Contributor

Looks OK, but there'll need to be similar changes for CoreAudioAE, PulseAE ?

@DDDamian
Copy link
Contributor

See my last comment on your PR #1826 re the audiooutput.audiodevice and the upgrade - wrote it just before I saw this.

All the other functions in AEFactory are defined at the engine level, just curious why not here. Ofc it eliminates the need to define in each engine which is a big plus, and there's nothing engine/platform dependent about the VerifyOutputDevice function, but not sure we can't always guarentee that.

Otherwise good catch on the channellayout redefinition.

@Montellese
Copy link
Member Author

@jmarshalllnz: This implementation is located in CAEFactory and not in one of the engines.

@DDDamian: Yeah I realized that as well but I didn't see any use in duplicating the same code at least three times as it operates on AEDeviceList and AEDevice which are engine-independent. Currently only CSoftAE seems to have such code and it could probably be removed but I didn't wanna do that to keep this as low-impact as possible.

@jmarshallnz
Copy link
Contributor

I was referring to your change in SoftAE (OnSettingsChanged) - perhaps this
isn't used in the other engines?

On Thu, Nov 22, 2012 at 8:26 PM, Sascha Montellese <notifications@github.com

wrote:

@jmarshalllnz: This implementation is located in CAEFactory and not in one
of the engines.

@DDDamian https://github.com/DDDamian: Yeah I realized that as well but
I didn't see any use in duplicating the same code at least three times as
it operates on AEDeviceList and AEDevice which are engine-independent.
Currently only CSoftAE seems to have such code and it could probably be
removed but I didn't wanna do that to keep this as low-impact as possible.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1828#issuecomment-10625720.

@Montellese
Copy link
Member Author

Ah sorry didn't realize it. My bad as VS only shows me hits in files it has in the project. Will hunt down the others and update the commit.

@Montellese
Copy link
Member Author

Done. According to git grep it was only used in SoftAE and CoreAudio but not in Pulse.

@DDDamian
Copy link
Contributor

@Montellese - I hear ya - there should only be one engine and smarter sinks ;)

Green light here.

@davilla
Copy link
Contributor

davilla commented Nov 22, 2012

ok to merge.

Montellese added a commit that referenced this pull request Nov 22, 2012
settings: improve migration of audio output settings from (pre-)Eden to Frodo
@Montellese Montellese merged commit 4709873 into xbmc:master Nov 22, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Fix non-breaking change which fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants