GNOME Bugzilla – Bug 723000
fails to build gnome-contacts
Last modified: 2014-02-11 22:55:05 UTC
vala git (0.23.1.24-603c) fails building gnome-contacts: contacts-view.vala:268.3-268.35: error: overriding method `Contacts.View.row_selected' is incompatible with base method `Gtk.ListBox.row_selected': incompatible type of parameter 1. public override void row_selected (ListBoxRow row) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ contacts-view.vala:268.3-268.35: error: Contacts.View.row_selected: no suitable method found to override public override void row_selected (ListBoxRow row) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This builds fine in vala 0.22.1.
Use Gtk.ListBoxRow? row, sorry for the breakage but that's necessary if you want to also consider the case when no row is selected. Assigning to gnome-contacts.
Created attachment 267611 [details] [review] src: Add a missing nullability marker This fixes builds with recent versions of Vala (such as 0.23.1.38-994dd).
Review of attachment 267611 [details] [review]: Looks good to me.
Beware that might not work with older vala anymore.
(In reply to comment #2) > Created an attachment (id=267611) [details] [review] > src: Add a missing nullability marker > > This fixes builds with recent versions of Vala (such as > 0.23.1.38-994dd). Can you update configure.ac as well, so it requires newwer vala versions ?
(In reply to comment #4) > Beware that might not work with older vala anymore. Why not? Vala’s had nullability support for ages hasn’t it? What versions of Vala do you think it might break with?
The versions that don't have ListBoxRow? in the vapi.
You can however conditionate over #if VALA_X_Y if you don't want to raise the dependency in configure.ac. Like: #if VALA_0_24 public override void row_selected (ListBoxRow? row) { #else public override void row_selected (ListBoxRow row) { #endif
Created attachment 268017 [details] [review] src: Add a missing nullability marker This fixes builds with recent versions of Vala (such as 0.23.1.38-994dd). The change was introduced in Vala’s GTK+ bindings in bug #722490.
Review of attachment 268017 [details] [review]: I liked the other one better. Actually I would prefer the code not containing ifdef/else kind of macro which only makes harder to read it. Please, use your previous version and update configure.ac properly.
(In reply to comment #10) > Review of attachment 268017 [details] [review]: > > I liked the other one better. Actually I would prefer the code not containing > ifdef/else kind of macro which only makes harder to read it. > > Please, use your previous version and update configure.ac properly. I would strongly suggest that you don't bump the Vala dependency just for this tiny patch. Every time you bump the Vala dependency, you are potentially forcing people to rebuild Vala, which wastes a lot of people's time.
(In reply to comment #11) > (In reply to comment #10) > > Review of attachment 268017 [details] [review] [details]: > > > > I liked the other one better. Actually I would prefer the code not containing > > ifdef/else kind of macro which only makes harder to read it. > > > > Please, use your previous version and update configure.ac properly. > > I would strongly suggest that you don't bump the Vala dependency just for this > tiny patch. Every time you bump the Vala dependency, you are potentially > forcing people to rebuild Vala, which wastes a lot of people's time. The thing is eventually vala will move on, and the stable version will meet the requirement, and that isn't too much far away, it should be ready for this release of GNOME. But, let's find a middle ground, do you have any other suggestion different from the ifdef macros ?
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > Review of attachment 268017 [details] [review] [details] [details]: > > > > > > I liked the other one better. Actually I would prefer the code not containing > > > ifdef/else kind of macro which only makes harder to read it. > > > > > > Please, use your previous version and update configure.ac properly. > > > > I would strongly suggest that you don't bump the Vala dependency just for this > > tiny patch. Every time you bump the Vala dependency, you are potentially > > forcing people to rebuild Vala, which wastes a lot of people's time. > > The thing is eventually vala will move on, and the stable version will meet the > requirement, and that isn't too much far away, it should be ready for this > release of GNOME. I suspect there are people who want to build gnome-contacts just using distribution-provided development packages, and I don’t think Vala is as up-to-date in distributions. > But, let's find a middle ground, do you have any other suggestion different > from the ifdef macros ? Not really. I think it’s either the ifdefs or bumping the Vala dependency in configure.ac.
I just pushed your last patch. Let's bear with the if/def for a while. At least till vala-0.23 appears on fedora and archlinux.