Skip to content
This repository was archived by the owner on Jun 8, 2021. It is now read-only.

Add CSS styling support #104

Merged
merged 3 commits into from
Jun 18, 2015
Merged

Add CSS styling support #104

merged 3 commits into from
Jun 18, 2015

Conversation

gsingh93
Copy link
Member

This PR adds CSS styling support by implementing CssProvider, StyleProvider, and StyleContext. An approach similar to that used with WidgetTrait was taken. get_screen is also implemented for WidgetTrait so this screen can be used with StyleContext::add_provider_for_screen.

There are some TODOs in the code for adding the proper #![feature] attributes. The Gtk documentation doesn't give this number for every method/class, and I think this is due to a lack of documentation, not the fact that the method/class was introduced before gtk3. How should I find these feature numbers?

@GuillaumeGomez
Copy link
Member

Very nice work ! =D
@gkoz: Code looks fine for me, I'm waiting your review. ;-)

pub fn load_from_path(path: &str) -> Result<CssProvider, glib::Error> {
unsafe {
let pointer = ffi::gtk_css_provider_new();
let mut error: *mut glib::ffi::GError = ::std::mem::zeroed();
Copy link
Member

Choose a reason for hiding this comment

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

This can simply be let mut error = ptr::null_mut();

Copy link
Member

Choose a reason for hiding this comment

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

It should be let mut error = ptr::null_mut();.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I was looking for how to do that. I saw ptr::null(), but missed ptr::null_mut().

@gkoz
Copy link
Member

gkoz commented Jun 16, 2015

Since you use impl_GObjectFunctions! I don't think there's a need for FFIStyleProvider.

The reference counting isn't handled well but I guess this will be fixed by the objects reform.

If the minimum GTK version for a method or class isn't specified it's safe to assume that it's present in GTK 3.4, which is the lowest version we support (also 'since 3.0' and 'since 3.2' can be ignored).

@gsingh93
Copy link
Member Author

Ok, fixed.

One more question: The gtk_style_context_add_provider method takes a guint method but I pass in an enum. Could the size mismatch cause a problem?

@gkoz
Copy link
Member

gkoz commented Jun 16, 2015

The gtk_style_context_add_provider method takes a guint method but I pass in an enum. Could the size mismatch cause a problem?

I believe a lot of the definitions currently assume that sizeof(c_enum) == sizeof(c_int) but here we can avoid that assumption by making the ffi type c_uint and casting the enum to c_uint in add_provider. What if someone wanted to pass a value in between the constants (e.g. 117) though?

@gsingh93
Copy link
Member Author

What if someone wanted to pass a value in between the constants (e.g. 117) though?

Should I add another variant called CustomPriority(u32)?

@gkoz
Copy link
Member

gkoz commented Jun 16, 2015

Should I add another variant called CustomPriority(u32)?

It would not be a C-like enum any more so you'd lose any conveniences and some derived traits. And anyone could put 100 or 200 there etc. Maybe simply using constants is the way. Or we could leave the type as u32 and tell the users to do the casting themselves if they want predefined values.

@gsingh93
Copy link
Member Author

Ok, I think I'll change the type to a u32 and convert the enum into constants.

@gsingh93
Copy link
Member Author

Ok, I added the constants. You can access them with gtk::traits::style_provider::PRIORITY_USER, as I decided not to export them.

@gsingh93
Copy link
Member Author

I also just added a commit that documents the priorities. I copied the documentation directly from the Gtk docs.

/// The priority used for default style information that is used in the absence of themes.
/// Note that this is not very useful for providing default styling for custom style classes,
/// themes are likely to override styling provided at this priority with catch-all * {...} rules.
pub const PRIORITY_FALLBACK: u32 = 1;
Copy link
Member

Choose a reason for hiding this comment

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

The idiomatic approach would be to define GTK_STYLE_PROVIDER_PRIORITY_* constants in the sys crate (those definitions will become autogenerated eventually) and use those on the right hand side here.

@gkoz
Copy link
Member

gkoz commented Jun 17, 2015

You can access them with gtk::traits::style_provider::PRIORITY_USER, as I decided not to export them.

It looks like we're going to reexport all symbols in the top namespace and the names would have to be longer in that case, e.g. STYLE_PROVIDER_PRIORITY_USER.

We may want to investigate using associated constants -- they seem to be available in the nightlies. Example: https://github.com/rust-lang/rust/blob/master/src/test/run-pass/associated-const-inherent-impl.rs

@gsingh93
Copy link
Member Author

It looks like we're going to reexport all symbols in the top namespace and the names would have to be longer in that case, e.g. STYLE_PROVIDER_PRIORITY_USER

Why not just use proper namespaces?

We may want to investigate using associated constants -- they seem to be available in the nightlies. Example: https://github.com/rust-lang/rust/blob/master/src/test/run-pass/associated-const-inherent-impl.rs

StyleProvider is a trait here, so it wouldn't work. Maybe after object reform?

@gsingh93
Copy link
Member Author

We may want to investigate using associated constants -- they seem to be available in the nightlies. Example: https://github.com/rust-lang/rust/blob/master/src/test/run-pass/associated-const-inherent-impl.rs

StyleProvider is a trait here, so it wouldn't work. Maybe after object reform?

Err, actually if you can provide default values for constants (which wasn't in the documentation), than we could do this.

@gkoz
Copy link
Member

gkoz commented Jun 17, 2015

Why not just use proper namespaces?

Some reasons here: gtk-rs/glib#47 (comment)

It may just be a fool's errand to try to add structure to all of GTK because it's too diverse. While converting every module to the new framework I find it very difficult to organize them coherently. An arbitrary mediocre structure may be worse that the flat convention.

@gkoz
Copy link
Member

gkoz commented Jun 17, 2015

I guess, we could also just have a flat series of modules (gtk::widget, gtk::button, gtk::style_provider, etc) and have constants live there. I'm not enthusiastic about this approach.

@gsingh93
Copy link
Member Author

Ok, I just left them as normal constants.

@gkoz
Copy link
Member

gkoz commented Jun 17, 2015

I guess we can reexport these consts in the root module now.

Otherwise seems okay to merge. Small style nits I'd just tweak in the process of refactoring.

@gsingh93
Copy link
Member Author

Oops, forgot to reexport. Fixed.

@gkoz
Copy link
Member

gkoz commented Jun 18, 2015

👍
@GuillaumeGomez

@GuillaumeGomez
Copy link
Member

@gkoz: Thanks for taking care of this one and helping him !
@gsingh93: Thanks for your work !

GuillaumeGomez added a commit that referenced this pull request Jun 18, 2015
Add CSS styling support
@GuillaumeGomez GuillaumeGomez merged commit 0c9d0c1 into gtk-rs:master Jun 18, 2015
alex179ohm pushed a commit to alex179ohm/gtk that referenced this pull request Oct 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants