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

Set accent color based on wallpaper #263

Merged
merged 29 commits into from May 4, 2021

Conversation

meisenzahl
Copy link
Member

@meisenzahl meisenzahl commented Apr 5, 2021

Goes with elementary/gala#1104

Works in conjunction with elementary/granite#483

Bildschirmfoto von 2021-04-11 22 04 23

@meisenzahl meisenzahl closed this Apr 8, 2021
@meisenzahl meisenzahl force-pushed the set-accent-color-based-on-wallpaper branch from af25074 to 0411780 Compare April 8, 2021 18:38
@meisenzahl meisenzahl reopened this Apr 8, 2021
@meisenzahl
Copy link
Member Author

It seems to work, though I get warnings when switching:

(io.elementary.switchboard:27819): GLib-CRITICAL **: 21:10:48.816: g_variant_get_type: assertion 'value != NULL' failed

(io.elementary.switchboard:27819): GLib-CRITICAL **: 21:10:48.816: g_variant_type_is_subtype_of: assertion 'g_variant_type_check (type)' failed

(io.elementary.switchboard:27819): GLib-CRITICAL **: 21:10:48.816: g_variant_get_int32: assertion 'g_variant_is_of_type (value, G_VARIANT_TYPE_INT32)' failed

…entary/switchboard-plug-pantheon-shell into set-accent-color-based-on-wallpaper
@meisenzahl
Copy link
Member Author

Works in conjunction with elementary/granite#483

@JoseExposito
Copy link
Member

I'm not sure what causes it, but when the Dark theme is selected, changing the accent color seems to toggle it to Default.
Let me know if you are not able to reproduce the issue and I'll add more details.

@meisenzahl
Copy link
Member Author

Yes I can confirm the problem. I'll take a closer look. Good catch!

@meisenzahl meisenzahl closed this Apr 11, 2021
@meisenzahl meisenzahl reopened this Apr 11, 2021
@meisenzahl
Copy link
Member Author

@JoseExposito but for me the error occurs only in the settings...

Bildschirmfoto von 2021-04-11 19 49 03

@meisenzahl
Copy link
Member Author

@JoseExposito I found it. Can you confirm that it works for you too?

JoseExposito
JoseExposito previously approved these changes Apr 12, 2021
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.

Now it works fine! 🎉

The code is fine, but I'd appreciate if someone with more experience working on this code base could have a look as well.

I left the UX review to @elementary/ux. I guess it is work in progress, but some wallpapers don't set the expected accent color.

@meisenzahl
Copy link
Member Author

@JoseExposito do you mean the accent colors for the included wallpapers? @cassidyjames had picked them by hand: elementary/settings-daemon#25 (comment)

cassidyjames
cassidyjames previously approved these changes Apr 29, 2021
@cassidyjames
Copy link
Contributor

@meisenzahl so UI-wise this looks good, but I'm seeing and issue where the color gets out of sync with what is actually being set under the hood. I'm not sure if it's an issue in Gala or here, though.

@cassidyjames cassidyjames added this to In progress in 6.0 Odin Release via automation May 2, 2021
@meisenzahl
Copy link
Member Author

@cassidyjames this is an issue with this PR. The logic for Onboarding is working. I will fix this.

@meisenzahl
Copy link
Member Author

@cassidyjames I found the problem. There was a missing line to set the active state of the RadioButton. Sorry that you had to spend more time on the review.

@cassidyjames cassidyjames requested a review from a team May 4, 2021 15:53
src/Views/Appearance.vala Outdated Show resolved Hide resolved
src/Views/Appearance.vala Outdated Show resolved Hide resolved
src/Views/Appearance.vala Outdated Show resolved Hide resolved
src/Views/Appearance.vala Outdated Show resolved Hide resolved
@meisenzahl
Copy link
Member Author

@danrabbit thanks for the great suggestions!

@danirabbit danirabbit merged commit 83e7dbf into master May 4, 2021
@danirabbit danirabbit deleted the set-accent-color-based-on-wallpaper branch May 4, 2021 18:25
6.0 Odin Release automation moved this from In progress to Done May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants