Skip to content
This repository has been archived by the owner on Jun 8, 2021. It is now read-only.

No current support for custom auto-complete #131

Open
john01dav opened this issue May 20, 2020 · 36 comments
Open

No current support for custom auto-complete #131

john01dav opened this issue May 20, 2020 · 36 comments

Comments

@john01dav
Copy link

I'm currently trying to use this crate in a project that needs heavy-duty source code editing. Part of that is a basic auto-complete implementation. It needs to be a bit more context-sensitive than the current words-based implementation, but not by much. I've decided what algorithms I want to use to do the actual completion, but getting the library to call my code to do the completion and present it to the user seems to currently be impossible. As such, this is a feature request to add the necessary API hooks to make that happen.

While only adding the necessary traits as described in the linked issue is one way of adding this support, ideally, a subclass or interface implementation would not be needed to use these features, as those techniques are cumbersome with GObject in general, but, it would seem, especially so in Rust since one must not only deal with GObject (which is as cumbersome as any C API), but must also deal with the intricacies of the binding at the same time.

As such, I propose a different interface where there is a simple trait that the user can implement, and then pass a dyn object to the Rust bindings to act in a similar way as the GObject interface, but where it is significantly easier to use because Rust has built-in support for implementing traits. This could be implemented via adding those traits, then creating a Rust implementation that has a Box<dyn TheTraitThatIJustMentioned> as a member.

While I am willing to attempt this addition myself and PR it, I don't think that I'm the best person to do that. I'm currently new to Rust, GTK/GObject, and these bindings (I've never even used Rust's FFI support), so if I were to do it it would likely have many errors, if I could get it working at all. I do have significant experience with other tools though, such as C without GObject, C++, and some minimal assembly (I once had to write some assembly to call C functions and be called as a C function, without the aid of a compiler, so I am familiar with how FFI probably works despite the fact that I haven't looked into it.).

@john01dav
Copy link
Author

@GuillaumeGomez Since you won't be doing this, can I get your opinion on what I said about using a Rust trait instead of normal interface implementations before I attempt to PR this? Also, would such a PR be welcome?

@GuillaumeGomez
Copy link
Member

If the API is easy to use in the end for the users, then I'm fine with it. We currently generate most of the API with gir, which allows us to have a low maintenance. With all this in mind, this library doesn't change much (even less since the version 4 has been released) so I guess it's ok to have more manual code.

So if you have the motivation to do it, then please do so. It's very welcome.

@john01dav
Copy link
Author

john01dav commented May 20, 2020

I'm going to attempt it.... I'm not sure how well it will turn out, but I'm attempting it.

Also let me know if you want to work on adding the subclassing pieces for this specific interface to the sourceview bindings. Then I can give you links to good code examples for what has to be done there in this case.

@sdroege Could I get these links? The most basic example that is currently here would be best, so that I just see what's important.

@sdroege
Copy link
Member

sdroege commented May 20, 2020

So, CompletionProvider fortunately exists already for the "consume" side of the bindings, and I hope it's complete enough: https://github.com/gtk-rs/sourceview/blob/master/src/auto/completion_provider.rs

Now this is an interface. So you would have to add something like https://github.com/gtk-rs/gio/blob/master/src/subclass/seekable.rs . Instead of the virtual methods of Seekable, you would add the ones from CompletionProvider there with the correct behaviour/semantics. https://developer.gnome.org/gtksourceview/stable/GtkSourceCompletionProvider.html#GtkSourceCompletionProviderIface gives a list of all of them. That fortunately doesn't look like it's a special or complicated case. So this comes in 4 parts: 1) an Impl trait with the virtual methods, 2) an IsImplementable impl, 3) the C/Rust FFI translation functions

At that point you would then have enough bindings to actually implement some glib::Object subclass that implements this interface. An example would here be the ReadInputStream in gio, which implements the Seekable interface: https://github.com/gtk-rs/gio/blob/master/src/read_input_stream.rs. The important parts here are the two top impl blocks and the SeekableImpl block.

@sdroege
Copy link
Member

sdroege commented May 20, 2020

The important parts here are the two top impl blocks and the SeekableImpl block.

Below that with glib_wrapper! and all that is not absolutely required and only exists to provide nice high-level Rust bindings for the new subclass. Only the part inside the imp module there is required. Without all that other stuff you would also just create an instance via glib::Object::new() like is done there.

@john01dav
Copy link
Author

I've been trying to make heads or tails out of GObject enough to be able to write something like this, and I'm not getting anywhere. Is there some documentation that exists that will clearly spell out how this is supposed to work? Even if it doesn't have the complete story, I know nothing right now aside from how to use the existing bindings. I'm seeing a jumble of macros, traits, structs, etc. when I look at the existing source, with no clear pattern to them. When I think that I've figured something out, I invariably find something else a few hours later that contradicts it. I don't think that I can do this without at least some documentation — I have literally no idea how GObject works, yet I'm trying to implement bindings in another language! It's mildly absurd, and I'll need some help if it is going to happen.

@sdroege
Copy link
Member

sdroege commented May 21, 2020

Maybe https://developer.gnome.org/gobject/2.62/pt02.html can help? That's the C part of the whole story.

From looking at the pieces of the gio::Seekable interface bindings, which parts are causing you trouble? Do my explanations in gtk-rs/examples#279 help you in any way?

I agree that we need to put something into a more complete documentation here to explain how all the pieces fit together but I don't really have the time to do that right now, it's going to be a day or two of work. So feel free to ask questions for now.

@john01dav
Copy link
Author

So feel free to ask questions for now. ... which parts are causing you trouble?

I think that probably the best way to help clarify this is if I put some questions here as I come across them — that should allow me to get an authoritative answer in a reasonable amount of time (as opposed to hours of digging through source to infer something that's probably wrong, which is what I've been doing).

The first issue that I'm having relates to "an Impl trait with the virtual methods." I created a trait that looks like this:

use glib::subclass::prelude::*;

pub trait CompletionImpl: ObjectImpl + Send + 'static {
    //...
}

and, I presume, that the methods from here should be mirrored into this trait, but it's not clear what the signatures should be. For example, take the C method get_name. The following all seem like valid Rust equivalents to me (among others):

fn get_name(&self) -> glib::char::Char; //Why would this type exist if it isn't used at the low level like this? The same pointer stuff below applies here to create 2 additional possibilities.
fn get_name(&self) -> char; //The seekable example seems to use native Rust types/
fn get_name(&self) -> *char; //Maybe Rust's unsafe pointers should be used?
fn get_name(&self) -> *mut char; //The pointer is mutable in C, maybe it is here too?

Of course, only one of them is likely correct, but I have no basis from which to make that determination (for this function, or for others). What are the rules for rewriting these signatures?

I know that it seems like I haven't gotten very far — that's because what I've done so far as mostly consisted of reading things and trying to make heads or tails out of what's going on. My learning style usually requires me to first use some well-written documentation to get a good conceptual foundation, and only then go on to writing code (or doing whatever it is that I'm trying to learn). For obvious reasons though, that didn't work very well in this case, so I'm now trying something different.

@sdroege
Copy link
Member

sdroege commented May 21, 2020

Of course, only one of them is likely correct, but I have no basis from which to make that determination (for this function, or for others). What are the rules for rewriting these signatures?

You would use the corresponding proper Rust types. So for example get_name() in C takes the instance as argument and returns a string (i.e. gchar *). In Rust that would be function taking &self as your guesses did, and you would return a String or Option<String> depending whether the C function is allowed to return NULL (that would be in the docs somewhere, or in the worst case you would have to read the code. My understanding is that it must not be NULL but various other functions there are allowed to return NULL). That would already be almost correct (and in fact would work). However we have a custom string type in the glib crate that uses the GLib allocator to prevent some more copies (glib::GString). You could use String but glib::String would be a bit more efficient here.

Now the function doesn't return a const gchar* but non-const one. That hints at us that the ownership of the string is returned to the caller. Similarly the documentation of the actual function confirms that (the part about g_free()). So for conversion from a Rust glib::GString (or String) to a gchar * you would use glib::translate::ToGlibPtr::to_glib_full() (i.e. your_string.to_glib_full() after the trait is in scope).

I know that it seems like I haven't gotten very far

No worries about that :) This is not exactly easy stuff. Personally I would tackle this by reading other code in the bindings that does similar things and try to understand that, probably having problems understanding how things fit together and then ask based on the existing code. But let's just go through it one by one here if you prefer that.

@sdroege
Copy link
Member

sdroege commented May 21, 2020

I forgot to actually write the whole signature in the end, sorry. That would be fn get_name(&self, obj: &CompletionProvider) -> glib::GString. In addition to &self we also always provide the GObject to the functions as second parameter. The first is your implementation of it (think: private state, the collection of private member variables if this was C++ or Java), the second is the public part.

@john01dav
Copy link
Author

john01dav commented May 21, 2020

Okay... what you've said has been very helpful. Thank you :) I think that I have a correct Impl trait. Is there any way to have the compiler check it? I'd rather have that happen at each step, to avoid the situation where there's some issue somewhere, and things mysteriously don't work (This usually happens when I work with C APIs, after implementing a big chunk of stuff without the ability to test each step. OpenGl has been the worst offender.).

Here's what I have:

use crate::CompletionActivation;
use crate::CompletionContext;
use crate::CompletionInfo;
use crate::CompletionProposal;
use crate::CompletionProvider;
use glib::subclass::prelude::*;

