Testers Wanted: GameListView Theming Refactoring

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.

A12C4

Any update on this ? I haven't commented here yet but I'm following this since the beggining and would love to see it happen.

Zigurana

Sadly, this has been put on hold for the time being.

A12C4

@Zigurana If you want, maybe I can take a look when my job on the grid view will be "finished". I start to understand well this part of the code base.