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

Migrate subclassing infrastructure to glib-rs #392

Merged
merged 26 commits into from Nov 20, 2018

Conversation

@sdroege
Copy link
Member

@sdroege sdroege commented Nov 19, 2018

For an example see the unit test in src/subclass/object.rs at the bottom: https://github.com/sdroege/glib-rs/blob/3eee4c7da6e6947efa4c1970d7d36e17cbfca577/src/subclass/object.rs#L288-L379 . Another example can be found here gtk-rs/examples#205 (https://github.com/gtk-rs/examples/blob/4cf46ec7a6409b92ed726eaec74d4d983c84e386/src/bin/listbox_model.rs#L251-L401)

This is a complete rewrite of gobject-subclass to be more flexible, lightweight, simple and to be usable with gnome-class too.

I have a WIP port of gst-plugin-rs for this (need to migrate all the code to the bindings first) and it all works well.

I also have a minimal change to gir (gtk-rs/gir#669) to add the Rust class type to the glib_wrapper! macro, which should be merged first. And then gtk-rs/gir#604 can be updated to actually generate quite a bit of the boilerplate code.


This is now part of the proper bindings instead of an external crate because otherwise the API can't be made as nice as it is now due to not being able to implement traits for types defined in other crates, and generally more traits for marking all the relationships are needed.

sdroege added 17 commits Nov 18, 2018
And fill it in by default with () in the macro to prevent existing code
from failing to compile.
And keep the existing ones on ObjectExt as wrappers around ObjectClass
The object is destroyed already at that time
It is potentially unsafe as the object is not fully initialized yet and
especially does not have its private struct created yet (we just do
exactly that here!).

It's better to use Object::constructed() for any initialization.
And add a test for properties
sdroege added 2 commits Nov 19, 2018
That way the users of the API don't have to cast.
…al method is actually called
Copy link
Member

@EPashkin EPashkin left a comment

Great work

src/lib.rs Show resolved Hide resolved
src/subclass/mod.rs Outdated Show resolved Hide resolved
src/subclass/mod.rs Outdated Show resolved Hide resolved
src/subclass/guard.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 19, 2018

Global comments while I'm reviewing: can you add punctuation on your (doc) comments please? A lot of them are missing a dot at the end.

src/subclass/simple.rs Outdated Show resolved Hide resolved
src/subclass/types.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 19, 2018

Could you also add a section into the README about this please?

src/object.rs Outdated Show resolved Hide resolved
src/subclass/mod.rs Outdated Show resolved Hide resolved
@sdroege sdroege force-pushed the sdroege:subclass branch 2 times, most recently from 61288ea to 7086167 Nov 19, 2018
src/subclass/object.rs Outdated Show resolved Hide resolved
src/subclass/types.rs Outdated Show resolved Hide resolved
src/subclass/types.rs Outdated Show resolved Hide resolved
src/subclass/types.rs Outdated Show resolved Hide resolved
src/subclass/types.rs Outdated Show resolved Hide resolved
src/subclass/types.rs Outdated Show resolved Hide resolved
src/subclass/types.rs Outdated Show resolved Hide resolved
src/subclass/types.rs Outdated Show resolved Hide resolved
@sdroege sdroege force-pushed the sdroege:subclass branch from 7086167 to 9698750 Nov 19, 2018
@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 20, 2018

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit dab55a4 into gtk-rs:master Nov 20, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants