haskell-gi icon indicating copy to clipboard operation
haskell-gi copied to clipboard

dbusmenu missing almost all nullable annotations

Open colonelpanic8 opened this issue 7 years ago • 14 comments

https://hackage.haskell.org/package/gi-dbusmenu-0.4.1/docs/GI-Dbusmenu-Objects-Menuitem.html#g:6

The docsrings of these functions indicate that the values can be null, but the values are returned without being wrapped in a maybe

colonelpanic8 avatar Jul 06 '18 06:07 colonelpanic8

Thanks for the report!

We could add overrides for this. If you have a list of functions that you would be interested to fix a pull request with overrides would be very welcome. (See for example https://github.com/haskell-gi/haskell-gi/blob/master/bindings/Gtk/Gtk.overrides for the syntax, particularly where the nullable attributes are set. It should be fairly self-explanatory, but please let me know if not.)

In any case it would also be good to report this upstream, in dbusmenu itself, so all language bindings benefit from the fix (well, at least the ones that care about nullability).

garetxe avatar Jan 01 '19 18:01 garetxe

Hi @IvanMalison. Would you be interested in providing a list of functions with missing nullability annotations?

It is straightforward to add overrides for these, https://github.com/haskell-gi/haskell-gi/blob/master/bindings/Gtk-3.0/Gtk.overrides has plenty of self-explanatory examples. A pull request would be greatly appreciated, if you are still interested.

Otherwise this issue probably belongs upstream, so that all language bindings can benefit from the improved annotations.

garetxe avatar Jul 13 '19 19:07 garetxe

If no one's currently working on this and it's straightforward to add, I'd like to take up the assignment, as I've been personally bitten by it (#242 ). I'm a little confused about reporting it upstream though. The problem is with this package, right?

ghost avatar Jul 23 '19 02:07 ghost

Thanks for the offer! Yes, this is straightforward to add, and I would be glad to help with the syntax. See for example https://github.com/haskell-gi/haskell-gi/blob/master/bindings/GObject/GObject.overrides. The basic syntax is

set-attr /some/path nullable 1

where /some/path is a path in DbusmenuGtk3-0.4.gir (which is an XML file). Unfortunately the syntax for specifying paths is not documented, but it should hopefully be fairly self-explanatory.

Please let me know if you run into any issues.

I'm a little confused about reporting it upstream though. The problem is with this package, right?

The problem is with the upstream package missing introspection annotations (the haskell bindings are generated based on these annotations). But haskell-gi provides a way of "overriding" annotations, for cases such as these, so that we can generate fixed bindings without having to wait for everybody to upgrade to fixed upstream versions.

garetxe avatar Jul 23 '19 08:07 garetxe

I can't figure out how to report this upstream. https://bugs.launchpad.net/libdbusmenu is saying "DBus Menu must be configured in order for Launchpad to forward bugs to the project's developers." I also can't build it, and some of the functions that need this fix don't seem to even be in the source code (grep turns up nothing for anything related to comboBox or combo_box in the repo). (It also seems from launchpad that the last activity there was almost a year ago.)

Assuming I'll have to make the patch here; I think I know what line I need to add to fix comboBoxTextGetActiveText, but I can't build the package in order to make sure. What's the build process here? cabal new-build from the bindings directory succeeded after my change, but I don't know where the output packages are going or how to use them, and I imagine I'm supposed to run it from the top directory (which fails with the wrong version of haskell-gi-base even after building from /haskell-gi-base with no errors).

Edit: I think I've realized that the reason I couldn't find comboBox stuff in libdbusmenu is because it isn't in dbusmenu; I was using this package to use Gtk. I don't know if dbusmenu was even involved in the problem I was having. I'll look into submitting a patch to Gtk; I found the function in their source that seems to be missing the annotation.

ghost avatar Jul 23 '19 14:07 ghost

What's the build process here? cabal new-build from the bindings directory succeeded after my change, but I don't know where the output packages are going or how to use them

After checking out the code in git (make sure that you pull the latest version, I just made some fixed that affect what I say below), in the bindings directory you want to do:

$ cabal new-run genBuildInfo $(./PKGS.sh)

This will generate the gi-* files for the supported bindings (you might want to remove things from PKGS.sh if you don't have the necessary devel libraries installed for some of the more exotic libs).

Then, in the main directory, you can do

$ cabal new-build gi-dbusmenugtk3

If you change the .overrides file you will want to force the code generator to rebuild everything taking into account the new overrides, to do this the simplest way is:

$ rm -rf bindings/DbusmenuGtk3/GI
$ rm -rf dist-newstyle/build/x86_64-linux/ghc-8.6.1/gi-dbusmenugtk3-*

(you might need to alter the last path a bit, depending on your configuration), and then try cabal new-build gi-gtkdbusmenu3 again.

garetxe avatar Jul 23 '19 15:07 garetxe

(I just saw your edit. What I said above applies equally well for gtk, if you want to add an override there.)

garetxe avatar Jul 23 '19 15:07 garetxe

Phew! I had to jump through a lot hoops, but I managed to get it built and the function worked. I still didn't figure out how to install the packages correctly on my system, but I could use them if I was in the build directory.

Unless you'd like a patch here anyway, since the problem seems to be with the annotations in the packages haskell-gi builds on rather than haskell-gi itself, I think I'll try to get a patch into Gtk instead and let the improvement flow downstream.

ghost avatar Jul 23 '19 16:07 ghost

I think I'll try to get a patch into Gtk instead and let the improvement flow downstream.

Sure, if you are in no hurry this is a good solution!

garetxe avatar Jul 23 '19 18:07 garetxe

:+1: I got my GTK patch merged :+1:

That's only fofr the one function I found though. The dbusmenu ones still need fixing (as well as possible others in GTK or other libraries). I wouldn't mind doing that too, but I probably won't be on it in the next couple of days.

ghost avatar Jul 24 '19 14:07 ghost

Great, thanks for doing this!

garetxe avatar Jul 24 '19 14:07 garetxe

@aprogrammer8 Yeah I was kind of surprised to see that anyone cared abut this issue at all. The dbusmenu library is pretty obscure (Only used for menus that need to be serialized and relayed over dbus) so I wouldnt expect that its use is very common outside the application of StatusNotifierItem.

colonelpanic8 avatar Jul 24 '19 16:07 colonelpanic8

I finally came back to this and reported it upstream in dbusmenu (https://bugs.launchpad.net/ubuntu/+source/libdbusmenu/+bug/1840278). It's been sitting for long enough that combined with the lack of recent development history I'm thinking about making a patch here. What do you guys think?

ghost avatar Aug 26 '19 14:08 ghost

In general it is probably preferable to make the patch upstream so other bindings benefit, but in this particular case it might indeed be better to make a patch to gi-dbusmenu itself.

Please feel free to open a pull request! You need to modify bindings/Dbusmenu/Dbusmenu.overrides, and perhaps bindings/DbusmenuGtk3/DbusmenuGtk3.overrides to add the nullable annotations to the bindings. The .overrides files specify how to patch the corresponding .gir files (for instance, /usr/share/gir-1.0/Dbusmenu-0.4.gir in linux), using syntax that should hopefully be obvious (but please feel free to ask if not!).

See for example here for an example of nullable annotations.

garetxe avatar Sep 15 '19 08:09 garetxe