Add grid view scroll sound and adjust sound theming #669

benjdero · 2020-07-01T19:54:49Z

Current behavior in normal text. Changes in bold. Suggestions in italic.

Game list views
- The Basic/Detailed/Video views use TextListComponent's own property scrollSound. It is not removed. If both are defined, the global scroll sound is used. Add scroll sound to the Grid view. Maybe games views could use the SYSTEM scroll sound when using quick system change
- Play the launch sound when launching a game. Now it also do when entering a folder. Maybe entering a folder should use the scroll sound instead.
- Play the back sound when exiting a folder. Now it also do when going back to system view
System view
- Add scroll sound to the system view
- Add launch sound when a system is chosen

Syntax:

<view name="basic, detailed, video, grid">
	<sound name="scroll">
		<path>./game_scroll.wav</path>
	</sound>
	<sound name="launch">
		<path>./game_launch.wav</path>
	</sound>
	<sound name="back">
		<path>./game_back.wav</path>
	</sound>
</view>
<view name="system">
	<sound name="scroll">
		<path>./system_scroll.wav</path>
	</sound>
	<sound name="launch">
		<path>./system_launch.wav</path>
	</sound>
</view>

You can test it by using my modified version of @lilbud 's material theme that you can find here

It should fix #485 and #155 and the numerous requests people did on the forum to add scroll sounds to the grid view.

Tests and feedbacks from users and theme designers would be highly appreciated.

- Add scroll sound to the grid view using new syntax - textlist property "scrollSound" is kept for backward compatibility but overrode by new syntax - Play sounds "back" and "launch" in a more consistent way

pjft · 2020-07-02T06:52:46Z

THEMES.md

@@ -713,7 +740,7 @@ Can be created as an extra.
 * `fontPath` - type: PATH.
 * `fontSize` - type: FLOAT.
 * `scrollSound` - type: PATH.
-	- Sound that is played when the list is scrolled.
+	- **Deprecated**: Use global property `sound name="scroll"` instead. Sound that is played when the list is scrolled.


I read the code. This still works, which is a good choice, but I imagine that the majority of themes use this, so I'd rather not mark this as deprecated but rather as a fallback option (which seems to be what the code uses it for).

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.

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.

pjft · 2020-07-02T06:53:32Z

es-app/src/views/SystemView.cpp