pub trait CompletionImpl: ObjectImpl + Send + 'static {
    fn get_name(&self, obj: &CompletionProvider) -> glib::GString;
    fn get_icon(&self, obj: &CompletionProvider) -> Option<gdk_pixbuf::Pixbuf>;
    fn get_icon_name(&self, obj: &CompletionProvider) -> Option<glib::GString>;
    fn get_gicon(&self, obg: &CompletionProvider) -> Option<gio_sys::GIcon>;
    fn populate(&self, obg: &CompletionProvider, context: &CompletionContext);
    fn get_activation(&self, obg: &CompletionProvider) -> CompletionActivation;
    fn provider_match(&self, obg: &CompletionProvider, context: &CompletionContext) -> bool;
    fn get_info_widget(
        &self,
        obg: &CompletionProvider,
        proposal: &CompletionProposal,
    ) -> Option<gtk::Widget>;
    fn update_info(
        &self,
        obg: &CompletionProvider,
        proposal: &CompletionProposal,
        info: &CompletionInfo,
    );
    fn get_start_iter(
        &self,
        obg: &CompletionProvider,
        context: &CompletionContext,
        proposal: &CompletionProposal,
        iter: &gtk::TextIter,
    ) -> bool;
    fn activate_proposal(
        &self,
        obg: &CompletionProvider,
        proposal: &CompletionProposal,
        iter: &gtk::TextIter,
    ) -> bool;
    fn get_interactive_delay(&self, obj: &CompletionProvider) -> i32;
    fn get_priority(&self, obg: &CompletionProvider) -> i32;
}

@sdroege
Copy link
Member

sdroege commented May 21, 2020

I think that I have a correct Impl trait. Is there any way to have the compiler check it

Not really. If it compiles then there's no mistake the compiler could catch. The trait looks generally good to me, not obvious problems.

When implementing the other pieces you might notice that things don't fit together in a way the compiler likes it (or the implementation doesn't make sense). But that would be exactly the next step. You'd add one by one the unsafe extern "C" functions for each of them. After each one you can check if it compiles. And in the very end you can add the IsImplementable trait implementation to put it all together.

@john01dav
Copy link
Author

john01dav commented May 21, 2020

When moving on to implementing the glue functions (the functions that the C code can call, which then delegate to the Rust implementation, such as this one — I'm not sure what the standard name is for these, so I must clearly define the terms that I'm using.), once again, I'm having trouble with determining the method signature. Specifically, I am working on the get_name() function, and its return type in C is gchar. I expected there to be a similar type in glib_sys, since that's where the seeker example that you linked gets its primitive types from (an example with gboolean). I was then expecting to have a pointer to the type, making the return type *glib_sys::gchar (or something similar) and the method signature in its entirety being

unsafe fn get_name_glue<T: ObjectSubclass + CompletionImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider) -> *glib_sys::gchar;

, but, it would appear that no such type exists in glib_sys, even under other names.

@sdroege
Copy link
Member

sdroege commented May 21, 2020

It's std::os::raw::c_char / libc::c_char. The correct signature would be

unsafe fn get_name_glue<T: ObjectSubclass>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider) -> *mut c_char;

Also for consistency your trait should be called CompletionProviderImpl.

@sdroege
Copy link
Member

sdroege commented May 21, 2020

https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/blob/master/gstreamer/src/subclass/uri_handler.rs#L75 would be an example of an interface that has functions returning strings.

@john01dav
Copy link
Author

john01dav commented May 23, 2020

