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

Add RadioToolButton #584

Merged
merged 3 commits into from Oct 25, 2017

Conversation

Projects
None yet
4 participants
@pizzaiter
Contributor

pizzaiter commented Oct 24, 2017

This PR adds RadioToolButton. The implementation is very similar to that of RadioButton.

pizzaiter added some commits Oct 24, 2017

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez
Member

GuillaumeGomez commented Oct 24, 2017

Thanks!

cc @EPashkin @sdroege

pub fn new() -> RadioToolButton {
assert_initialized_main_thread!();
unsafe {
ToolItem::from_glib_none(ffi::gtk_radio_tool_button_new(ptr::null_mut())).downcast_unchecked()

This comment has been minimized.

@sdroege

sdroege Oct 24, 2017

Member

Why ToolItem and not RadioToolButton? And generally, you can just use from_glib_none and let the compiler figure out the right implementation

@sdroege

sdroege Oct 24, 2017

Member

Why ToolItem and not RadioToolButton? And generally, you can just use from_glib_none and let the compiler figure out the right implementation

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 24, 2017

Member

Oh good catch, didn't see it at all...

@GuillaumeGomez

GuillaumeGomez Oct 24, 2017

Member

Oh good catch, didn't see it at all...

This comment has been minimized.

@EPashkin

EPashkin Oct 24, 2017

Member

It because says Gtk-3.0.gir:

        <return-value transfer-ownership="none">
          <doc xml:space="preserve">The new #GtkRadioToolButton</doc>
          <type name="ToolItem" c:type="GtkToolItem*"/>
        </return-value>
@EPashkin

EPashkin Oct 24, 2017

Member

It because says Gtk-3.0.gir:

        <return-value transfer-ownership="none">
          <doc xml:space="preserve">The new #GtkRadioToolButton</doc>
          <type name="ToolItem" c:type="GtkToolItem*"/>
        </return-value>

This comment has been minimized.

@pizzaiter

pizzaiter Oct 25, 2017

Contributor

I thought it would be best to be consistent with what gets auto-generated for similar functions, even if it does not really make a difference how one calls from_glib_none.

@pizzaiter

pizzaiter Oct 25, 2017

Contributor

I thought it would be best to be consistent with what gets auto-generated for similar functions, even if it does not really make a difference how one calls from_glib_none.

pub fn new_from_stock(stock_id: &str) -> RadioToolButton {
assert_initialized_main_thread!();
unsafe {
ToolItem::from_glib_none(ffi::gtk_radio_tool_button_new_from_stock(ptr::null_mut(), stock_id.to_glib_none().0)).downcast_unchecked()

This comment has been minimized.

@sdroege

sdroege Oct 24, 2017

Member

This one is deprecated since 3.10, maybe let's not add it? Docs say that gtk_radio_tool_button_new() should be used instead.

Also it probably makes sense to expose something for the weird groups API it provides? IIRC we already have the same problem with other group API elsewhere in GTK ( @GuillaumeGomez you remember? ) so let's take inspiration from there.

@sdroege

sdroege Oct 24, 2017

Member

This one is deprecated since 3.10, maybe let's not add it? Docs say that gtk_radio_tool_button_new() should be used instead.

Also it probably makes sense to expose something for the weird groups API it provides? IIRC we already have the same problem with other group API elsewhere in GTK ( @GuillaumeGomez you remember? ) so let's take inspiration from there.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Oct 24, 2017

Member

I don't recall which at all. :-/

@GuillaumeGomez

GuillaumeGomez Oct 24, 2017

Member

I don't recall which at all. :-/

This comment has been minimized.

@sdroege

sdroege Oct 24, 2017

Member

@EPashkin Maybe? Some other RadioWhatever API (the plain RadioButton? RadioMenuItems?) where you have groups of things. I don't have time for looking closer now and GTK UI stuff is not really my main area of expertise so I would have to look up, sorry :)

@sdroege

sdroege Oct 24, 2017

Member

@EPashkin Maybe? Some other RadioWhatever API (the plain RadioButton? RadioMenuItems?) where you have groups of things. I don't have time for looking closer now and GTK UI stuff is not really my main area of expertise so I would have to look up, sorry :)

This comment has been minimized.

@EPashkin

EPashkin Oct 24, 2017

Member

You meant #488?
Problem with this class that it don't have join_group function but it has group property that maybe works same and better deignored.
I will check this PR more later.

@EPashkin

EPashkin Oct 24, 2017

Member

You meant #488?
Problem with this class that it don't have join_group function but it has group property that maybe works same and better deignored.
I will check this PR more later.

This comment has been minimized.

@pizzaiter

pizzaiter Oct 25, 2017

Contributor

I actually missed that the from_stock functions are deprecated. I can remove them if you want. I am fine either way.

For the group API, I will try to implement what @EPashkin proposed below. At the moment one has to use one of the new_from_widget functions.

@pizzaiter

pizzaiter Oct 25, 2017

Contributor

I actually missed that the from_stock functions are deprecated. I can remove them if you want. I am fine either way.

For the group API, I will try to implement what @EPashkin proposed below. At the moment one has to use one of the new_from_widget functions.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Oct 24, 2017

Member

Problem that get_group return vector of RadioButton: I don't see way get this button from RadioToolButton as C function _gtk_tool_button_get_button is private, but maybe this not real problem.

Tested that group property works same as join_group.
Generated code looks like this:

    fn set_property_group(&self, group: Option<&RadioToolButton>) {
        unsafe {
            gobject_ffi::g_object_set_property(self.to_glib_none().0, "group".to_glib_none().0, Value::from(group).to_glib_none().0);
        }
    }

Only minor problem with it that it need Option, and we prefer use Into<Option<&RadioToolButton>> to simplify usage.
I propose ignore property group (as now) and add manual version of join_group with Into<Option parameter same as other radio types and use this code in it.

Not sure about _from_stock but if it works for @pizzaiter, then IMHO it can stay.

Member

EPashkin commented Oct 24, 2017

Problem that get_group return vector of RadioButton: I don't see way get this button from RadioToolButton as C function _gtk_tool_button_get_button is private, but maybe this not real problem.

Tested that group property works same as join_group.
Generated code looks like this:

    fn set_property_group(&self, group: Option<&RadioToolButton>) {
        unsafe {
            gobject_ffi::g_object_set_property(self.to_glib_none().0, "group".to_glib_none().0, Value::from(group).to_glib_none().0);
        }
    }

Only minor problem with it that it need Option, and we prefer use Into<Option<&RadioToolButton>> to simplify usage.
I propose ignore property group (as now) and add manual version of join_group with Into<Option parameter same as other radio types and use this code in it.

Not sure about _from_stock but if it works for @pizzaiter, then IMHO it can stay.

@pizzaiter

This comment has been minimized.

Show comment
Hide comment
@pizzaiter

pizzaiter Oct 25, 2017

Contributor

I pushed a commit that adds join_group as @EPashkin suggested.

Contributor

pizzaiter commented Oct 25, 2017

I pushed a commit that adds join_group as @EPashkin suggested.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Oct 25, 2017

Member

@pizzaiter Thanks.
About ToolItem::from_glib_none you can't replace this as ffi function returns ToolItem*

@GuillaumeGomez, @sdroege 👍 for merge from me

Member

EPashkin commented Oct 25, 2017

@pizzaiter Thanks.
About ToolItem::from_glib_none you can't replace this as ffi function returns ToolItem*

@GuillaumeGomez, @sdroege 👍 for merge from me

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Oct 25, 2017

Member

For me as well, so let's wait for @sdroege's confirmation.

Member

GuillaumeGomez commented Oct 25, 2017

For me as well, so let's wait for @sdroege's confirmation.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Oct 25, 2017

Member

I have no further comments, go ahead :)

Member

sdroege commented Oct 25, 2017

I have no further comments, go ahead :)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Oct 25, 2017

Member

Thanks then!

Member

GuillaumeGomez commented Oct 25, 2017

Thanks then!

@GuillaumeGomez GuillaumeGomez merged commit 8d5f44a into gtk-rs:master Oct 25, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pizzaiter

This comment has been minimized.

Show comment
Hide comment
@pizzaiter

pizzaiter Oct 26, 2017

Contributor

🎉

Contributor

pizzaiter commented Oct 26, 2017

🎉

@pizzaiter pizzaiter deleted the pizzaiter:radiotoolbutton branch Oct 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment