-
Notifications
You must be signed in to change notification settings - Fork 344
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 grid view scroll sound and adjust sound theming #669
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
#include "Settings.h" | ||
#include "SystemData.h" | ||
#include "Window.h" | ||
#include "Sound.h" | ||
|
||
// buffer values for scrolling velocity (left, stopped, right) | ||
const int logoBuffersLeft[] = { -5, -2, -1 }; | ||
|
@@ -151,12 +152,14 @@ bool SystemView::input(InputConfig* config, Input input) | |
case VERTICAL_WHEEL: | ||
if (config->isMappedLike("up", input)) | ||
{ | ||
listInput(-1); | ||
if (listInput(-1)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In what cases will listInput return false in the wheel - doesn't it loop around? Is the "if" needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. listInput return lastCursorPos != newCursorPos so it made sense to me to handle it like this, to directly tie the scroll sound to the "cursor changed position" event. Maybe I can add an intermediary var to make the code look more explicit. That or maybe put the scroll sound up in the IList, since ImageGridComponent, TextListComponent and SystemView all inherit from the IList. The pb is the IList don't have anything to do with theme, so that would have to be set from the child class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. I suppose you're using the "if" to cater for the case where the list only has one item, so that the sound doesn't play if the same element stays selected. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idk if there is other edge case where this can happen. But I think it's more logic to tie the scroll sound to if the cursor moved or not, than tie it directly to user input. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. |
||
Sound::get(mScrollSound)->play(); | ||
return true; | ||
} | ||
if (config->isMappedLike("down", input)) | ||
{ | ||
listInput(1); | ||
if (listInput(1)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment. |
||
Sound::get(mScrollSound)->play(); | ||
return true; | ||
} | ||
break; | ||
|
@@ -165,19 +168,22 @@ bool SystemView::input(InputConfig* config, Input input) | |
default: | ||
if (config->isMappedLike("left", input)) | ||
{ | ||
listInput(-1); | ||
if (listInput(-1)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same - won't repeat for the others. |
||
Sound::get(mScrollSound)->play(); | ||
return true; | ||
} | ||
if (config->isMappedLike("right", input)) | ||
{ | ||
listInput(1); | ||
if (listInput(1)) | ||
Sound::get(mScrollSound)->play(); | ||
return true; | ||
} | ||
break; | ||
} | ||
|
||
if(config->isMappedTo("a", input)) | ||
{ | ||
Sound::get(mLaunchSound)->play(); | ||
stopScrolling(); | ||
ViewController::get()->goToGameList(getSelected()); | ||
return true; | ||
|
@@ -186,15 +192,18 @@ bool SystemView::input(InputConfig* config, Input input) | |
{ | ||
// get random system | ||
// go to system | ||
setCursor(SystemData::getRandomSystem()); | ||
if (setCursor(SystemData::getRandomSystem())) | ||
Sound::get(mScrollSound)->play(); | ||
return true; | ||
} | ||
}else{ | ||
if(config->isMappedLike("left", input) || | ||
config->isMappedLike("right", input) || | ||
config->isMappedLike("up", input) || | ||
config->isMappedLike("down", input)) | ||
listInput(0); | ||
config->isMappedLike("down", input)) { | ||
if (listInput(0)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we playing the sound on scroll and on stopping the scroll? I believe this is run when the button is released - is this deliberate? |
||
Sound::get(mScrollSound)->play(); | ||
} | ||
if(!UIModeController::getInstance()->isUIModeKid() && config->isMappedTo("select", input) && Settings::getInstance()->getBool("ScreenSaverControls")) | ||
{ | ||
mWindow->startScreenSaver(); | ||
|
@@ -419,6 +428,9 @@ void SystemView::getViewElements(const std::shared_ptr<ThemeData>& theme) | |
if (sysInfoElem) | ||
mSystemInfo.applyTheme(theme, "system", "systemInfo", ThemeFlags::ALL); | ||
|
||
mScrollSound = Sound::getPath(theme, "system", "scroll"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These seem to be set in applyTheme - is there a case where sysInfoElem doesn't exist yet we'll reload these sounds without applying the theme? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry - just re-read: probably it's the other way around - you only want to set these if the new theme is applied, then (meaning, inside the if)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "if" is only there to make sure the ThemeElement is not null, Sound::getPath already do that check and return an empty string if it happen. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that the getPath getter is safe. Still, unless there is a use case for the sound to be updated when the theme is not, this is a case where these should be inside the if. I take it that mScrollSound and mLaunchSound are initialized somewhere else with default values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what you mean. The 2 "if" that we see before just check if the theme element is here and if it is, it is applied. The "systemInfo" is just a sub element of the system theme, just like the sound. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it - nevermind. |
||
mLaunchSound = Sound::getPath(theme, "system", "launch"); | ||
|
||
mViewNeedsReload = false; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,6 +91,7 @@ bool ISimpleGameListView::input(InputConfig* config, Input input) | |
// it's a folder | ||
if(cursor->getChildren().size() > 0) | ||
{ | ||
Sound::getFromTheme(getTheme(), getName(), "launch")->play(); | ||
mCursorStack.push(cursor); | ||
populateList(cursor->getChildrenListToDisplay()); | ||
FileData* cursor = getCursor(); | ||
|
@@ -101,12 +102,12 @@ bool ISimpleGameListView::input(InputConfig* config, Input input) | |
return true; | ||
}else if(config->isMappedTo("b", input)) | ||
{ | ||
Sound::getFromTheme(getTheme(), getName(), "back")->play(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a change in the feature set - can you elaborate? It seems we're now adding the "back" sound when changing from the system view to the carousel. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the game* view to the system* view. |
||
if(mCursorStack.size()) | ||
{ | ||
populateList(mCursorStack.top()->getParent()->getChildren()); | ||
setCursor(mCursorStack.top()); | ||
mCursorStack.pop(); | ||
Sound::getFromTheme(getTheme(), getName(), "back")->play(); | ||
}else{ | ||
onFocusLost(); | ||
SystemData* systemToView = getCursor()->getSystem(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New themes should use global property
sound name="scroll"
, "scrollSound" can still be used for backward compatibility. Maybe "deprecated" is not the right word, if you have a better suggestion I will take it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a fallback, I suppose - if you don't have a global setting defined for the system, it will use this one. I like the way you explain it - it can still be used for backward compatibility.
I don't have a strong opinion, but I'm mindful of there existing hundreds of themes right now, potentially using the previous syntax, and on the flipside, not wanting to risk this being effectively removed at some point.