So, in order to get it to build get_name_glue and use that function in my IsImplementable implementation, I had to make some changes. Specifically, I had to make it return *const libc::c_char instead of *mut libc::c_char (without this change, I get type errors in my IsImplementable implementation, and I had to add CompletionProviderImpl to the bounds on T for get_name_glue (without this change, the function get_name doesn't exist on T). These both differ from the signature that you gave. Am I missing something, or is there an error in your signature? It looks like it's an error, but you know this a lot better than I do, so I want to make sure that I'm not missing something. It also might not be a mistake because the seekable example doesn't have any such additional type bound on its glue functions. In either case, I'd like to know what's going on.

Here's what I have, after changing the signature.

unsafe impl<T: ObjectSubclass + CompletionProviderImpl> IsImplementable<T> for CompletionProvider {
    unsafe extern "C" fn interface_init(iface: glib::glib_sys::gpointer, _iface_data: glib::glib_sys::gpointer) {
        let iface = &mut *(iface as *mut gtk_source_sys::GtkSourceCompletionProviderIface);
        iface.get_name = Some(completion_provider_get_name::<T>);
    }
}

unsafe extern "C" fn completion_provider_get_name<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider) -> *const libc::c_char{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.get_name(&from_glib_borrow(completion_provider)).to_glib_full()
}

@sdroege
Copy link
Member

sdroege commented May 23, 2020

So, in order to get it to build get_name_glue and use that function in my IsImplementable implementation, I had to make some changes. Specifically, I had to make it return *const libc::c_char instead of *mut libc::c_char (without this change, I get type errors in my IsImplementable implementation,

That seems like a weird inconsistency between the C struct definition in the docs and in the sourceview -sys crate. const is correct according to the -sys crate, not sure what is actually correct.

and I had to add CompletionProviderImpl to the bounds on T for get_name_glue (without this change, the function get_name doesn't exist on T).

This is correct, yes, see below

It also might not be a mistake because the seekable example doesn't have any such additional type bound on its glue functions.

It does, it has a where T: SeekableImpl clause :) It's just not in the <> part of the function. Makes no difference here how you do it.

Here's what I have, after changing the signature.

Looks good to me :)

@john01dav
Copy link
Author

I'm having some trouble with the get_icon function. This one is supposed to return a possibly-null pointer to a struct. It seems like I should use match to handle the null case, but I'm at a loss for how to convert the Rust struct into a C struct. The normal to_glib() family of functions isn't useful here because ToGlibPtr isn't implemented, either on the Option of this type or on the type itself, it seems. I looked through the documentation for gdk_pixbuf::Pixbuf, but I don't see anything useful for this there.

unsafe extern "C" fn completion_provider_get_icon<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider) -> *mut gdk_pixbuf_sys::GdkPixbuf{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    match imp.get_icon(&from_glib_borrow(completion_provider)) {
        Some(pixbuf) => pixbuf.to_glib(), //method not found
        None =>  std::ptr::null_mut()
    }
    imp.get_icon(&from_glib_borrow(completion_provider)).to_glib() //method not found here either, in the real code I only want one of something like this, but I'm showing both here to demonstrate both things that I know to try
}

@sdroege
Copy link
Member

sdroege commented May 24, 2020

This should be to_glib_full() (or to_glib_none().0 plus hacks, depends on the C API). That is also implemented on Option<Pixbuf> and will handle the NULL-case just fine.

@john01dav
Copy link
Author

It seems like to_glib_full() is wrong because the C documentation for get_icon() says that ownership isn't transferred to the caller. Specifically, it says "transfer_none" by the return type, and, upon mousing over on this value, it says "Don't free data after the code is done.". Looking at ToGlibPtr's definition, it says that transfer none corresponds to to_glib_none, which, I assume, implies that I should use the to_glib_none().0 option that you gave.

What hacks are needed to make this work? I tried to find some examples in the code base, but I can't find anything that looks like an interface as I'm trying to do, just calls like this one, where the function is being used to call into GTK's C code, not to allow GTK's C code to call into Rust. There also don't seem to be any hacks going on in the examples that I can find. For now, I'm trying it without anything special (just calling the function and accessing .0) to allow me to continue working, but since you said that there should be hacks there, it definitely seems like the right thing to make sure that I'm doing it properly.

Also, thank you so much for all your help! I feel like I'm finally getting somewhere with understanding this system :). Sorry if I'm asking too many questions, I'll try to reduce it a bit.

@john01dav
Copy link
Author

With the assumption that to_glib_none().0 is correct, I was able to resolve the rest of the issues that I had and create what seems like a correct set of glue functions and IsImplementable implementation. I'll be moving on later today to attempt to use this interface, but a quick sanity check would be very much appreciated.

use crate::CompletionActivation;
use crate::CompletionContext;
use crate::CompletionInfo;
use crate::CompletionProposal;
use crate::CompletionProvider;
use glib::translate::*;
use glib::subclass::prelude::*;

pub trait CompletionProviderImpl: ObjectImpl + Send + 'static {
    fn get_name(&self, obj: &CompletionProvider) -> glib::GString;
    fn get_icon(&self, obj: &CompletionProvider) -> Option<gdk_pixbuf::Pixbuf>;
    fn get_icon_name(&self, obj: &CompletionProvider) -> Option<glib::GString>;
    fn get_gicon(&self, obg: &CompletionProvider) -> Option<gio::Icon>;
    fn populate(&self, obg: &CompletionProvider, context: &CompletionContext);
    fn get_activation(&self, obg: &CompletionProvider) -> CompletionActivation;
    fn provide_match(&self, obg: &CompletionProvider, context: &CompletionContext) -> bool;
    fn get_info_widget(
        &self,
        obg: &CompletionProvider,
        proposal: &CompletionProposal,
    ) -> Option<gtk::Widget>;
    fn update_info(
        &self,
        obg: &CompletionProvider,
        proposal: &CompletionProposal,
        info: &CompletionInfo,
    );
    fn get_start_iter(
        &self,
        obg: &CompletionProvider,
        context: &CompletionContext,
        proposal: &CompletionProposal,
        iter: &gtk::TextIter,
    ) -> bool;
    fn activate_proposal(
        &self,
        obg: &CompletionProvider,
        proposal: &CompletionProposal,
        iter: &gtk::TextIter,
    ) -> bool;
    fn get_interactive_delay(&self, obj: &CompletionProvider) -> i32;
    fn get_priority(&self, obg: &CompletionProvider) -> i32;
}

unsafe impl<T: ObjectSubclass + CompletionProviderImpl> IsImplementable<T> for CompletionProvider {
    unsafe extern "C" fn interface_init(iface: glib::glib_sys::gpointer, _iface_data: glib::glib_sys::gpointer) {
        let iface = &mut *(iface as *mut gtk_source_sys::GtkSourceCompletionProviderIface);
        iface.get_name = Some(completion_provider_get_name::<T>);
        iface.get_icon = Some(completion_provider_get_icon::<T>);
        iface.get_icon_name = Some(completion_provider_get_icon_name::<T>);
        iface.get_gicon = Some(completion_provider_get_gicon::<T>);
        iface.populate = Some(completion_provider_populate::<T>);
        iface.get_activation = Some(completion_provider_get_activation::<T>);
        iface.match_ = Some(completion_provider_provide_match::<T>);
        iface.get_info_widget = Some(completion_provider_get_info_widget::<T>);
        iface.update_info = Some(completion_provider_update_info::<T>);
        iface.get_start_iter = Some(completion_provider_get_start_iter::<T>);
        iface.activate_proposal = Some(completion_provider_activate_proposal::<T>);
        iface.get_interactive_delay = Some(completion_provider_get_interactive_delay::<T>);
        iface.get_priority = Some(completion_provider_get_priority::<T>);
    }
}

unsafe extern "C" fn completion_provider_get_name<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider) -> *const libc::c_char{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.get_name(&from_glib_borrow(completion_provider)).to_glib_full()
}

unsafe extern "C" fn completion_provider_get_icon<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider) -> *mut gdk_pixbuf_sys::GdkPixbuf{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.get_icon(&from_glib_borrow(completion_provider)).to_glib_none().0 //method not found here either, in the real code I only want one of something like this, but I'm showing both here to demonstrate both things that I know to try
}

unsafe extern "C" fn completion_provider_get_icon_name<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider) -> *const libc::c_char{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.get_icon_name(&from_glib_borrow(completion_provider)).to_glib_none().0
}

unsafe extern "C" fn completion_provider_get_gicon<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider) -> *mut gio_sys::GIcon{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.get_gicon(&from_glib_borrow(completion_provider)).to_glib_none().0
}

unsafe extern "C" fn completion_provider_populate<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider, context: *mut gtk_source_sys::GtkSourceCompletionContext){
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.populate(&from_glib_borrow(completion_provider), &from_glib_borrow(context))
}

unsafe extern "C" fn completion_provider_get_activation<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider) -> gtk_source_sys::GtkSourceCompletionActivation{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.get_activation(&from_glib_borrow(completion_provider)).to_glib()
}

unsafe extern "C" fn completion_provider_provide_match<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider, context: *mut gtk_source_sys::GtkSourceCompletionContext) -> glib_sys::gboolean{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.provide_match(&from_glib_borrow(completion_provider), &from_glib_borrow(context)).to_glib()
}

unsafe extern "C" fn completion_provider_get_info_widget<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider, proposal: *mut gtk_source_sys::GtkSourceCompletionProposal) -> *mut gtk_sys::GtkWidget{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.get_info_widget(&from_glib_borrow(completion_provider), &from_glib_borrow(proposal)).to_glib_none().0
}

unsafe extern "C" fn completion_provider_update_info<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider, proposal: *mut gtk_source_sys::GtkSourceCompletionProposal, completion_info: *mut gtk_source_sys::GtkSourceCompletionInfo) {
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.update_info(&from_glib_borrow(completion_provider), &from_glib_borrow(proposal), &from_glib_borrow(completion_info))
}

unsafe extern "C" fn completion_provider_get_start_iter<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider, context: *mut gtk_source_sys::GtkSourceCompletionContext, proposal: *mut gtk_source_sys::GtkSourceCompletionProposal, iter: *mut gtk_sys::GtkTextIter) -> glib_sys::gboolean{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.get_start_iter(&from_glib_borrow(completion_provider), &from_glib_borrow(context), &from_glib_borrow(proposal), &from_glib_borrow(iter)).to_glib()
}

