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

Implement environment property for component screenshots #530

Merged
merged 5 commits into from
Sep 29, 2023

Conversation

ximion
Copy link
Owner

@ximion ximion commented Sep 22, 2023

This adds a new "environment" attribute to the "screenshot" element to specify the environment in which the screenshot was recorded. It is comprised of a ":<style>" string, where "style" could be a specific visual style, like "light" or "dark" or even form factor like "mobile", and "env" is the desktop environment, like "gnome", "plasma" or even "windows".

This change will allow software centers to pick a screenshot visual style most closely aligned with the current environment.

This would resolve a whole bunch of feature requests all at once: #390, #410, #304

Please have a look specifically at the specification wording, and see if you spot any issues!
This code does not yet come with validator extensions to check the environment attribute values, I'll add that before merging this once I am happy with the design and no fundamental objections are raised.

In brief, this patch allows XML like this:

<screenshots>
  <screenshot type="default">
    <caption>The FooBar main window.</caption>
    <image type="source" width="1600" height="900">https://example.com/foobar/screenshot-1.png</image>
  </screenshot>

  <screenshot environment="plasma-mobile">
    <caption>The FooBar main window, but on Plasma Mobile</caption>
    <image type="source" width="1600" height="900">https://example.com/foobar/screenshot-1_plasma-mobile.png</image>
  </screenshot>

  <screenshot environment="gnome:dark">
    <caption>The FooBar main window, on GNOME in dark mode</caption>
    <image type="source" width="1600" height="900">https://example.com/foobar/screenshot-1_gnome_dark.png</image>
</screenshot>

Considerations

Why not one attribute for style and one for environment?

Separating style and environment would mean you could combine them arbitrarily, so suppose "plasma" would define a style "breeze" and "breeze-dark" because it's switching to the theme, those styles would suddenly appear valid-ish for the "gnome" environment as well. By keeping "environment" one identifier string, we can only allow certain combinations, which simplifies validation. It also makes this string a bit more extensible, if we ever want to accommodate for special needs in future.

Why environment and not desktop? It's a DE afterall that provides the UI!

Using the XDG desktop environment strings seems like an attractive choice at first, but the GUI shells have meanwhile evolved where one desktop can provide multiple different environments. E.g. the "KDE" "desktop" might want to provide a plasma and plasma-mobile environment, even though it's the same desktop. Those environments also have different visual styles.

So, using environment as a string here seemed like the better name choice, even though I still slightly dislike it because it clashes with the "environment" component of a platform triplet in AppStream (which would be "gnu"). I don't think there's much room for confusion here though.
An alternative name would be "shell" or "gui", but that has even narrower connotations.

Why is this an attribute of screenshot and not image/video?

Making the environment part of image/video would make the implementation harder, especially since the latter two are also translatable. But most importantly it would mean that if you have a screenshot for one environment you must provide a 1:1 match for the other environments too, which does not always make sense - you may want to show off different stuff in different environments.
Even more important, the screenshot caption might need to be different between environments. Therefore, the environment tag is logically placed as attribute of screenshot, not of any of its media types.

Issues

Screenshot fallbacks

Suppose the metadata author has defined a bunch of screenshots for KDE Plasma, but we're on GNOME. Which screenshot to we pick?
In the current implementation, there always must be one, and only one, default screenshot, and if we could not find a suitable screenshot for the "gnome" environment, we check whatever environment the screenshot that was marked as default had and load all screenshots for that environment.

This also means that if the type="default" screenshot was for plasma and the component has plasma:dark screenshots defined, and we select for gnome screenshots in style dark, we will still end up with the light Plasma screenshots, even though the darker ones would fit better in this case.

Since the "style" is arbitrarily defined, we can't really work around this in a smart way, and I would hate it if every software center started to implement new odd heuristics. This patch introduces a as_component_filter_screenshots function to select a good fit screenshot based on the logic outlined in the specification, but maybe over time we can make this function even smarter.

Potential screenshot duplication

Assume we have a bunch of screenshots for different environments, and also recorded a really nice video demonstrating our app, but we don't want to repeat making the video for every environment.
If we put the video without any environment property into the metadata, nobody will ever see it as the screenshot with the video will be ignored as "not matching the current environment".
So presumably what people will start doing is to duplicate the screenshot entry for every environment. This will be annoying for appstream-compose and appstream-generator, which probably need to find a smarter way of deduplicating the image and video data.
But this will also lead to lots of duplicated tags and translations.

