Testers Wanted: GameListView Theming Refactoring

Zigurana

Hi all!
It's been a while, but I've been busy!
This change is less of a novel feature and more of an infrastructure redesign. For some extensive background & discussion, see here. Its quite a read, but the end-goal can be condensed to this:

In a perfect world, a theming system will allow themers to create new views by adding a new view node to their xml files, and adding the elements they would like to see there.

Nothing more, nothing less :-).

Intro

In the current situation, ES has a number of theme elements that are hardcoded to be present in the Detailed and Video GameListViews (GLVs). That means: in order for the theme to work correctly, you need to place the publisher label text (mdl_publisher) somewhere, even if you don't even want to see it in the first place. So themers resort to placing it at location [1,1], to shift if off the screen. That's just wrong, and it bothered me ;-).
Whats more important, the creation of new GLV types is very cumbersome. The new Video GLV needed extensive code-work, much of it duplication, to work as it should. Also, there is currently little or no way to create separate 3:4 and 16:8 views in the same theme. Now, I believe I have created a solution for that: the universal GameListView class.

Leaving the implementation details for the github discussion, what it boils down to is this:

There no longer is a dedicated Basic, Detailed or Video GameListView, or at least not in the source code.
Instead, all available GLVs are defined by the theme.
As a result, current themes are still fully functional (or should be, let me know if not!).
There can be as many GLVs as you like, named as you like (except for 'system', and 'grid', those names are reserved).
The only requirement for GLVs is to have a gamelist element, all other elements are optional.

What's the use?

With this in place Themers can create many views that use the same set of assets/components. So you could consider creating views for varying screen sizes/ratios, similar to web development designing for desktop or mobile. Also, you could create different color palettes, and have users select them from the ES UI. There are probably more applications, let us know if you think of any!

Preview

So, with this added to the carbon.xml theme:

<view name="test">
![0_1497992189527_ThemeRefact.png](/forum/assets/uploads/files/1497992195868-themerefact-resized.png) 	<image name="md_image">
		<pos>0.763 0.400</pos>
		<maxSize>0.453 0.436</maxSize>
		<origin>0.5 0.5</origin>
	</image>
	<textlist name="gamelist">
		<selectorColor>1c1c1c</selectorColor>
		<selectedColor>8b0000</selectedColor>
		<primaryColor>969696</primaryColor>
		<secondaryColor>7a6161</secondaryColor>
		<fontPath>./art/Cabin-Bold.ttf</fontPath>
		<forceUppercase>1</forceUppercase>
		<pos>0.021 0.168</pos>
		<size>0.489 0.754</size>
		<alignment>left</alignment>
		<horizontalMargin>0.00</horizontalMargin>
		<fontSize>0.03</fontSize>
		<scrollSound>./art/scroll.wav</scrollSound>
	</textlist>
	<text name="md_title">
		<color>969696</color>
		<forceUppercase>0</forceUppercase>
		<fontPath>./art/Cabin-Bold.ttf</fontPath>
		<fontSize>0.023</fontSize>
		<pos>0.53 0.63</pos>
		<size>0 0.1</size>
	</text>
</view>

I get:

TODO

There is of course quite some stuff left to do, and some of that I will pick up soon, but other things you can help with as well!
For ease of testing, the old implementations of the Basic, Detailed and Video views are still there, as well as the 'auto' option. This way you can easily compare behaviour/performance going from the old to the new GLVs.

Testing

To help hunting and squashing bugs, install this ES branch using @meleu's great tool: URL: https://github.com/zigurana / branch: ThemingRefactoring

Take it for a Ride, try out your favorite Themes, old new, any! How does it behave, how is performance? Any observations?
What happens when there is only a single theme installed, and it is invalid (eg. not xml compliant)? I have not tried this, no idea what happens.
What are the limits? What do you miss?

Coding

My next stop will be the first of these items, as I need it for the kid-modes / favorites to work as intended.
The other items are more a brainstorm than anything else at this point.

Bug: check availability of selected view type when calling onThemeChanged(), fallback to available view.
Extension: Introduce new ToggleImageComponent item, that toggles visibility via the SetValue() call, and populates the texture via the OnThemeChanged() method.
Bug: Fix showsnapshot functionality for VideoComponents.
Consider: would it be useful to set a default (or forces) filter type together with the view? So say: create a sonic specific view, to set sonic specific assets, and also show only sonic specific games? Probably not, but I am curious what you guys would think of such a workflow.
Fully implement CarouselComponent class (stub present), so we can start creating either variations of SystemListViews, or even combined System/GamelistViews (ghasp!)
If all goes well, we might want to consider phasing out the use of "supported feature" modifiers.

So to conclude, please give this a try, and leave some feedback, thanks for your help!

Bonus: There is a new metadata theme-element called 'md_title', which will be populated by the 'name' metadata value.

lilbud

@Zigurana So, we don't have to define everything for a theme and every view?

Like, if a theme doesn't use the publisher tag, we don't have to define it?

Hex