unsafe extern "C" fn completion_provider_activate_proposal<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider, proposal: *mut gtk_source_sys::GtkSourceCompletionProposal, iter: *mut gtk_sys::GtkTextIter) -> glib_sys::gboolean{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.activate_proposal(&from_glib_borrow(completion_provider), &from_glib_borrow(proposal), &from_glib_borrow(iter)).to_glib()
}

unsafe extern "C" fn completion_provider_get_interactive_delay<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider) -> libc::c_int{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.get_interactive_delay(&from_glib_borrow(completion_provider))
}

unsafe extern "C" fn completion_provider_get_priority<T: ObjectSubclass + CompletionProviderImpl>(completion_provider: *mut gtk_source_sys::GtkSourceCompletionProvider) -> libc::c_int{
    let instance = &*(completion_provider as *mut T::Instance);
    let imp = instance.get_impl();
    imp.get_priority(&from_glib_borrow(completion_provider))
}

@sdroege
Copy link
Member

sdroege commented May 24, 2020

which, I assume, implies that I should use the to_glib_none().0 option that you gave.

Correct based on what you found :)

What hacks are needed to make this work?

It's unsafe to return something with transfer none as nothing guarantees you that the implementation is actually keeping it around. It might be that it returned the only reference, and that would then be dropped at the end of your trampoline function.

See this here for an example how to ensure this is all safe: https://github.com/gtk-rs/gio/blob/master/src/subclass/io_stream.rs#L127

where the function is being used to call into GTK's C code

There it is not a problem because the Rust code keeps the reference alive. It's the difference between borrowing a parameter to a function or having a function return a reference.

Sorry if I'm asking too many questions, I'll try to reduce it a bit.

No worries, you're not asking that many questions actually :)

I'll be moving on later today to attempt to use this interface, but a quick sanity check would be very much appreciated.

You only would have to implement the functions that are mandatory, and the optional ones that you need yourself. But even better if you implemented all :)

See above for the to_glib_none().0 hack that needs to be applied. Otherwise looks good but I didn't check the ownership transfer everywhere. I assume you checked that against the docs like with the Pixbuf icon, then it should be correct :)

@john01dav
Copy link
Author

john01dav commented May 25, 2020

You mentioned that I don't need to implement anything for non-mandatory functions. This interface seems to have a few such functions. Two of them, get_info_widget and update_info, are something that most subclasses probably won't want to implement (it seems like a fair bit of work, but where the default is completely reasonable in most cases). If I have them in my Impl trait, however, rustc will force such an implementation to exist. Does this mean that I shouldn't support these functions?

@john01dav
Copy link
Author

Also, in your examples, the interfaces return values aren't nullable. But, in the interface that I want to use, they are. Thus, my Impl trait returns Options, but as_ptr() isn't implemented for the Option types. Should I do manual match-based handling of this, and return std::ptr::null_mut() as here, or is there some better way of handling this (I'm not even sure if that sort of matching would work).

@sdroege
Copy link
Member

sdroege commented May 25, 2020

Does this mean that I shouldn't support these functions?

You could provide a default implementation for them that behaves correctly

Should I do manual match-based handling of this

Something like that, yes. Or option.map(|v| v.as_ptr()).unwrap_or(ptr::null_mut()) :)

@john01dav
Copy link
Author

john01dav commented May 25, 2020

You could provide a default implementation for them that behaves correctly'

Is there some easy way to have it forward to the default C implementation in this case?

@john01dav
Copy link
Author

I've added in the logic for making these functions safe. The complete file is here (it's getting long enough to be unwieldy to pass around in comments).

@john01dav
Copy link
Author

john01dav commented May 25, 2020

I've gotten far enough along to attempt to make a subclass. I have this so far, but rustc complains about a variety of traits not being implemented on () (when I implement one, via unimplemented! temporarily, just to figure out the types), it complains of another. In the example that you linked, read_input_stream, I don't see any implementation of the traits that rustc is complaining about here, hence my confusion.

What I have:

struct CustomAutocomplete;

impl ObjectSubclass for CustomAutocomplete{
    const NAME: &'static str = "CustomAutocomplete";
    type ParentType = sourceview::CompletionProvider;
    type Instance = glib::subclass::simple::InstanceStruct<Self>;
    type Class = glib::subclass::simple::ClassStruct<Self>;

    glib::glib_object_subclass!();

    fn type_init(type_: &mut glib::subclass::InitializingType<Self>){
        type_.add_interface::<CompletionProvider>();
    }

    fn new() -> Self{
        CustomAutocomplete
    }

}

impl ObjectImpl for CustomAutocomplete{
    glib::glib_object_impl!();
}

impl sourceview::CompletionProviderImpl for CustomAutocomplete{
    fn get_name(&self, obj: &CompletionProvider) -> GString {
        unimplemented!()
    }

