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

Add Volume to Game OSD settings #12765

Merged
merged 1 commit into from Sep 28, 2017
Merged

Add Volume to Game OSD settings #12765

merged 1 commit into from Sep 28, 2017

Conversation

garbear
Copy link
Member

@garbear garbear commented Sep 2, 2017

This adds a Volume setting to the game OSD.

The main reason was to show the hotkey in the GUI, but I decided to make the volume setting clickable.

How Has This Been Tested?

Tested on Windows.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@garbear
Copy link
Member Author

garbear commented Sep 3, 2017

Pushed a commit removing a global from the code. @FernetMenta is the change to the player callback OK, or should we add a second player callback for volume?

@FernetMenta
Copy link
Contributor

this looks like hard to understand spaghetti code to me. you register a player method into GUI and call back into application, that fires a thread msg to set volume?? (if I interpret this correctly).
volume is set from application to player, not the other way round.

@garbear
Copy link
Member Author

garbear commented Sep 3, 2017

Volume is set from application to player. Player fires a GUI msg to update the GUI. No architectural dependencies are violated.

@FernetMenta
Copy link
Contributor

FernetMenta commented Sep 3, 2017

No architectural dependencies are violated.

I see it differently. You created a circular dependency between player and application. Creating GetVolume / SetVolume in IPlayerCallback is no-go. Concatenating callbacks is still bad enough.

EDIT:

Player fires a GUI msg to update the GUI

Player should not be aware of GUI

@garbear
Copy link
Member Author

garbear commented Sep 3, 2017

Creating GetVolume / SetVolume in IPlayerCallback is no-go

Thanks, that's what I was asking. I'll adjust the PR.

@Rechi
Copy link
Member

Rechi commented Sep 15, 2017

@garbear this needs a rebase

@garbear
Copy link
Member Author

garbear commented Sep 15, 2017

Rebased. I was able to solve the gui dependency in #12715, once that gets merged I'll port the fix to this PR.

@garbear garbear force-pushed the game-volume branch 2 times, most recently from 5741fb1 to 8c28e91 Compare September 27, 2017 18:17
@garbear
Copy link
Member Author

garbear commented Sep 27, 2017

Circular dependency has been fixed. Good call, impact is much less. Jenkins build this please.

@garbear garbear merged commit 75304cf into xbmc:master Sep 28, 2017
@garbear garbear deleted the game-volume branch September 28, 2017 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants