GNOME Bugzilla – Bug 726408
Allow plugins to set/get the search context in GeditDocument
Last modified: 2014-03-16 15:47:39 UTC
For gedit-file-search plugin (http://oliver.github.com/gedit-file-search/) it would be useful to have access to the search context of a Gedit document. The workflow is: * user searches for a text in some files * list of matching lines is displayed * user doubleclicks on a result line, and the file is opened at the correct line. It would be nice if in the opened document the search term was highlighted now as well (see http://oliver.github.io/gedit-file-search/gedit-file-search-screenshot-g3-4.png for an example). While this would be possible also by creating a separate search context for the file search plugin, this means there could be two highlights (possibly for different search terms). Also, the "Clear Highlight" menu item would only work for one of the search contexts (the file search plugin would have to hook into that menu item, or provide a separate "Clear Highlight" function which would be confusing). In Gedit 3.8 and earlier this functionality was available through the gedit_document_set_search_text() function, which is deprecated now. A proposed solution is to make gedit_document_set_search_context() and gedit_document_get_search_context() public.
> A proposed solution is to make gedit_document_set_search_context() and > gedit_document_get_search_context() public. Yes, I think it's the simplest. Another solution is to add such an API in GtkSourceBuffer, but it's more complicated. GtkSourceBuffer doesn't own the search contexts. On the other hand, GeditDocument owns the search context, so it can destroy the previous one.
Created attachment 272023 [details] [review] make gedit_document_get/set_search_context() public This is a patch to make gedit_document_get_search_context() and gedit_document_set_search_context() public. Not sure if the "transfer" annotations are really correct, though. This implementation works fine for my plugin at least.
Review of attachment 272023 [details] [review]: Thanks for the patch and for testing it in a plugin. A few comments below. ::: gedit/gedit-document.c @@ +2533,3 @@ + * gedit_document_set_search_context: + * @doc: a #GeditDocument + * @search_context: (transfer none) (allow-none): the new #GtkSourceSearchContext There is a g_object_ref() on the @search_context, so the transfer is full. @@ +2535,3 @@ + * @search_context: (transfer none) (allow-none): the new #GtkSourceSearchContext + * + * Sets the new search context (for simple search) for the document. I'll improve the comments later. set_search_context() must be used only if the highlighting is enabled, for example. But it is not only for the simple search. The search and replace dialog use this function too. @@ +2578,3 @@ + * @doc: a #GeditDocument + * + * Return value: (transfer none): the current search context (for simple search) Instead of "Return value", it should be "Returns". I think "Return value" is deprecated by GTK-Doc. (The transfer annotation is correct here.) ::: gedit/gedit-view-frame.c @@ +1262,1 @@ search_context); nitpick: the second argument is not well indented on the parenthesis.
Review of attachment 272023 [details] [review]: ::: gedit/gedit-document.c @@ +2533,3 @@ + * gedit_document_set_search_context: + * @doc: a #GeditDocument + * @search_context: (transfer none) (allow-none): the new #GtkSourceSearchContext If the methods makes a reference to @search_context, then this means the reference is _not_ transfered, i.e. transfer none. Also, that's the default, so the annotation can be removed.
Oops, you are right Jesse. The documentation is misleading: > none: the recipient does not own the value Here the recipient owns the value, since it calls g_object_ref()… https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations
And with a little more thoughts, it is more logical to have (transfer full) for set_search_context() (and thus not calling g_object_ref() inside the function), because the purpose of set_search_context() is to take ownership of the search context. GeditDocument should be the one and only owner of the highlighted search context.
Created attachment 272043 [details] [review] make gedit_document_get/set_search_context() public (try#2) (In reply to comment #3) > Review of attachment 272023 [details] [review]: > > Thanks for the patch and for testing it in a plugin. A few comments below. Thanks for the review; here's an updated patch. > ::: gedit/gedit-document.c > @@ +2533,3 @@ > + * gedit_document_set_search_context: > + * @doc: a #GeditDocument > + * @search_context: (transfer none) (allow-none): the new > #GtkSourceSearchContext > > There is a g_object_ref() on the @search_context, so the transfer is full. For now I've left it at "transfer none" (and removed the annotation). Maybe the change to "transfer full" could be done in a separate patch? I guess that also requires changing the current call sites of the function. > @@ +2535,3 @@ > + * @search_context: (transfer none) (allow-none): the new > #GtkSourceSearchContext > + * > + * Sets the new search context (for simple search) for the document. > > I'll improve the comments later. set_search_context() must be used only if the > highlighting is enabled, for example. Oh - do I have to check for this in the plugin as well? > But it is not only for the simple search. The search and replace dialog use > this function too. Ok, changed comments a bit. > @@ +2578,3 @@ > + * @doc: a #GeditDocument > + * > + * Return value: (transfer none): the current search context (for simple > search) > > Instead of "Return value", it should be "Returns". I think "Return value" is > deprecated by GTK-Doc. Thanks, fixed. > (The transfer annotation is correct here.) > > ::: gedit/gedit-view-frame.c > @@ +1262,1 @@ > search_context); > > nitpick: the second argument is not well indented on the parenthesis. Fixed.
(In reply to comment #7) > For now I've left it at "transfer none" (and removed the annotation). > Maybe the change to "transfer full" could be done in a separate patch? I guess > that also requires changing the current call sites of the function. Yes, maybe a separate patch. And yes, the call sites must be updated too, to remove the g_object_unref() on the search context. > > I'll improve the comments later. set_search_context() must be used only if the > > highlighting is enabled, for example. > > Oh - do I have to check for this in the plugin as well? set_search_context() should simply not be used for background searches. For your plugin it is ok to use set_search_context(), because you want to highlight the search matches.
Review of attachment 272043 [details] [review]: I'll push this commit after the freeze, for the 3.13 development cycle.
I think we can sneak this in before 3.12 and have Oliver's plugin working, from the gedit stability point of view the patch is harmless
(In reply to comment #6) > And with a little more thoughts, it is more logical to have (transfer full) for > set_search_context() (and thus not calling g_object_ref() inside the function), > because the purpose of set_search_context() is to take ownership of the search > context. > > GeditDocument should be the one and only owner of the highlighted search > context. The transfer ownership is not really literally "ownership". It doesn't carry those semantics. You can anyway not avoid that the object is not stored somewhere else as well. What (transfer full) means in gobject is that the callee will take over the "ref" of the object (not the entire object). Making api (transfer full) is bad because you cannot see at the call site that it's going to consume the reference, so you'll always have to be very careful to comment why it appears that you have a leak. Especially if the ref is passed through some other functions it suddenly would be consumed by the API leading to many surprising situations. It's much less error prone to make functions manage their memory/ref ownership. This is also why you will not see much API out there that would transfer objects fully to the callee.
Yes, I agree that we should keep it transfer none and let the caller unref explicitely, this is the more usual convention for non-widget objects
Comment on attachment 272043 [details] [review] make gedit_document_get/set_search_context() public (try#2) OK let's go. I've pushed also another commit to improve the doc. The functions are a bit strange to use. You can't keep your own reference to the search context. And when using get_search_context() you have to check if the returned search context is yours. The problem is that GtkSourceBuffer doesn't own the search contexts. If it was the case, it would have been possible to add a function there. Maybe for GtkSourceView 4 :-) ?