Fix #131 and #102 #132
Fix #131 and #102 #132
Conversation
…ndefined behaviour)
|
It looks like Travis caught some sort of problem, but I can't find any message that explains what that problem is. |
src/completion_provider.rs
Outdated
| @@ -0,0 +1,335 @@ | |||
| use crate::CompletionActivation; | |||
There was a problem hiding this comment.
This file should be in src/subclass like in the other crates
src/completion_provider.rs
Outdated
| }; | ||
|
|
||
| let old_ptr = gobject_sys::g_object_get_qdata( | ||
| completion_provider as *mut _, |
There was a problem hiding this comment.
As we discussed this widget can be different for each proposal. So forbidding this seems like a problem, and the lifetime should probably be bound (via the qdata here) to the proposal and not to the provider.
src/completion_provider.rs
Outdated
| gobject_sys::g_object_set_qdata_full( | ||
| completion_provider as *mut _, | ||
| COMPLETION_PROVIDER_INFO_WIDGET.to_glib(), | ||
| ret_ptr as *mut libc::c_void, |
There was a problem hiding this comment.
| ret_ptr as *mut libc::c_void, | |
| ret.to_glib_full() as *mut libc::c_void, |
src/completion_provider.rs
Outdated
| ); | ||
| } | ||
|
|
||
| gobject_sys::g_object_set_qdata_full( |
There was a problem hiding this comment.
This only has to be done if ret is Some
There was a problem hiding this comment.
If when ret is None the qdata isn't set, won't the user's implementation function be able to first return None then return Some? This seems like a violation of returning the same object with each call. I'm committing the correction without fully understanding it yet in the pursuit of finishing this soon, but I may be misunderstanding the correction.
src/completion_provider.rs
Outdated
| gobject_sys::g_object_set_qdata_full( | ||
| completion_provider as *mut _, | ||
| COMPLETION_PROVIDER_GICON_QUARK.to_glib(), | ||
| ret_ptr as *mut libc::c_void, |
There was a problem hiding this comment.
Also here and elsewhere
| ret_ptr as *mut libc::c_void, | |
| ret.to_glib_full() as *mut libc::c_void, |
And the whole thing only if ptr is Some
There was a problem hiding this comment.
Mostly looks good to me, for the subclassing part. I don't really know the sourceview API so the examples are something for @GuillaumeGomez
… get_info_widget, even when the proposal differs
|
With regard to the examples, I can't guarantee that they're free of error, but I'm a lot more confident with the main API than I am with subclassing (and the examples work), so I don't think that a detailed review of that part is vital to merge this PR, in case no one wants to do it. Of course, it is better to have one if someone is willing to. |
|
Somehow rustc didn't catch that error when I was working on my code, and it kept building, but once I cleaned and rebuilt from scratch it did on my computer. I'm not sure how to fix it though. |
See gtk-rs/gio#299 |
|
It's fixed... sorry about that. |
|
No worries. Looks good to me now, the subclassing part. @GuillaumeGomez what do you think about the example? |
|
I'd prefer if the example was added into the examples repository (in the pending branch!). |
|
@GuillaumeGomez My only concern with having it there and only there is that there's no test coverage in this repository then. With the examples where I put them, this repository has all that it needs to test (albeit manually, but that is how things are for GUIs in most cases that I've seen). If you really want, I'll move them from this PR and PR them there, but I think that it's important to address why I put them here in the first place. |
|
@GuillaumeGomez I think having the examples here is ok. As this is not a core GNOME library, it probably shouldn't have the example in the examples repository, especially considering that you were planning to merge all the core libraries into the same repository (which then should also put the examples into the same repository). |
|
This is a good point. @john01dav: Then please add a check for your newly added examples folder (in |
They're build automatically by cargo if you add |
|
It still remains something to be added, no? Or did I miss something? |
|
Yes, just explaining what has to be done :) |
This is my fix for #131, and, to test it, I added some examples to the repository, thus also fixing #102. Presently, it doesn't have any easy-mode interface, but I think that that's acceptable because one of the examples that I provided gives copy/pasteable code that accomplishes the same thing, and the set of functions that GTK's C api exposes is more complete than what I would do for that part.