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

Utils: add force_elementary_style () #486

Merged
merged 10 commits into from
Apr 20, 2021
Merged

Conversation

cassidyjames
Copy link
Contributor

Turns elementary/calculator#177 into a one-liner.

@cassidyjames cassidyjames requested a review from danirabbit April 20, 2021 02:42
@danirabbit
Copy link
Member

I suppose maybe this should be "force" instead of just "use"?

@cassidyjames
Copy link
Contributor Author

cassidyjames commented Apr 20, 2021

@danrabbit yeah that's probably fair. I was kind of mixed since force sounds a bit more harsh (and users can always override GTK_THEME because GTK), but I don't feel strongly.

@cassidyjames
Copy link
Contributor Author

I'm also unsure if I should handle the variant thing here or ignore if for now; I figured it might be nice to consider forcing a specific variant, but if that's too much scope creep, I'm fine dropping it.

@cassidyjames cassidyjames marked this pull request as ready for review April 20, 2021 03:50
@cassidyjames cassidyjames changed the title Utils: add use_elementary_style () Utils: add force_elementary_style () Apr 20, 2021
@Marukesu
Copy link
Contributor

Maybe we want to manage the notify signal here? with elementary/calculator#177 if i start the program with the elementary stylesheet and change to other theme they will fallback to Adwaita. If i start with Adwaita, the theme is frozen on io.elementary.blueberry ever if i set the theme to any stylesheet theme.

lib/Widgets/Utils.vala Outdated Show resolved Hide resolved
@cassidyjames
Copy link
Contributor Author

cassidyjames commented Apr 20, 2021

Maybe we want to manage the notify signal here? with elementary/calculator#177 if i start the program with the elementary stylesheet and change to other theme they will fallback to Adwaita. If i start with Adwaita, the theme is frozen on io.elementary.blueberry ever if i set the theme to any stylesheet theme.

I thought about that but honestly supporting changing to/from/between non-elementary stylesheets on the fly is much less of a concern since that's already pretty janky in Flatpak apps to begin with; I mainly want to support the case where you are on elementary OS or at least using the elementary stylesheet (so we don't change anything and just use the stylesheet as-is) or are not on elementary stylesheet so we just force a specific one.

I think this PR does the least amount of work to get that working satisfactorily.

@cassidyjames
Copy link
Contributor Author

cassidyjames commented Apr 20, 2021

I do wonder if it makes sense to check this against a list of valid variants instead of just allowing any arbitrary string; maybe we accept a Granite.Settings.AccentColor instead of a string? Again, I'm also happy to split that off into its own branch if it is creeping too much scope here. Maybe we just start without any params.

@Marukesu
Copy link
Contributor

I do wonder if it makes sense to check this against a list of valid variants instead of just allowing any arbitrary string; maybe we accept a Granite.Settings.AccentColor instead of a string? Again, I'm also happy to split that off into its own branch if it is creeping too much scope here. Maybe we just start without any params.

I don't think setting a variant here is necessary now, apps that use fixed accent are already doing it with Gtk.StyleContext and a custom css file.

@cassidyjames
Copy link
Contributor Author

@Marukesu yep that was my conclusion as well; I've removed that from this branch.

@cassidyjames cassidyjames requested a review from a team April 20, 2021 18:24
@alice-mkh
Copy link
Contributor

If it's "force", I'd expect it to also track changes and not be a one-time thing. :)

@cassidyjames
Copy link
Contributor Author

@Exalm I mean, see the previous comments in that vein. I was going to prefer use but realized that on elementary OS, that's already the assumption, so force or something does make sense. I'm open to suggestions or improvements to track changes here but honestly I don't want to turn this into a bikeshed. Further improvements can follow in future PRs.

lib/Widgets/Utils.vala Outdated Show resolved Hide resolved
lib/Widgets/Utils.vala Outdated Show resolved Hide resolved
@alice-mkh
Copy link
Contributor

Not bikeshed at all, what I mean is it can track changes as well instead of being a one-time thing. :)

cassidyjames and others added 2 commits April 20, 2021 13:51
Co-authored-by: Daniel Foré <daniel@elementary.io>
@cassidyjames cassidyjames requested a review from danirabbit April 20, 2021 19:55
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

Code looks good and works as intended afaict. Nice job :)

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