Unfortunately, I don't really see a way around this without breaking the world. The easiest in theory would be to define a new any environment name, which specifies that the screenshot should be visible in all environments, but then we run into an ordering issue: With the addition of environment, screenshot tag order matters now, to determine which one is displayed first for the given environment. With the "all" screenshot, we would have no idea where to place it, and it may even end up being the first screenshot in the list, without being intended for it by the metadata author.

EDIT: I was overthinking this a bit - having any as "display this screenshot for any environment" value actually does make sense and will work with ordering if screenshots are always ordered by their intended appearance, and not placed in blocks ordered by their environment, which is what I thought would happen. So we could (and should?) have any as a possible value.

Overloading of type="default"'s meaning

This one is minor. Before, "default" only meant "this is the primary screenshot, display it first and make it representative for this app". Now it also means "any screenshot with the same environment as this one will be used as fallback". This means we can not have a default screenshot per environment, and the first one in the list for the given environment will be selected as primary screenshot.


Feedback on this stuff is very welcome!
It's one of the few really big things left to do before AppStream 1.0 can be released :-)

@ximion
Copy link
Owner Author

ximion commented Sep 22, 2023

@cassidyjames @pwithnall @JakobDev @sophie-h Have a look at the broad strokes if you have time - the actual code will probably change a bit (and there will be appstreamcli validate support for this, that's currently missing), but I'm specifically interested whether this solves the feature requests and the tradeoffs outlined are worth it.
Also, we could probably greatly improve heuristics by defining styles like light and dark and potentially "friend" DEs, like LXQt+Plasma and GNOME+Elementary for better fallback heuristics (even though I'm not sure if I like that... Could become too "magical" and complex internally at some point...).

@danirabbit
Copy link
Contributor

@ximion I'm not sure it would be a super value add to do something like "friend DEs", but I think we probably want to always prefer desktop screenshots on desktop or always mobile screenshots on mobile if they're available. It's not super clear to me how that would work in this draft. Would it make sense to have form factor be separated from the environment so you'd have like pantheon:mobile:dark for example?

@ximion
Copy link
Owner Author

ximion commented Sep 23, 2023

AppStream does not know a form factor still, so it can't make that decision. A software center could request pantheon environment screenshots in style mobile:dark though, in which case if there's an exact match, those screenshots will be displayed, otherwise we will fall back to displaying any pantheon screenshot, and if that fails and there are none, we will display the component's default screenshots, at least with the current logic.

We could also give style preference over environment. In that case, if no pantheon:mobile:dark exists, we pick the next best mobile:dark screenshot at random, and if that doesn't exist, we pick the default. Could make some sense...

Or, of course, the environment would be pantheon-mobile and the style would be dark...
The second option sounds attractive, but only until people start to define gnome:phablet:dark and plasma-mobile:light and plasma:breeze:mobile:dark or something similar to further split the environment into even more styles and colors and form factors.

@cassidyjames
Copy link
Contributor

cassidyjames commented Sep 25, 2023

Question: should environments just prioritize matching screenshots, then still display other ones later in the carousel/list?

This is how the Google Play Store handles screenshots for different display sizes/device types, for example, and it's actually handy: if I'm browsing on my phone and taking a good look at screenshots, swiping over and seeing the tablet and watch screenshots actually shows me how it will look on my other devices as well, which is useful. I may pick an app based on how it adapts to other form factors rather than just how it looks on my current form factor.

Similarly, if I'm browsing in light style right now because it's daytime, I still may be interested to see how the app adapts to a dark style, so I'd like those in the carousel—maybe just not the ones that are shown first.

In that case, it actually could simplify the implementation, imho, because it wouldn't imply completely hiding mismatched screenshots; it would just be about which ones get featured first. But it would solve the duplication issue you mentioned.

Edit: I guess maybe that could be up to the client, but I'd like the use case to be considered in the spec, at least.

@ximion
Copy link
Owner Author

ximion commented Sep 25, 2023

That's a good point! And that logic makes a ton of sense to me :-)

I could imagine an API like

void as_component_sort_screenshots (AsComponent *cpt,
					     const gchar *environment,
					     const gchar *style,
					     gboolean prioritize_style);

which would just bring the screenshots in the preferred order for the respective software center (environment first, then style, unless prioritize_style is set to TRUE), and then you would just fetch them all via get_components_all, with the matching ones at the front, followed by the rest in whatever order they were set by the metadata author.

That would simplify the implementation quite a bit.
The KDE people around @hsitter want to use this to include Windows screenshots too though, which might clash a little... But then again, I wouldn't see a reason to not show them (as last entry...) on Linux too - it's the same app, after all.

@cassidyjames
Copy link
Contributor

@ximion yeah, if an app developer is shipping a cross-platform app and supports other platforms, I think it does make sense to show those for the same reason as the other form factors. Maybe clients can decide whether to show them, or to badge them as being for another platform or something, to reduce potential confusion? I'm not sure.

@danirabbit
Copy link
Contributor

Yeah, I think this might be a case where letting the client handle which screenshots to show and how might be best. The iOS App store for example has separate carousels for each form factor and collapses all but the platform you're currently on by default, so that you can see for example the screenshots for Watch or TV or Desktop while on Mobile.

Here's an example from their web store with an app that has versions for all the different form factors: https://apps.apple.com/us/app/calm/id571800810

@ximion
Copy link
Owner Author

ximion commented Sep 25, 2023

I like this a lot, it makes logical sense to me and there's less to do in libappstream :-)
The one thing though that I think this will require us to provide, is some mapping of "style" and "environment" to a translatable human-readable string, so in case a software center wants to display screenshots like apple does, they can display "KDE Plasma (Dark)" in the user's native language, rather than "plasma:dark".

I also do think the as_component_sort_screenshots method would be helpful for software centers that want to use it :-)
If I have some time, I'll implement this tomorrow and change this PR to match.
This is the last remaining planned major feature (because it's technically a breaking change...) before the 1.0 release can happen :-)

This adds a new "environment" attribute to the "screenshot" element to
specify the environment in which the screenshot was recorded.
It is comprised of a "<env>:<style>" string, where "style" could be a
specific visual style, like "light" or "dark" or even form factor like
"mobile", and "env" is the desktop environment, like "gnome", "plasma"
or even "windows".

Resolves: #390, #410, #304
@ximion ximion force-pushed the wip/mak/screenshot-envs branch 2 times, most recently from 205eec6 to a4a0e67 Compare September 27, 2023 03:39
@ximion ximion force-pushed the wip/mak/screenshot-envs branch from a4a0e67 to 4fd4ca5 Compare September 27, 2023 20:09
@ximion ximion changed the title WIP: Implement environment property for component screenshots Implement environment property for component screenshots Sep 27, 2023
@ximion
Copy link
Owner Author

ximion commented Sep 27, 2023

I've implemented the discussed ordering changes (so, no more filtering), added validation for the new environment property and added new API to libappstream to fetch a translated name for the respective allowed environment:style combination.

From my perspective, this is good to go unless someone has more remarks?

Of course, recognized environments still need to be added to data/desktop-style-ids.txt, as that file is currently a bit empty ;-)

@ximion ximion merged commit 13d888f into main Sep 29, 2023
@ximion ximion deleted the wip/mak/screenshot-envs branch September 29, 2023 22:25
@ximion
Copy link
Owner Author

ximion commented Sep 29, 2023

In order for this to work properly, all allowed env/style combinations will need to be registered in https://github.com/ximion/appstream/blob/main/data/desktop-style-ids.txt now, together with their translation :-)

@ximion ximion restored the wip/mak/screenshot-envs branch December 16, 2023 17:44
@ximion ximion deleted the wip/mak/screenshot-envs branch December 16, 2023 17:44
gnomesysadmins pushed a commit to GNOME/gnome-sudoku that referenced this pull request Jun 4, 2024
See ximion/appstream#530
We can't have two defaults, and width/height are unnecessary
gnomesysadmins pushed a commit to GNOME/gnome-sudoku that referenced this pull request Jun 6, 2024
See ximion/appstream#530
We can't have two defaults, and width/height are unnecessary
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants