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

Check if DisplayServer supports icons before attempting setting it #89181

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Mar 5, 2024

Silences this error when editing or running any project with Wayland:

WARNING: Icon not supported by this display server.
     at: set_icon (servers/display_server.cpp:591)

Inspired by #89099 and #89178.

@bruvzg We should likely apply the same treatment to all feature-specific APIs so we only call them internally when they're supported. I applied it to set_native_icon here while I was at it.

@akien-mga akien-mga added this to the 4.3 milestone Mar 5, 2024
@akien-mga akien-mga requested a review from a team March 5, 2024 13:44
@akien-mga akien-mga requested review from a team as code owners March 5, 2024 13:44
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

FEATURE_NATIVE_ICON checks are pretty redundant, all are in platform specific #ifdefs, but I guess it's OK for readability.

Other platform specific features should already have feature checks (at least menu and macOS expand to title bar, not sure if there are other). I guess it's it was missing for FEATURE_ICON since it was supported by all platforms before Wayland addition.

Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

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

Looks perfect!

@Riteo
Copy link
Contributor

Riteo commented Mar 5, 2024

FEATURE_NATIVE_ICON checks are pretty redundant, all are in platform specific #ifdefs, but I guess it's OK for readability.

Yeah we should kinda make the whole warning logic more consistent. Perhaps emit a warning for all optional features, giving the caller the responsibility of calling it and gracefully fallbacking (if possible) when needed.

@akien-mga akien-mga merged commit 39f9060 into godotengine:master Mar 5, 2024
16 checks passed
@akien-mga akien-mga deleted the displayserver-check-support-before-setting-icon branch March 5, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants