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 ListModel and ListStore bindings #131

Merged
merged 1 commit into from Jul 2, 2018

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented Jul 1, 2018

This would later allow us to add the model support of gtk::ListBox

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 1, 2018

Member

CC @EPashkin

Currently this fails to compile because of https://github.com/gtk-rs/sys/blob/b76cceca2bc81c822f1bb7320783824bdd0f79fe/gio-sys/src/lib.rs#L8240-L8241 . The reason for that is this in the GIR file

          <parameter name="additions" transfer-ownership="none">
            <doc xml:space="preserve">the items to add</doc>
            <array length="3" zero-terminated="0" c:type="gpointer*">
              <type name="GObject.Object"/>
            </array>
          </parameter>

Currently gir is checking for subclassing relationship between the types given there, however it does not consider raw pointers. This is in codegen/sys/ffi_type.rs:ffi_inner(). For any pointer-typed parameter, we should consider gpointer valid here.

I'm not sure how to teach gir that though, I only got it to produce *mut gobject_ffi::GObject (one *mut missing) as the information that this should be a pointer to a pointer seems to be missing at that place.

Member

sdroege commented Jul 1, 2018

CC @EPashkin

Currently this fails to compile because of https://github.com/gtk-rs/sys/blob/b76cceca2bc81c822f1bb7320783824bdd0f79fe/gio-sys/src/lib.rs#L8240-L8241 . The reason for that is this in the GIR file

          <parameter name="additions" transfer-ownership="none">
            <doc xml:space="preserve">the items to add</doc>
            <array length="3" zero-terminated="0" c:type="gpointer*">
              <type name="GObject.Object"/>
            </array>
          </parameter>

Currently gir is checking for subclassing relationship between the types given there, however it does not consider raw pointers. This is in codegen/sys/ffi_type.rs:ffi_inner(). For any pointer-typed parameter, we should consider gpointer valid here.

I'm not sure how to teach gir that though, I only got it to produce *mut gobject_ffi::GObject (one *mut missing) as the information that this should be a pointer to a pointer seems to be missing at that place.

fn insert<P: IsA<glib::Object>>(&self, position: u32, item: &P);
//#[cfg(any(feature = "v2_44", feature = "dox"))]
//fn insert_sorted<P: IsA<glib::Object>, Q: Into<Option</*Unimplemented*/Fundamental: Pointer>>>(&self, item: &P, compare_func: /*Unknown conversion*//*Unimplemented*/CompareDataFunc, user_data: Q) -> u32;

This comment has been minimized.

@sdroege

sdroege Jul 1, 2018

Member

For these scoped function pointers we should probably add some support to gir also, it's well-defined how they work

@sdroege

sdroege Jul 1, 2018

Member

For these scoped function pointers we should probably add some support to gir also, it's well-defined how they work

impl<O: IsA<ListStore> + IsA<glib::object::Object>> ListStoreExtManual for O {
#[cfg(any(feature = "v2_44", feature = "dox"))]
fn insert_sorted<P: IsA<glib::Object>, F: FnMut(&Object, &Object) -> Ordering>(&self, item: &P, compare_func: F) -> u32 {

This comment has been minimized.

@EPashkin

EPashkin Jul 1, 2018

Member

Maybe compare_func need be 'FnMut(&P, &P) -> Ordering`?

@EPashkin

EPashkin Jul 1, 2018

Member

Maybe compare_func need be 'FnMut(&P, &P) -> Ordering`?

This comment has been minimized.

@sdroege

sdroege Jul 1, 2018

Member

No, that's unsafe because you could store any GObject inside that is a subclass of the type given in the constructor. It could be the type from the constructor, but here all different kinds of subclasses of that can be passed in

@sdroege

sdroege Jul 1, 2018

Member

No, that's unsafe because you could store any GObject inside that is a subclass of the type given in the constructor. It could be the type from the constructor, but here all different kinds of subclasses of that can be passed in

This comment has been minimized.

@EPashkin

EPashkin Jul 1, 2018

Member

You right

@EPashkin

EPashkin Jul 1, 2018

Member

You right

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 1, 2018

Member

@sdroege You right about g_list_store_splice.
You add PR to gir?

Member

EPashkin commented Jul 1, 2018

@sdroege You right about g_list_store_splice.
You add PR to gir?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 1, 2018

Member

@sdroege You right about g_list_store_splice.
You add PR to gir?

I tried but I couldn't figure out how to fix this in gir, see #131 (comment)

Member

sdroege commented Jul 1, 2018

@sdroege You right about g_list_store_splice.
You add PR to gir?

I tried but I couldn't figure out how to fix this in gir, see #131 (comment)

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 1, 2018

Member

Ok, I try look at this.
Function need be

pub fn g_list_store_splice(store: *mut GListStore, position: c_uint, n_removals: c_uint,
additions: *mut [*mut GObject], n_additions: c_uint);

or

pub fn g_list_store_splice(store: *mut GListStore, position: c_uint, n_removals: c_uint,
additions: *mut [*mut gpointer], n_additions: c_uint);

?

Member

EPashkin commented Jul 1, 2018

Ok, I try look at this.
Function need be

pub fn g_list_store_splice(store: *mut GListStore, position: c_uint, n_removals: c_uint,
additions: *mut [*mut GObject], n_additions: c_uint);

or

pub fn g_list_store_splice(store: *mut GListStore, position: c_uint, n_removals: c_uint,
additions: *mut [*mut gpointer], n_additions: c_uint);

?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 1, 2018

Member

It needs to be pub fn g_list_store_splice(store: *mut GListStore, position: c_uint, n_removals: c_uint, additions: *mut *mut gobject_ffi::GObject, n_additions: c_uint);

Thanks!

Member

sdroege commented Jul 1, 2018

It needs to be pub fn g_list_store_splice(store: *mut GListStore, position: c_uint, n_removals: c_uint, additions: *mut *mut gobject_ffi::GObject, n_additions: c_uint);

Thanks!

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 1, 2018

Member

It really difficult problem, I currently no idea how to fix gir.
Maybe just add this function manual way?

Member

EPashkin commented Jul 1, 2018

It really difficult problem, I currently no idea how to fix gir.
Maybe just add this function manual way?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 1, 2018

Member

It really difficult problem, I currently no idea how to fix gir.
Maybe just add this function manual way?

Why are we doing these checks for matching types? If there is a type annotation like here, we should be able to always trust it.

And instead of manually implementing, I could also update gir-files to use the correct type there (GObject**) instead.

Member

sdroege commented Jul 1, 2018

It really difficult problem, I currently no idea how to fix gir.
Maybe just add this function manual way?

Why are we doing these checks for matching types? If there is a type annotation like here, we should be able to always trust it.

And instead of manually implementing, I could also update gir-files to use the correct type there (GObject**) instead.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 1, 2018

Member

Don't know why we check it.
Agree that changing Gio-2.0.gir is better option

Member

EPashkin commented Jul 1, 2018

Don't know why we check it.
Agree that changing Gio-2.0.gir is better option

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 1, 2018

Member

Agree that changing Gio-2.0.gir is better option

Will do that then tonight, thanks :)

Member

sdroege commented Jul 1, 2018

Agree that changing Gio-2.0.gir is better option

Will do that then tonight, thanks :)

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 1, 2018

Member

Hm, seems solution found in gir, I missed that gpointer add "*mut" to base type itself

            if inner != glib_name {
+                if inner == "gpointer" {
+                    fix_name(env, tid, &glib_name).map_any(|s| format!("*mut {}", s))
                } else if implements_c_type(env, tid, &inner) {

Member

EPashkin commented Jul 1, 2018

Hm, seems solution found in gir, I missed that gpointer add "*mut" to base type itself

            if inner != glib_name {
+                if inner == "gpointer" {
+                    fix_name(env, tid, &glib_name).map_any(|s| format!("*mut {}", s))
                } else if implements_c_type(env, tid, &inner) {

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 1, 2018

Member

@sdroege I restarted appveyor, it now fail on examples

Member

EPashkin commented Jul 1, 2018

@sdroege I restarted appveyor, it now fail on examples

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 1, 2018

Member

It duesnt seem to use the new version of sys

Member

sdroege commented Jul 1, 2018

It duesnt seem to use the new version of sys

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 1, 2018

Member

Ah i looked at Travis. Interesting!

Member

sdroege commented Jul 1, 2018

Ah i looked at Travis. Interesting!

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 1, 2018

Member

@EPashkin @GuillaumeGomez can you restart travis too? This looks weird

Member

sdroege commented Jul 1, 2018

@EPashkin @GuillaumeGomez can you restart travis too? This looks weird

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 1, 2018

Member

I restarted travis.

Member

GuillaumeGomez commented Jul 1, 2018

I restarted travis.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 1, 2018

Member

Restarting won't help, error repeats and it appears after this PR.

Member

EPashkin commented Jul 1, 2018

Restarting won't help, error repeats and it appears after this PR.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 1, 2018

Member

Restarting won't help, error repeats and it appears after this PR.

Travis didn't have this error though. It also makes no sense to me :) To you? Maybe GtkListStore vs GListStore confusion?

Member

sdroege commented Jul 1, 2018

Restarting won't help, error repeats and it appears after this PR.

Travis didn't have this error though. It also makes no sense to me :) To you? Maybe GtkListStore vs GListStore confusion?

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 1, 2018

Member

Travis have same error: https://travis-ci.org/gtk-rs/gio/jobs/398752719#L901
Yes, it ListStoreExtManual traits confusion, we need rename gtk's variants (Ext too) as we do with applicationExt
This PR IMHO ready for merge, as error solved by use gtk::ListStoreExtManual; or fixing gtk

Member

EPashkin commented Jul 1, 2018

Travis have same error: https://travis-ci.org/gtk-rs/gio/jobs/398752719#L901
Yes, it ListStoreExtManual traits confusion, we need rename gtk's variants (Ext too) as we do with applicationExt
This PR IMHO ready for merge, as error solved by use gtk::ListStoreExtManual; or fixing gtk

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 1, 2018

Member

Or we can rename gtk first then restart travis here to be sure

Member

EPashkin commented Jul 1, 2018

Or we can rename gtk first then restart travis here to be sure

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 1, 2018

Member

I'll send gtk rename PR later tonight if nobody is faster

Member

sdroege commented Jul 1, 2018

I'll send gtk rename PR later tonight if nobody is faster

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez
Member

GuillaumeGomez commented Jul 1, 2018

Done in gtk-rs/gtk#670.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Jul 1, 2018

Member

@GuillaumeGomez Please, restart CI here

Member

EPashkin commented Jul 1, 2018

@GuillaumeGomez Please, restart CI here

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 1, 2018

Member

Yes sir!

Member

GuillaumeGomez commented Jul 1, 2018

Yes sir!

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 1, 2018

Member

All green! Thanks for your help :) Tomorrow I'll add the GtkListBox changes for this, and then an example

Member

sdroege commented Jul 1, 2018

All green! Thanks for your help :) Tomorrow I'll add the GtkListBox changes for this, and then an example

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Jul 2, 2018

Member

@GuillaumeGomez good to go now :)

Member

sdroege commented Jul 2, 2018

@GuillaumeGomez good to go now :)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Jul 2, 2018

Member

Thanks!

Member

GuillaumeGomez commented Jul 2, 2018

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit 71ad404 into gtk-rs:master Jul 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
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