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

Do not use color properties directly #1245

Merged
merged 1 commit into from
Sep 10, 2021
Merged

Do not use color properties directly #1245

merged 1 commit into from
Sep 10, 2021

Conversation

tintou
Copy link
Member

@tintou tintou commented Sep 6, 2021

They are being copied when retrieving as they are simple structs.
Closes #1243

Copy link
Member

@JoseExposito JoseExposito left a comment

Choose a reason for hiding this comment

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

Is the same change required in Background.load_pattern ()?

By the way, tested with latest Mutter 👍

@davidmhewitt
Copy link
Member

davidmhewitt commented Sep 6, 2021

To be honest, this binding felt a lot better when Clutter.Color.from_string just returned a Clutter.Color, even if it did hide the boolean (that we don't use anyway). It felt like a "constructor", like all the other from_x methods that exist across GLib libraries.

What I don't understand is how these bindings are being generated because of this:

color_from_* skip

If those symbols are skipped and I can't find anywhere they're manually re-added, why are they in the bindings at all?

Edit: Additionally, this is still bound in the old way in Clutter vapi that's not part of the Mutter tree:
https://valadoc.org/clutter-1.0/Clutter.Color.Color.from_string.html

And, in GJS they have it bound so that from_string returns a Clutter.Color (as well as the boolean, which we can't do in Vala):
https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/main/js/ui/background.js#L342

But this just me saying I'm in favour keeping it as it was before the bindings change, just so we can keep it as a one liner in Gala.

@tintou
Copy link
Member Author

tintou commented Sep 8, 2021

@davidmhewitt I've changed the .metadata and -custom.vapi to reflect the API you wanted here, makes things easier

They are being copied when retrieving as they are simple structs.
Copy link
Member

@JoseExposito JoseExposito left a comment

Choose a reason for hiding this comment

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

I was experimenting crashes when changing the wallpaper and I though it was because of this change, but no, this code is fine.

After bisecting it looks like the problem was introduced in 4aec79a

Merging this branch, as it works as expected both with elementary's Mutter and Fedora's Mutter, and I'll try to fix the crash.

@JoseExposito JoseExposito merged commit 78abc34 into master Sep 10, 2021
@JoseExposito JoseExposito deleted the tintou/color branch September 10, 2021 06:00
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.

Clutter.Color broken since Mutter 41 compatibility patch
3 participants