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

Replace functions for RadioButton and RadioMenuItem #529

Merged
merged 2 commits into from Jun 19, 2017

Conversation

Projects
None yet
3 participants
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 14, 2017

Member

@GuillaumeGomez, @sdroege please check it.

Member

EPashkin commented Jun 14, 2017

@GuillaumeGomez, @sdroege please check it.

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jun 14, 2017

Member

Seems good for me, however I never used the radio groups so I'd prefer to rely on @sdroege on this.

Member

GuillaumeGomez commented Jun 14, 2017

Seems good for me, however I never used the radio groups so I'd prefer to rely on @sdroege on this.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jun 15, 2017

Member

I never used the radio groups API either, at least not in the last 10 years, and don't really work with GTK

This looks good to me and should work fine, however it seems potentially confusing to users as the normal constructors now don't provide a way to define a group and you have to use the ones with _widget instead. Maybe implement that ones with the GSList group manually, so that the actual GSList is passed there and not a NULL pointer?

Member

sdroege commented Jun 15, 2017

I never used the radio groups API either, at least not in the last 10 years, and don't really work with GTK

This looks good to me and should work fine, however it seems potentially confusing to users as the normal constructors now don't provide a way to define a group and you have to use the ones with _widget instead. Maybe implement that ones with the GSList group manually, so that the actual GSList is passed there and not a NULL pointer?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 15, 2017

Member

Problem with GSList changed on every g_slist_prepend so it almost impossible make it actual when it changed in C side

Member

EPashkin commented Jun 15, 2017

Problem with GSList changed on every g_slist_prepend so it almost impossible make it actual when it changed in C side

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jun 15, 2017

Member

I was thinking of just getting the GSList* from one of the buttons in the list (the first I guess?), probably making sure that all of the buttons in the list are in the same group (or maybe not?), and then passing it to the constructor.

Member

sdroege commented Jun 15, 2017

I was thinking of just getting the GSList* from one of the buttons in the list (the first I guess?), probably making sure that all of the buttons in the list are in the same group (or maybe not?), and then passing it to the constructor.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 15, 2017

Member

Then we need ensure that user don't use list after it, or we again have wrong list (need change to consume group parameter).
I don't see it really more usable

let rd1 = new_with_label(None, "a");
let gr = rd1.get_group();
let rd2 = new_with_label(gr, "b");
let gr = rd2.get_group();
let rd3 = new_with_label(gr, "c");

that realized in this PR

let rd1 = new_with_label("a");
let rd2 = new_with_label("b");
let rd3 = new_with_label("c");
rd2.join_group(&rd1);
rd3.join_group(&rd1);
Member

EPashkin commented Jun 15, 2017

Then we need ensure that user don't use list after it, or we again have wrong list (need change to consume group parameter).
I don't see it really more usable

let rd1 = new_with_label(None, "a");
let gr = rd1.get_group();
let rd2 = new_with_label(gr, "b");
let gr = rd2.get_group();
let rd3 = new_with_label(gr, "c");

that realized in this PR

let rd1 = new_with_label("a");
let rd2 = new_with_label("b");
let rd3 = new_with_label("c");
rd2.join_group(&rd1);
rd3.join_group(&rd1);
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jun 15, 2017

Member

I was thinking of never passing the GSList* to the user, but using it internally in the constructor, e.g.

    pub fn new(group: &[RadioMenuItem]) -> RadioMenuItem {
        assert_initialized_main_thread!();
        unsafe {
            let c_group = ffi::gtk_radio_menu_item_get_group(to_glib_none(group[0])); // check if there is actually group[0] or if it's empty
            // possibly: check if all in group have the same group, or make it so
            Widget::from_glib_none(ffi::gtk_radio_menu_item_new(c_group)).downcast_unchecked()
        }
    }

But I missed the join_group API, I guess it's sufficient like that then :)

Member

sdroege commented Jun 15, 2017

I was thinking of never passing the GSList* to the user, but using it internally in the constructor, e.g.

    pub fn new(group: &[RadioMenuItem]) -> RadioMenuItem {
        assert_initialized_main_thread!();
        unsafe {
            let c_group = ffi::gtk_radio_menu_item_get_group(to_glib_none(group[0])); // check if there is actually group[0] or if it's empty
            // possibly: check if all in group have the same group, or make it so
            Widget::from_glib_none(ffi::gtk_radio_menu_item_new(c_group)).downcast_unchecked()
        }
    }

But I missed the join_group API, I guess it's sufficient like that then :)

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 15, 2017

Member

Hm. this way also doable (but need some checks for empty list). I add it to issue.

Member

EPashkin commented Jun 15, 2017

Hm. this way also doable (but need some checks for empty list). I add it to issue.

@EPashkin EPashkin referenced this pull request Jun 15, 2017

Closed

Radio buttons problem #488

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 17, 2017

Member

@sdroege You agree to merge PR as is?
IMHO arrayed version while doable, confuses the user: usage &[] for first member, need collect members to array or use &[firstmember] to adding.

Member

EPashkin commented Jun 17, 2017

@sdroege You agree to merge PR as is?
IMHO arrayed version while doable, confuses the user: usage &[] for first member, need collect members to array or use &[firstmember] to adding.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jun 19, 2017

Member

Ping

Member

EPashkin commented Jun 19, 2017

Ping

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jun 19, 2017

Member

Fine with me, yes

Member

sdroege commented Jun 19, 2017

Fine with me, yes

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jun 19, 2017

Member

Good for me too. Thanks!

Member

GuillaumeGomez commented Jun 19, 2017

Good for me too. Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 4e0cb02 into gtk-rs:master Jun 19, 2017

2 checks passed

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

@EPashkin EPashkin deleted the EPashkin:radiobutton branch Jun 19, 2017

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