    fn get_icon(&self, obj: &CompletionProvider) -> Option<gdk_pixbuf::Pixbuf> {
        unimplemented!()
    }

    fn get_icon_name(&self, obj: &CompletionProvider) -> Option<GString> {
        unimplemented!()
    }

    fn get_gicon(&self, obg: &CompletionProvider) -> Option<gio::Icon> {
        unimplemented!()
    }

    fn populate(&self, obg: &CompletionProvider, context: &sourceview::CompletionContext) {
        unimplemented!()
    }

    fn get_activation(&self, obg: &CompletionProvider) -> sourceview::CompletionActivation {
        unimplemented!()
    }

    fn provide_match(&self, obg: &CompletionProvider, context: &sourceview::CompletionContext) -> bool {
        unimplemented!()
    }

    fn get_info_widget(&self, obg: &CompletionProvider, proposal: &sourceview::CompletionProposal) -> Option<gtk::Widget> {
        unimplemented!()
    }

    fn update_info(&self, obg: &CompletionProvider, proposal: &sourceview::CompletionProposal, info: &CompletionInfo) {
        unimplemented!()
    }

    fn get_start_iter(&self, obg: &CompletionProvider, context: &sourceview::CompletionContext, proposal: &sourceview::CompletionProposal, iter: &gtk::TextIter) -> bool {
        unimplemented!()
    }

    fn activate_proposal(&self, obg: &CompletionProvider, proposal: &sourceview::CompletionProposal, iter: &gtk::TextIter) -> bool {
        unimplemented!()
    }

    fn get_interactive_delay(&self, obj: &CompletionProvider) -> i32 {
        unimplemented!()
    }

    fn get_priority(&self, obg: &CompletionProvider) -> i32 {
        unimplemented!()
    }
}

Rustc's error:

error[E0277]: the trait bound `(): gio::subclass::IsSubclassable<CustomAutocomplete>` is not satisfied
   --> examples/custom_autocompletion_subclass.rs:46:5
    |
46  |     glib::glib_object_subclass!();
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `gio::subclass::IsSubclassable<CustomAutocomplete>` is not implemented for `()`
    | 
   ::: /home/david/.cargo/git/checkouts/glib-928cf7b282977403/d322eaf/src/subclass/types.rs:460:71
    |
460 |     <<T as ObjectSubclass>::ParentType as ObjectType>::RustClassType: IsSubclassable<T>,
    |                                                                       ----------------- required by this bound in `glib::subclass::register_type`
    |
    = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

@sdroege
Copy link
Member

sdroege commented May 26, 2020

Is there some easy way to have it forward to the default C implementation in this case?

If there is one, sure. Is there? You can find an example of that here.

type ParentType = sourceview::CompletionProvider;

It's an interface, you can't use that as parent type. You can use glib::Object here for example. Then this should compile.

@john01dav
Copy link
Author

After changing the parent type, that specific error goes away, but a new one comes up. sourceview::CompletionExt::add_provider expects a type that implements IsA<CompletionProvider> as its argument. I don't see this explicitly implemented in your read_input_stream example, and when I try to implement it myself, it simply creates more errors. Additionally, the traits glib::StaticType and glib::IsA<glib::Object> have a similar situation, where they seem to be required, but aren't explicitely implemented in your examples and aren't implemented at all in my code.

Here's rustc's full error message:

error[E0277]: the trait bound `CustomAutocomplete: glib::IsA<sourceview::CompletionProvider>` is not satisfied
  --> examples/custom_autocompletion_subclass.rs:29:27
   |
29 |             .add_provider(&completion);
   |                           ^^^^^^^^^^^ the trait `glib::IsA<sourceview::CompletionProvider>` is not implemented for `CustomAutocomplete`

error[E0599]: no function or associated item named `static_type` found for struct `CustomAutocomplete` in the current scope
  --> examples/custom_autocompletion_subclass.rs:41:33
   |
37 | struct CustomAutocomplete;
   | --------------------------
   | |
   | function or associated item `static_type` not found for this
   | doesn't satisfy `CustomAutocomplete: glib::StaticType`
...
41 |         glib::Object::new(Self::static_type(), &[])
   |                                 ^^^^^^^^^^^ function or associated item not found in `CustomAutocomplete`
   |
   = note: the method `static_type` exists but the following trait bounds were not satisfied:
           `CustomAutocomplete: glib::StaticType`
           which is required by `&CustomAutocomplete: glib::StaticType`
           `CustomAutocomplete: glib::StaticType`
           which is required by `&mut CustomAutocomplete: glib::StaticType`
   = help: items from traits can only be used if the trait is implemented and in scope
   = note: the following trait defines an item `static_type`, perhaps you need to implement it:
           candidate #1: `glib::StaticType`

error[E0277]: the trait bound `CustomAutocomplete: glib::IsA<glib::Object>` is not satisfied
  --> examples/custom_autocompletion_subclass.rs:43:14
   |
43 |             .downcast()
   |              ^^^^^^^^ the trait `glib::IsA<glib::Object>` is not implemented for `CustomAutocomplete`
   |
   = note: required because of the requirements on the impl of `glib::object::CanDowncast<CustomAutocomplete>` for `glib::Object`

error: aborting due to 3 previous errors

Some errors have detailed explanations: E0277, E0599.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `sourceview`.

To learn more, run the command again with --verbose.

@sdroege
Copy link
Member

sdroege commented May 26, 2020

expects a type that implements IsA<CompletionProvider> as its argument. I don't see this explicitly implemented in your read_input_stream example, and when I try to implement it myself, it simply creates more errors. Additionally, the traits glib::StaticType and glib::IsA<glib::Object> have a similar situation

You don't implement any of these traits. This is exactly what the glib_wrapper! macro is for, which you'll also see in the read_input_stream.rs. All you did so far is the "private", implementation part of your subclass. glib_wrapper! would now declare the "public" part of it, the part that works like any other sourceview::AutocompletionProvider in public API.

@john01dav
Copy link
Author

So, I have a working implementation, but I need to flout Rust's safety rules in order to make it work, and the C APIs pitch a fit of warnings in the console log when doing so, which definitely needs to be fixed. It seems like the issue is with the get_info_widget function on the interface. As you advised, I'm re-implementing the default functionality in Rust. My implementation looks like this:

fn get_info_widget(
    &self,
    obg: &CompletionProvider,
    proposal: &sourceview::CompletionProposal,
) -> Option<gtk::Widget> {
    let label = gtk::Label::new(Some(match proposal.get_label() {
        Some(ref label_text_gstring) => label_text_gstring.as_str(),
        None => "",
    }));

    Some(label.upcast::<gtk::Widget>())
}

Clearly, however, this implementation will give a new gtk::Label instance each time that get_info_widget is called, but get_info_widget is transfer_none, which I'm pretty sure means that it needs to return the same gtk::Label instance each time, and my code for the trampoline function agrees (it panics when a different gtk::Label is returned, even if the proposal parameter is different). My educated guess about needing the same object to be returned doesn't seem to be just wrong, since there's console spam (see below), but the console spam seems to have nothing to do with the duplication issue. Can you clarify on these issues?

The console spam when I comment out the safety checks in the trampoline function:

(custom_autocompletion_subclass:8308): GLib-GObject-CRITICAL **: 21:52:53.883: g_object_unref: assertion 'G_IS_OBJECT (object)' failed

(custom_autocompletion_subclass:8308): Gtk-CRITICAL **: 21:52:53.883: gtk_container_add: assertion 'GTK_IS_WIDGET (widget)' failed

(custom_autocompletion_subclass:8308): GLib-GObject-CRITICAL **: 21:52:53.921: g_object_unref: assertion 'G_IS_OBJECT (object)' failed

(custom_autocompletion_subclass:8308): Gtk-CRITICAL **: 21:52:53.921: gtk_container_add: assertion 'GTK_IS_WIDGET (widget)' failed

@sdroege
Copy link
Member

sdroege commented May 28, 2020

but I need to flout Rust's safety rules in order to make it work

In what sense?

which I'm pretty sure means that it needs to return the same gtk::Label instance each time

Yes, it must be the same every time, it must stay alive for as long as your instance is alive, and your trampoline function must ensure that.

Can you clarify on these issues?

I'd need a runnable example that reproduces this case, then I can take a look. This looks to me however like the returned widget from that function is not ensured to be kept alive.

@john01dav
Copy link
Author

I put the working example on my fork here. To run it, clone the entire repository then run cargo run --example custom_autocompletion_subclass¹. The flout of rust's safety rules takes place here. It's unclear to me how the same widget could be returned each time from this member function, as it is necessary that the label has different contents depending on which completion proposal is being shown. I tried to figure out what's going on the C API, and it seems to just return NULL. When I return None from my Rust implementation it does work fine, but having custom widgets is a valid use case that I'd like to have working properly.

@sdroege
Copy link
Member

sdroege commented May 28, 2020

That's even more complicated then. You somehow need to ensure that the widget stays alive as long as the completion proposal it belongs to does. For this you'd need to go for example via g_object_add_weak_notify() to get notified when the proposal is destroyed.

So your problem is most likely that you don't ensure that it stays around at all. The warnings will probably disappear if you use to_glib_full() instead of to_glib_none().0 when returning? You'll just leak all the widgets then.

@john01dav
Copy link
Author

john01dav commented May 28, 2020 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants