Created attachment 270623 [details] [review]
untested patch

I was thinking something along these lines could do the trick, but I'd really like some feedback from someone more familiar with the message bus.

This interface requires sending a sync message to get back the extension object, is this a good idea?


the consumer of the api would do something like:

        action = Gio.SimpleAction(name="blah")
        self.window.add_action(action)
        msg = self.bus.send_sync('/plugins/filebrowser', 'extend_context_menu')
        self.menu_ext = msg.props.extension
        item = Gio.MenuItem.new(_("Blah Blah"), "win.blah")
        self.menu_ext.append_menu_item(item)

Created attachment 270698 [details] [review]
update for previous patch (move extension-section)

The proposed patch basically works for me. The extension-section in the menu needs to be moved to the top-level menu, though (see attached patch).

This is the code I used to add/remove the menu item:

    def _addFileBrowserMenuItem (self):
        action = Gio.SimpleAction(name="gedit-file-search-filebrowser")
        action.connect("activate", lambda action, parameter: self.onFbMenuItemActivate(action))
        self.window.add_action(action)
        
        msg = self._bus.send_sync("/plugins/filebrowser", "extend_context_menu")
        self.menu_ext = msg.props.extension
        item = Gio.MenuItem.new(_("Search in files..."), "win.gedit-file-search-filebrowser")
        self.menu_ext.append_menu_item(item)

    def _removeFileBrowserMenuItem (self):
        self.window.remove_action("gedit-file-search-filebrowser")
        self.menu_ext = None

(In reply to comment #1)
> This interface requires sending a sync message to get back the extension
> object, is this a good idea?

Yes, this is perfectly fine. The message bus should actually be considered as a message passing style language. Originally, the split between sync and async was made because the bus was thought to be bridged to dbus and there it made sense to have async messages. I think we could probably do away with sync vs async messages on the bus. The way async is implemented is anyway just using an idle handler, so it's not even really async in the sense of being concurrent (it still runs on the same main loop).

Review of attachment 270623 [details] [review]:

Minor comment

::: plugins/filebrowser/gedit-file-browser-widget.c
@@ +2227,3 @@
+	gint i, n_items;
+	GMenuModel *section = NULL;
+

you're missing here a g_return_val_if_fail

Ok pushed the patch with the suggested fixes.



After discussing on irc I think we also need a way to know when the filebrowser is ready so that one can send this (and other) messages.

Jesse: am I missing something which is already there in a generic way to know when something is available on the bus?

(In reply to comment #5)
> Jesse: am I missing something which is already there in a generic way to know
> when something is available on the bus?

No, you can currently only check if a certain message type has been registered, but you can't get notified when it becomes available if it wasn't already. It would be pretty easy to add that though.

Yes, I considered adding a "ready" message or something like that for the filebrowser, but this is indeed a generic issue and it would be cool to have a generic solution

One thought here, do we really need this for the filebrowser? We should actually create the menu from an appactivatable, and then fetch it from the widget. At this point the menu is already accessible from the moment the plugin is activated.

Ok sorry, I was replying from memory which I shouldn't do. There are actually two things:

1. The message bus already has a signal for when a new message gets registered. You can simply obtain the path and method from the signal handler. The corresponding signal is called "registered" and you get two string parameters, object_path and method.

2. You can register callbacks for messages (i.e. gedit_message_bus_connect) before that message type has been registered by someone so order doesn't matter.

(In reply to comment #8)
> One thought here, do we really need this for the filebrowser? We should
> actually create the menu from an appactivatable, and then fetch it from the
> widget. At this point the menu is already accessible from the moment the plugin
> is activated.

No, the problem here is that there is no guarantee that the filebrowser plugin will be activated before your plugin (not only because the startup order is undefined, but also because one could literally enable the filbrowser plugin later with the prefs dialog.

So we need a way to say: "when the filebrowser plugin is ready and available on the bus, send it this sync message"

(In reply to comment #9)
> Ok sorry, I was replying from memory which I shouldn't do. There are actually
> two things:
> 
> 1. The message bus already has a signal for when a new message gets registered.
> You can simply obtain the path and method from the signal handler. The
> corresponding signal is called "registered" and you get two string parameters,
> object_path and method.
> 

Yes, this is a viable workaround, but seems a bit ugly: I'd prefer to have a clear way to express "when filebrowser is on the bus" rather then picking one specific message as a sentinel value and then waiting for its registration

> 2. You can register callbacks for messages (i.e. gedit_message_bus_connect)
> before that message type has been registered by someone so order doesn't
> matter.

Sure, but this works if you are waiting for a message from the filebrowser, this case is the reverse: we want to send a message to the filebrowser, and if you send it before the filebrowser is up, then you get an error

(In reply to comment #11)
> (In reply to comment #9)
> > Ok sorry, I was replying from memory which I shouldn't do. There are actually
> > two things:
> > 
> > 1. The message bus already has a signal for when a new message gets registered.
> > You can simply obtain the path and method from the signal handler. The
> > corresponding signal is called "registered" and you get two string parameters,
> > object_path and method.
> > 
> 
> Yes, this is a viable workaround, but seems a bit ugly: I'd prefer to have a
> clear way to express "when filebrowser is on the bus" rather then picking one
> specific message as a sentinel value and then waiting for its registration

I don't know, knowing that the file browser is active isn't actually relevant. You only need to have the 'extend_context_menu' on '/plugins/filebrowser' being available, which is what you can get from that signal.

I wonder if we can create a "detailed" signal so that you can connect to "register:/plugins/filebrowser" or if that is something special done for "notify" internally to gobject.

Oh well, I guess

bus.connect("register", lambda path, message: add_menu() if path is "/plugins/filebrowser" and message is "extend-context-menu")

is not that bad afterall.

Oliver, can you try this and see if fits your needs?

This works if I wrap a GLib.idle_add() around the add_menu() call.

If I try to modify the context menu right away as soon as the extend-context-menu message is registered, it fails with "AttributeError: 'NoneType' object has no attribute 'append_menu_item'", because msg.props.extension is None. Maybe the menu isn't completely initialized yet when the message is registered.

Not sure if the idle_add() workaround is reliable... Thoughts? But anyway, apart from this the interface is fine for me.

let's close this for now, since the original bug is fixed and the workaround about waiting for a signal to be registered is good enough