GNOME Bugzilla – Bug 678655
Need an override for g_action_map_add_action_entries()
Last modified: 2018-01-10 20:18:15 UTC
I've been tinkering with Gtk.Application.set_menubar() and Gtk.Application.set_app_menu() in Python and have encountered a few issues. The g_action_map_add_action_entries() doesn't seem to play nice into python. (it wants a Gio.Action but gets StructMeta). Ideally it could work something like: --- # window is Gtk.ApplicationWindow entries = [ ("fullscreen", window.activate_toggle, False, change_fullscreen_state), # etc. ] window.add_action_entries(entries) --- I thought I could simple hack together a patch in gi/overrides/Gio.py similar to what is done with the Gtk.ActionGroup.add_actions(), but, all the GVariant stuff went over my head. So this is what DOES work. It results in a lot of tedious typing. --- # window is Gtk.ApplicationWindow action = Gio.SimpleAction.new_stateful("fullscreen", None, GLib.Variant("b", False)) action.connect("activate", window.activate_toggle) action.connect("change-state", self.change_fullscreen_state) window.add_action(action) # etc.. --- I *think* some of the other classes from GLib like GAction, GActionMap, and GSimpleAction might need some overrides too. That is, I assume we don't want Python applications directly using GLib.Variant? Or maybe we do? It seems to me the "pythonic" way is to make the GVariant based on the python type: --- action = Gio.SimpleAction(state=False) action = Gio.SimpleAction(state='left') ---
Created attachment 217253 [details] [review] Override ActionMap and ActionMap.add_action_entries() I've attached a patch that I think does an okay job of solving this. After this patch, entries can be added in bulk like in C: --- entries = [ ("copy", self.copy), ("paste", self.paste), ("fullscreen", self.activate_toggle, None, "false", self.change_fullscreen_state), ("justify", self.activate_radio, "s", "'left'", self.change_justify_state), ] self.add_action_entries(entries) --- One thing I wasn't sure about: I have it throwing an exception when the parameter type is not a valid GVariantType and also if the state cannot be parsed as a GVariant, however, that prevents all of the remaining actions from being added. Maybe it would be better to issue a warning and simply omit that one action? How is that typically addressed in PyGObject?
(In reply to comment #0) > The g_action_map_add_action_entries() doesn't seem to play nice into python. > (it wants a Gio.Action but gets StructMeta). Indeed, when you write (a, b, ..) in Python this is a simple tuple, not a Gio.Action object. Unfortunately you cannot create structs in pygobject which have nontrivial data types: >>> a = Gio.Action() Traceback (most recent call last):
+ Trace 230497
> I assume we don't want Python applications directly using GLib.Variant? We actually do, in a few cases. We went through a fair amount of effort to ensure that creating variants is as comfortable as it can get (see the GLib overrides). > It seems to me the "pythonic" way is to make the GVariant based on the python type: That does work in a lot of cases, but not in all. E. g. Python just has one kind of integer, while C APIs/variants have bytes, int16, int32 and int64, and then the same for unsigned again. Likewise, you cannot guess whether a string is a "normal" string (type s) or an object path (type o) or a signature (type g). Same for float vs. double. (In reply to comment #1) > One thing I wasn't sure about: I have it throwing an exception when the > parameter type is not a valid GVariantType and also if the state cannot be > parsed as a GVariant, however, that prevents all of the remaining actions from > being added. Maybe it would be better to issue a warning and simply omit that > one action? How is that typically addressed in PyGObject? I think it's a lot better to throw an exception. Your program should crash with a proper backtrace when you write wrong code; merely issuing a warning is very likely to be overlooked, and then you just have a much more subtle bug. I agree that an override here makes things a lot easier (like the example you showed how it should look like). Some notes about the patch: + variant_state = GLib.Variant.parse(None, state, None, None) We already know the type, so it should be supplied as first argument instead of None. Also, this is in a try/except which catches and re-raises GLib.GError. I think it would be more appropriate to just let it fail with the original exception. If that exception cdoes not make sense, let's fix the exception in glib instead of hacking around it in pygobject. But right now that patch does not check what kind of error has been raised at all. + if user_data is None: + action.connect('change-state', change_state) + else: + action.connect('change-state', change_state, user_data) (same a few lines below). That seems unnecessarily complicated. Just use the last line, if user_data is None it doesn't matter. Can you please add some test cases to tests/test_overrides.py which exercise a few ways to instantiate this, as well as some error cases (mismatching signature/type, etc.)? Thanks!
I finally have a small chunk of time to work on this. The test should be put in tests/test_overrides_gio.py not tests/test_overrides.py, right?
(In reply to comment #3) > I finally have a small chunk of time to work on this. The test should be put in > tests/test_overrides_gio.py not tests/test_overrides.py, right? Indeed.
ping?
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/pygobject/issues/29.