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

Add Plasma Mobile to desktop-style-ids.txt #622

Merged
merged 2 commits into from
Apr 7, 2024
Merged

Conversation

1Maxnet1
Copy link
Contributor

As discussed in #611 this PR adds Plasma Mobile to the list of desktop style ids.

Let me know if anything is missing

@tintou
Copy link
Contributor

tintou commented Mar 27, 2024

Wouldn't a "chassis" property make more sense? (as defined in https://www.freedesktop.org/software/systemd/man/latest/org.freedesktop.hostname1.html#Semantics )
This would be treated as a hint for the screenshot

@1Maxnet1
Copy link
Contributor Author

Wouldn't a "chassis" property make more sense? (as defined in https://www.freedesktop.org/software/systemd/man/latest/org.freedesktop.hostname1.html#Semantics )

I agree introducing a chassis type, makes sense, but would not eliminate the need for plasma-mobile to be added. If you have chassis type desktop/mobile and plasma/plasma-mobile, you could still have Plasma on a desktop/laptop, Plasma on a tablet or convertible (which could prefer the mobile chassis for screenshots, but runs the Desktop Version of plasma), a plasma-mobile installation on a smartphone and plasma-mobile on a docked smartphone (which again would potentially prefer the desktop screenshots as long as you are in a "desktop/docked" mode. To identify all those four cases, one would need both attributes. I admit however that this is currently an edge case. Nevertheless plasma-mobile and plasma are two different desktop environments so ignoring the mentioned edge case it might make sense to add it anywhere.

@tintou
Copy link
Contributor

tintou commented Mar 27, 2024

Ah so if it's a completely different environment it makes sense, I'm surprised to not see them in https://specifications.freedesktop.org/menu-spec/latest/apb.html though for instance

@razzeee
Copy link
Contributor

razzeee commented Mar 27, 2024

I guess we should add phosh and gnome-mobile too then

@1Maxnet1
Copy link
Contributor Author

I guess we should add phosh and gnome-mobile too then

I agree for phosh, however gnome-mobile is as far as I know "only" a WIP branch of GNOME which eventually aims to be completely upstreamed. (See the postmarketOS Wiki for details https://wiki.postmarketos.org/wiki/GNOME) so I would not add it to the list. Phosh and Plasma Mobile make sense to me, because they plan to stay separate environments from GNOME/KDE Plasma, while sharing certain components. Would you add Phosh to this MR or should I create another for it?

@razzeee
Copy link
Contributor

razzeee commented Mar 28, 2024

Still, apps targeting gnome-mobile might have different screenshots (probably portait mode) and it would be useful to know that via metadata. That's why I thought it might be smart to add gnome-mobile - that said, you're not wrong.

I think adding phosh here would be fine, but @ximion 's call

@1Maxnet1
Copy link
Contributor Author

Still, apps targeting gnome-mobile might have different screenshots (probably portait mode) and it would be useful to know that via metadata. That's why I thought it might be smart to add gnome-mobile - that said, you're not wrong.

Maybe a fitting alternative here would be to add gnome:mobile instead as a style. See also the note here: #611 (comment)

@razzeee
Copy link
Contributor

razzeee commented Mar 28, 2024

Still, apps targeting gnome-mobile might have different screenshots (probably portait mode) and it would be useful to know that via metadata. That's why I thought it might be smart to add gnome-mobile - that said, you're not wrong.

Maybe a fitting alternative here would be to add gnome:mobile instead as a style. See also the note here: #611 (comment)

I think that would be bad as it probably breaks the possibility to have gnome-mobile:dark then

Copy link
Contributor

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I agree with the others we should at least add phosh and probably also gnome mobile too, to that list :)

data/desktop-style-ids.txt Outdated Show resolved Hide resolved
src/as-desktop-env-data.h Outdated Show resolved Hide resolved
@ximion
Copy link
Owner

ximion commented Mar 31, 2024

Also I agree with the others we should at least add phosh and probably also gnome mobile too, to that list :)

I do no longer just add stuff because it might be used in future, but only when it is specifically requested by the respective project owners / maintainers / contributors.
There were some instances in the past where we preemptively added stuff to AppStream that the respective projects had different ideas about, that's why I am fairly conservative (we can always add stuff, but once IDs are in, removing them will be very hard or even impossible).

@ximion
Copy link
Owner

ximion commented Mar 31, 2024

Also, that C header is auto-generated, it doesn't need to be changed specifically as it will be regenerated at the next opportunity (but changing it also doesn't hurt in this case, so with respect to that, this patch is fine).

@razzeee
Copy link
Contributor

razzeee commented Mar 31, 2024

Sorry for the noise, please don't add phosh and gnome mobile then

Edit: To be clear, upon thinking this over - I don't think gnome mobile should be it's own platform. We probably don't want any screenshots, that actually include the shell or phosh shell.

Co-authored-by: Carl Schwan <[email protected]>
@1Maxnet1
Copy link
Contributor Author

1Maxnet1 commented Apr 3, 2024

Thanks for all the comments. I updated the PR with @CarlSchwan's suggestions and as mentioned by @ximion I won't add Phosh or GNOME Mobile for now.

@ximion
Copy link
Owner

ximion commented Apr 7, 2024

Nice! Let's merge it then! :-)

@ximion ximion merged commit b7f1fc9 into ximion:main Apr 7, 2024
10 checks passed
@1Maxnet1 1Maxnet1 deleted the patch-1 branch April 7, 2024 19:02
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.

5 participants