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

Init: set accent color from portal #704

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Conversation

danirabbit
Copy link
Member

@danirabbit danirabbit commented Mar 20, 2024

It currently isn't being updated while running because it looks like our portal isn't sending settings-changed signals?

Requires:

@danirabbit danirabbit marked this pull request as ready for review March 20, 2024 19:01
@danirabbit danirabbit requested a review from a team March 20, 2024 19:01
This was referenced Mar 20, 2024
zeebok
zeebok previously approved these changes Jun 5, 2024
lib/Widgets/Settings.vala Outdated Show resolved Hide resolved
lib/Widgets/Settings.vala Outdated Show resolved Hide resolved
@zeebok zeebok dismissed their stale review June 11, 2024 13:05

Ryo made some good comments

@danirabbit danirabbit requested a review from ryonakano June 16, 2024 18:09
@ryonakano
Copy link
Contributor

@danirabbit Hmm, could you tell me how to test this? Maybe using an label with .accent style in a Flatpak app and then changing accent color in System Settings? 🤔

@danirabbit
Copy link
Member Author

@ryonakano it should work with the demo. I can’t remember if this needs the new granite stylesheet or if it works with the current stylesheet though. You might need to set the stylesheet in inspector to Granite

@danirabbit

This comment was marked as resolved.

@@ -1,3 +1,11 @@
.accent {
color: #{'mix(@accent_color, @fg_color, 0.27)'};
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this will produce very good results for e.g. yellow. In libadwaita I had much better results with oklch colorspace (tho that would require you to update gtk once 4.16 is out)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, that or calculate it programmatically ofc, then you don't need to wait.

@alice-mkh
Copy link
Contributor

It currently isn't being updated while running because it looks like our portal isn't sending settings-changed signals?

I see what's happening. It stops working once you open inspector. Looks like while you do support multiple GdkDisplays for the most part, accent_provider is not per-display and basically you need separate providers for each.

What does completely fail to pick changes for me is color-scheme instead, at least in calculator.

@@ -61,7 +61,7 @@ namespace Granite {

Gtk.StyleContext.remove_provider_for_display (display, accent_provider);
accent_provider.load_from_string ("@define-color accent_color %s;".printf (accent_color));
Gtk.StyleContext.add_provider_for_display (display, accent_provider, Gtk.STYLE_PROVIDER_PRIORITY_APPLICATION);
Gtk.StyleContext.add_provider_for_display (display, accent_provider, Gtk.STYLE_PROVIDER_PRIORITY_APPLICATION - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use THEME + 1 instead

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried THEME + 1 but it didn't work for some reason

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. THat is strange.


accent_color = parse_color (variant);

portal.setting_changed.connect ((scheme, key, value) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@alice-mkh maybe it's because I'm on an older version of some library but I'm not getting anything at all here when I change the color in system settings so I can't really test further because it's broken for me all the way at the beginning

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh okay. It did work for me, but only until I open inspector due to the global provider issue I mentioned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I can't even test that 😅

@danirabbit danirabbit marked this pull request as draft June 21, 2024 21:42
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.

4 participants