@@ -151,12 +152,14 @@ bool SystemView::input(InputConfig* config, Input input)
 		case VERTICAL_WHEEL:
 			if (config->isMappedLike("up", input))
 			{
-				listInput(-1);
+				if (listInput(-1))


In what cases will listInput return false in the wheel - doesn't it loop around? Is the "if" needed?

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.

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.

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.

pjft · 2020-07-02T06:53:40Z

es-app/src/views/SystemView.cpp

 				return true;
 			}
 			if (config->isMappedLike("down", input))
 			{
-				listInput(1);
+				if (listInput(1))


Same comment.

pjft · 2020-07-02T06:54:05Z

es-app/src/views/SystemView.cpp

@@ -165,19 +168,22 @@ bool SystemView::input(InputConfig* config, Input input)
 		default:
 			if (config->isMappedLike("left", input))
 			{
-				listInput(-1);
+				if (listInput(-1))


Same - won't repeat for the others.

pjft · 2020-07-02T06:56:21Z

es-app/src/views/SystemView.cpp

@@ -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");


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?

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)?

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.

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.

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.

Got it - nevermind.

pjft · 2020-07-02T06:59:25Z

es-app/src/views/SystemView.cpp

-			config->isMappedLike("down", input))
-			listInput(0);
+			config->isMappedLike("down", input)) {
+			if (listInput(0))


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?

pjft · 2020-07-02T07:00:22Z

Thanks for this.

I read the code and added a few remarks, as I am not especially familiar with this part of the code so the side effects aren't clear.

benjdero · 2020-07-02T11:55:03Z

@pjft Thanks for the input. I think this also need more testing to get feedback from designers and users, there is a few points that I'm not sure about, for example:

Currently there is no sound played when switching system using the quick system switch, maybe this could play the scroll sound, or the scroll sound from the system view but then I would have to do a "small hack" to get the scroll sound of system view inside the game list view.
Using the "launch" and "back" sounds for going in and out of a folder seems a bit weird, especially with the "launch" sound I used in my test theme, maybe this should use the "scroll" sound as it is the same kind of navigation event.
...

cmitu · 2020-07-02T11:58:17Z

I agree with @pjft about keeping the existing scrollSound element - backwards compatibility matters, especially for themes which will are no longer updated.

Personally, I don't see the need for the back sound, either from exiting a system or a folder - and this might pose problems with fast system switching (which, as you remarked, is not implemented). Sounds for 'launch' and 'folder' enter should be separated - they're not the same function. Maybe use 'scroll' for folder entering/exiting ?

benjdero · 2020-07-02T12:08:07Z

@cmitu Maybe there is a little misunderstanding here:

scrollSound is not meant to be removed, if there is a "global scroll sound" it is used first, and if there isn't then the scrollSound is used. Previous themes will still work. New Themes can choose to or not to get backward compatible with previous/other EmulationStation versions by adding the scrollSound on top of the "global scroll sound".
The back sound is already used to exit a folder, I just changed it so it also play when you exit a system.

Beside that, I agree with you on fast system switch and scroll sound for folder entering (and maybe exiting but that would change behavior for older themes).

pjft · 2020-07-02T15:17:08Z

es-app/src/views/gamelist/ISimpleGameListView.cpp

@@ -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();


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.

From the game* view to the system* view.

benjdero · 2020-07-02T17:50:42Z

@pjft @cmitu I rewrote my first post. Hope it's more clear like this.

pjft · 2020-07-02T17:54:58Z

Thanks.

For the suggestions, I agree that entering a folder should use the scroll sound instead. Undecided about the quick system select, to be honest, as I don't have a strong opinion either way.

pjft · 2020-07-03T22:20:52Z

From my end, I have no more comments from reading the code.

Given that the Theming part of the code is an area that @jrassa would be more familiar with, I'd definitely appreciate his or @tomaz82 's additional remarks.

tomaz82 · 2020-07-04T09:00:55Z

This is an area of ES that I have not touched much, so I can't really give any input on this.

jrassa · 2020-07-04T12:41:31Z

I'll take a closer look and test soon, but it looks good to me.

Add grid view scroll sound and adjust sound theming

46b4dd3

- Add scroll sound to the grid view using new syntax - textlist property "scrollSound" is kept for backward compatibility but overrode by new syntax - Play sounds "back" and "launch" in a more consistent way

pjft reviewed Jul 2, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add grid view scroll sound and adjust sound theming #669

Add grid view scroll sound and adjust sound theming #669

benjdero commented Jul 1, 2020 •

edited

pjft Jul 2, 2020

benjdero Jul 2, 2020

pjft Jul 2, 2020

pjft Jul 2, 2020

benjdero Jul 2, 2020

pjft Jul 2, 2020

benjdero Jul 2, 2020

pjft Jul 3, 2020

pjft Jul 2, 2020

pjft Jul 2, 2020

pjft Jul 2, 2020

pjft Jul 2, 2020

benjdero Jul 2, 2020

pjft Jul 2, 2020

benjdero Jul 2, 2020

pjft Jul 3, 2020

pjft Jul 2, 2020

pjft commented Jul 2, 2020

benjdero commented Jul 2, 2020

cmitu commented Jul 2, 2020

benjdero commented Jul 2, 2020

pjft Jul 2, 2020

benjdero Jul 2, 2020

benjdero commented Jul 2, 2020

pjft commented Jul 2, 2020

pjft commented Jul 3, 2020

tomaz82 commented Jul 4, 2020

jrassa commented Jul 4, 2020

Add grid view scroll sound and adjust sound theming #669

Are you sure you want to change the base?

Add grid view scroll sound and adjust sound theming #669

Conversation

benjdero commented Jul 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjft commented Jul 2, 2020

benjdero commented Jul 2, 2020

cmitu commented Jul 2, 2020

benjdero commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjdero commented Jul 2, 2020

pjft commented Jul 2, 2020

pjft commented Jul 3, 2020

tomaz82 commented Jul 4, 2020

jrassa commented Jul 4, 2020

benjdero commented Jul 1, 2020 •

edited