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

Remove traits for disguised types without children #812

Merged
merged 3 commits into from May 22, 2019

Conversation

Projects
None yet
4 participants
@EPashkin
Copy link
Member

commented May 11, 2019

@sdroege

This comment has been minimized.

Copy link
Member

commented May 11, 2019

Seems good

@EPashkin EPashkin force-pushed the EPashkin:fix_disguised_final_types branch 2 times, most recently from 2327808 to 8093477 May 15, 2019

@EPashkin EPashkin changed the title WIP: Remove traits for disguised types without children Remove traits for disguised types without children May 15, 2019

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Updated, added builders

cc @GuillaumeGomez, @sdroege, @antoyo

Gir.toml Outdated
@@ -214,6 +214,146 @@ generate = [
"Gtk.WrapMode",
]

generate_builders = [

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez May 15, 2019

Member

I still think we should call this list "builders". :p

@antoyo

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Looks good to me.
I was under the impression that @sdroege wanted to enable builders on a case-by-case basis or by need. What do you think @sdroege?

@sdroege

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Case-by-case basis, yes. And I want that to be opt-in.

I'd be fine with enabling it for all widgets though, as long as it results in a useful API. So please carefully review the API of each builder you enable if the properties they have make any sense like that :)

@EPashkin EPashkin force-pushed the EPashkin:fix_disguised_final_types branch from 8093477 to 82fff95 May 15, 2019

@antoyo

This comment has been minimized.

Copy link
Member

commented May 15, 2019

The API of each builder in this PR makes sense.

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Seems builders need more work:

    let window = gtk::ApplicationWindowBuilder::new()
.application(application.clone())
.build();

Firstly it need clone as setter want Application but not &Application or &IsA<Application>.
Secondly it produce many colored criticals:

(builders.exe:2172): GLib-GObject-CRITICAL **: 21:04:58.413: g_object_set_qdata: assertion 'G_IS_OBJECT (object)' failed

(builders.exe:2172): GLib-GObject-WARNING **: 21:04:58.420: instance with invalid (NULL) class pointer

(builders.exe:2172): GLib-GObject-CRITICAL **: 21:04:58.425: g_signal_handlers_destroy: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed

(builders.exe:2172): GLib-GObject-CRITICAL **: 21:04:58.429: g_object_unref: assertion 'G_IS_OBJECT (object)' failed

(builders.exe:2172): GLib-GObject-WARNING **: 21:04:58.435: instance with invalid (NULL) class pointer

(builders.exe:2172): GLib-GObject-CRITICAL **: 21:04:58.439: g_signal_handlers_destroy: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed
error: process didn't exit successfully: `D:\eap\rust\0\./target\debug\builders.exe` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)
@sdroege

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@EPashkin Do you use the latest version of the glib crate with my g_object_new() fix? That looks very similar to that one.

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

@sdroege Thanks, I really not updated glib.
Now even full version works fine:

    let window = gtk::ApplicationWindowBuilder::new()
.application(application.clone())
.title("First GTK+ Program")
.border_width(10)
.window_position(gtk::WindowPosition::Center)
.default_width(350)
.default_height(70)
.build();

@EPashkin EPashkin force-pushed the EPashkin:fix_disguised_final_types branch from 82fff95 to 6593dc1 May 16, 2019

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Updated, builders still configured with array, except for "Gtk.TreeView"

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

*still but intentional.
PR IMHO is ready if CI agrees.

@EPashkin EPashkin force-pushed the EPashkin:fix_disguised_final_types branch from 6593dc1 to 46ae40d May 17, 2019

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Updated with gtk-rs/gir#775

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented May 20, 2019

CI is breaking on 1.31. You need to rename the import into something else. I vote for "_whateveeeeeeeer". XD

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

My bad, I checked on stable only.
How about "GlibObjectType" ?

@sdroege

This comment has been minimized.

@sdroege

This comment has been minimized.

Copy link
Member

commented May 21, 2019

How about "GlibObjectType" ?

Works for me though

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

All current variants about use glib::object::ObjectType as _ failure in rust 1.31.0:
rename to "GlibObjectType",
rename to "ObjectType_" (same as we do with Box),
make minimum supported version 1.34.2 (or 1.34.0) and change .travis.yml (from https://github.com/gtk-rs/cairo/pull/260/files#r285871201)

@@ -214,6 +214,144 @@ generate = [
"Gtk.WrapMode",
]

builders = [
"Gtk.AboutDialogBuilder",

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

Hmm why do we not refer to the actual type name here but the Builder type? Will we also get Gtk.BuilderBuilder? ;)

It seems nicer to specify the actual types instead of the Builder type, that's what we do everywhere else in the configuration too.

This comment has been minimized.

Copy link
@EPashkin

EPashkin May 21, 2019

Author Member

Gir support both way, for builders IMHO having suffix is more reasonable.
Although current version will have problem with Gtk.Builder instead of Gtk.BuilderBuilder,
but it have only one translation_domain: Option<String>, so I don't generate it anyway.

@EPashkin EPashkin force-pushed the EPashkin:fix_disguised_final_types branch from 76c2301 to 60af9f5 May 21, 2019

@EPashkin

This comment has been minimized.

Copy link
Member Author

commented May 21, 2019

Updated with use glib::object::ObjectType as ObjectType_;

@sdroege

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@GuillaumeGomez Please merge

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Thanks @EPashkin !

@GuillaumeGomez GuillaumeGomez merged commit 7325e89 into gtk-rs:master May 22, 2019

1 of 2 checks passed

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

@EPashkin EPashkin deleted the EPashkin:fix_disguised_final_types branch May 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.