@Zigurana Regarding the missing theme part, a mock theme which displays very basic message that themes are not installed and where a user can get them would be excellent. Additionally if themes are missing and the machine is connected to internet then the files can be downloaded by ES automatically for a default theme. ??

TMNTturtlguy

@Zigurana any chance this could incorporate the addition of a few more md categories like md_boxart and md_wheelart as well as let the marquee work in any view no tied to video? Will videos be be able to play in the system selection screen or are these changes only for the list view? This is exciting work! Thanks for all the hard work.

jdrassa

@Zigurana first off let me say great job for the most part it is working very well.

First some issues I have found.

Try it out with the nes-mini theme. I tested it with a bunch of different themes and this was the only one that seemed to have obvious issues.
If you set gamelist style to "video" in the menu, then switch to a theme that does not support "video". Initially, no theme will be loaded (since there isn't one). It would be nice if it would default to a GLV that is defined in theme.
In addition, to the above behavior, if you go back to the UI Settings menu, the GameList View Style option has a render error since the current theme does not have a view defined that matches the current settings value. If you then try to switch themes without first switching to a valid view, a core dump occurs.
The showSnapshot options for video do not work. This appears to be due to the fact that the image is never being set on the VideoComponent.

Now onto some notes/observations.

I have some reservations about themes randomly creating new GLV types. I like the idea, but I worry that without some standardization, switching themes will become a 2 step process where we always have to first pick the theme and then pick the style.
It looks like the way metadata values are binded to elements is based on the element name. While this is necessary for compatibility with existing themes, it would be nice if their were a way to set it as a property on the element. This would allow for binding the same metadata value to multiple elements (i.e. a ratings image and ratings text). It would also simplify adding additional image types.

Zigurana

@lilbud correct!

Zigurana

@TMNTturtlguy said in Testers Wanted: GameListView Theming Refactoring:

@Zigurana any chance this could incorporate the addition of a few more md categories like md_boxart and md_wheelart as well as let the marquee work in any view no tied to video?

Please see https://retropie.org.uk/forum/topic/10654/es-add-some-news-artworks-to-skin-like-in-a-vg-museum/8 for more on that discussion.

Will videos be be able to play in the system selection screen or are these changes only for the list view?

For now at least, this only concerns the Gamelistviews.

Zigurana

@jdrassa : thanks for testing this out!
I've added your issues to the todo list. (have not yet looked at the nes-mini theme, are there additional issues for that theme?)

Now onto some notes/observations.

I have some reservations about themes randomly creating new GLV types. I like the idea, but I worry that without some standardization, switching themes will become a 2 step process where we always have to first pick the theme and then pick the style.

That could be, but is that really such a nuisance?

It looks like the way metadata values are binded to elements is based on the element name. While this is necessary for compatibility with existing themes, it would be nice if their were a way to set it as a property on the element. This would allow for binding the same metadata value to multiple elements (i.e. a ratings image and ratings text). It would also simplify adding additional image types.

In order to maintain backwards compatibility, this approach would then need to live alongside the current mechanism. I'm not necessarily opposing the idea, but it will complicate the design again.
Another issue I'm not sure of is if we would want to have the theme dictate the metadata types that are (supposedly) available. One could argue that this a responsibility of the end-user (not the Themer).

That would point at an additional resource file describing the binding between metada and theme elements. But maybe that only complicates things too much for 'regular' users.

I'm open to suggestions, but it sure would be nice to be more flexible with the metada types available.

Zigurana

I've pushed an update for this.
Changes:

If you switch themes, there is now a check (and fallback) to a view that is actually available, because different themes will likely support different views.
Image components can now be toggled (for visibility) using the SetValue() method, for use with metadata-dependent icons (favorites, hidden, Kids).
Some small improvements (code simplifications, helper methods, etc).

Zigurana

This branch has now been rebased to ES 2.4.1rp.

Note: The changes that I propose here assume that the theme has the same views defined for all systems, or it will not be able to populate the available GLV list.
If you use a Theme that does not support a default theme, nor a specific theme for the favorites / recent / all auto-systems, only the legacy types are supported (which we are trying to get rid off). The recently updated Carbon theme has no issue.

Currently known bugs / TODO list:

~~Bug: check availability of selected view type when calling onThemeChanged(), fallback to available view.~~
~~Extension: Introduce new ToggleImageComponent item, that toggles visibility via the SetValue() call, and populates the texture via the OnThemeChanged() method.~~
Bug: Fix showsnapshot functionality for VideoComponents.
To Consider: would it be useful to set a default (or forces) filter type together with the view? So say: create a sonic specific view, to set sonic specific assets, and also show only sonic specific games? Probably not, but I am curious what you guys would think of such a workflow.
Fully implement CarouselComponent class (stub present), so we can start creating either variations of SystemListViews, or even combined System/GamelistViews (ghasp!)
Remove Legacy gamelistviews code.
Phase out the use of "supported feature" modifiers.

pjft

@zigurana well done!

Can you elaborate on the "to consider" bullet point, just so I fully understand what you have in mind in terms of usage and use case?

Best.

TMNTturtlguy

@Zigurana where are you at with backward compatibility? The more I keep reading on this, I still see your descriptions stating things that either will be required to be updated to work, or things like features tags that will be phased out. I am concerned that there are a ton of themes out there, and not all of them are actively managed. Then there are some of our themes that support a huge number of systems. My ComicBook theme has 4 versions (due to current issue with themeing that you may be resolving), each of my versions has 116 systems and each systems theme.xml has unique properties so a lot of changes cannot be handled by a find replace all tool.

Anyways to keep on topic, in my opinion it is important we support legacy themes, otherwise a lot of content may be lost. Thanks

Zigurana

@tmntturtlguy, thanks for sharing your concerns, let me try to put them to rest.

I would never even consider breaking support for our archive of existing themes, as I consider that to be one of our greatest assets.
The proposed changes are not meant to break any existing themes. If that's the case, I would consider that a bug that needs fixing before we commit it to the master branch.
The legacy GameListView types ('basic', 'detailed', 'video') will still be listed. The difference is that they will be generated by a single generalized method, as opposed to three hard-coded ones.
I've kept these hardcoded ones in place for ease of testing, but they should be essentially the same thing.

Rest assured that this activity is meant to reduce the amount of work for themers, not increase it. For instance, it will no longer be necessary to create separate 4:3 and 19:9 themes, while 90% of their assets are the same. Now, you could just create another view within the same theme, and reuse these assets. That saves you from having to maintain multiple copies of a very similar theme.
Similarly, you can supply presets for color variations, without the end-user having to go and edit the xml themselves.

In terms of functional improvements, these changes allow you a much more fine-grained control over what Metadata elements you want to include to the different views or not, rather than the current all (detailed) or nothing (basic).

Regarding phasing out the 'supported-feature' tag (and the 'extra' tag while we're at it), these are workarounds to have ES play nice with themes that contain more elements than expected. If I get my way, ES will just ignore the tags it does not recognize (instead of failing to parse the theme altogether). Thus, we do not need these workarounds any longer.

In that sense I am not planning to retain backwards compatibility for people using older versions of ES with newer themes. I think those folks should just update their ES.

If there is an issue with newer versions of ES which prevent them from updating, then that issue should be addressed separately. But we should not bend over backwards to accommodate older ESes.

I hope that this clears things up for you, thank you for bringing this up.

Zigurana

@pjft that is really just a brainstorm item, where I tried to tie in this new possibility of many views with the filter functionality. Basically letting the themer set a filter for the user.

Then we could show all games of a certain genre, with a view relevant to that genre.

Would be even better if we would have free text filtering on game-titles...

pjft

@zigurana Got it.

Sounds interesting, though then those themes would start from a collection of "all games" and filter them out, is that it?

I can certainly see that for non-free text filters. As for free-text filters, it starts being a can of worms - "Mega Man", "Megaman", etc. - but I think there might be something there to explore. On the other hand, with custom collections, we might solve that in a way that doesn't necessarily explicitly tie the view to the data. But we'll see!

We can certainly add free-text filtering on game titles at a later stage - I have just avoided doing anything that requires a physical keyboard for the user to use so far :)

Thanks!

Zigurana

@pjft quite, very much in the brainstorm domain for sure!

Zigurana

@jdrassa said in Testers Wanted: GameListView Theming Refactoring:

The showSnapshot options for video do not work. This appears to be due to the fact that the image is never being set on the VideoComponent.

Looked at this yesterday and tonight, and the issue is not straightforward to solve.
The way I understand it: Rather than setting a single value, the VideoComponent needs to use two game-dependent values: the video-path and thumbnail path. This is also the reason why the setValue() method is not being used in the current VideoGameListView, but instead two calls are made: mVideo->setVideo(video_path), and mVideo->setImage(thumbnail_path);

Currently, the VideoComponent class is the only one with this behavior, the other GuiComponent subclasses can suffice with a single call to SetValue() to update the content based on the currently selected game.

Now to solve this, there are a couple of solutions:

make an exception for the VideoComponent while populating its values (hacky)
change all the implementations of SetValue to take the full metadata, rather than just a single string (muchos effort).
leave as is, and create an ImageComponent as well as a VideoComponent when we encounter md_video in the theme. (simplify videocomponent + thumbnail member becomes obsolete?)

Personally I don't use videoviews, and I am unsure of the difference between the games image and its thumbnail (I guess the thumbnail is a still from the movie?). I'd like some feedback on this, and if people have other suggestions.

pjft

I like the simplicity and option 3 is probably the best - even if in the worst case themes would need to explicitly add an image element under the video.

Unless we would make the video component contain an image component as well? Would that be an elegant solution?

Or what else could we think of?

Zigurana

@pjft said in Testers Wanted: GameListView Theming Refactoring:

Unless we would make the video component contain an image component as well? Would that be an elegant solution?

That is the current design, a single component, where we need to somehow set two values with a single SetValue() call.
I'll think about it some more. See if I can make it work.

pjft

@zigurana but is the problem that we need to set to values with a single call, or is it more that each new object you're defining is meant to have a single "value"?

I was interpreting it to be the latter.

If that's not the case, then we could think of options to make it happen in an elegant manner.