GNOME Bugzilla – Bug 691078
Remove now redundant upstream vapi files
Last modified: 2016-03-31 13:56:59 UTC
See patch!
Yikes, patch too big for bz. I'll split later and attach..
Created attachment 232672 [details] [review] Remove now redundant upstream cairo.vapi Vala 0.17.3 already has the updated VAPI that we were working around with our own copy from vala's git master.
Created attachment 232673 [details] [review] Remove now redundant upstream clutter-1.0.vapi & clutter-gtk-1.0.vapi Vala 0.17.3 already has the updated VAPI that we were working around with our own copy from vala's git master. Having said that, with newer vapis we seem to be using lots of deprecated API. It'll take a bit of effort/time to update all these but from what I can tell looking at the docs, we shouldn't need to bump our deps to fix (at least most) of these as the replacement API already existed in 3.6.x times. Here are the warnings we get with latest vala: revealer.vala:35.49-35.68: warning: Clutter.BinAlignment has been deprecated since 1.12 revealer.vala:36.49-36.68: warning: Clutter.BinAlignment has been deprecated since 1.12 revealer.vala:39.49-39.68: warning: Clutter.BinAlignment has been deprecated since 1.12 revealer.vala:40.49-40.68: warning: Clutter.BinAlignment has been deprecated since 1.12 revealer.vala:71.17-71.29: warning: Clutter.Actor.animate has been deprecated since 1.12 revealer.vala:79.17-79.29: warning: Clutter.Actor.animate has been deprecated since 1.12 revealer.vala:85.17-85.23: warning: Clutter.Actor.animate has been deprecated since 1.12 revealer.vala:87.17-87.23: warning: Clutter.Actor.animate has been deprecated since 1.12 revealer.vala:97.24-97.36: warning: Clutter.Actor.animate has been deprecated since 1.12 revealer.vala:98.13-98.26: warning: Clutter.Animation.completed has been deprecated since 1.12 revealer.vala:105.24-105.36: warning: Clutter.Actor.animate has been deprecated since 1.12 revealer.vala:106.13-106.26: warning: Clutter.Animation.completed has been deprecated since 1.12 revealer.vala:114.17-114.23: warning: Clutter.Actor.animate has been deprecated since 1.12 revealer.vala:116.17-116.23: warning: Clutter.Actor.animate has been deprecated since 1.12 machine.vala:521.9-521.23: warning: Clutter.BoxLayout.vertical has been deprecated since 1.12 sidebar.vala:43.42-43.61: warning: Clutter.BinAlignment has been deprecated since 1.12 sidebar.vala:44.42-44.61: warning: Clutter.BinAlignment has been deprecated since 1.12 sidebar.vala:66.9-66.15: warning: Clutter.BinLayout.add has been deprecated since 1.12 sidebar.vala:67.18-67.37: warning: Clutter.BinAlignment has been deprecated since 1.12 sidebar.vala:68.18-68.37: warning: Clutter.BinAlignment has been deprecated since 1.12 sidebar.vala:79.9-79.15: warning: Clutter.BinLayout.add has been deprecated since 1.12 sidebar.vala:79.30-79.49: warning: Clutter.BinAlignment has been deprecated since 1.12 sidebar.vala:79.57-79.76: warning: Clutter.BinAlignment has been deprecated since 1.12
Created attachment 232674 [details] [review] Remove now redundant upstream cogl-1.0.vapi Vala 0.17.3 already has the updated VAPI that we were working around with our own copy from vala's git master.
Created attachment 232675 [details] [review] Remove now redundant upstream gtk+-3.0.vapi & gdk-3.0.vapi Vala 0.17.3 already has the updated VAPI that we were working around with our own copy from vala's git master. Having said that, with newer vapis we seem to be using lots of deprecated API. It'll take a bit of effort/time to update all these but from what I can tell looking at the docs, we shouldn't need to bump our deps to fix (at least most) of these as the replacement API already existed in 3.6.x times. Here are the warnings we get with latest vala: i-properties-provider.vala:105.25-105.34: warning: Gtk.HScale has been deprecated since 3.2 machine.vala:534.20-534.27: warning: Gtk.VBox has been deprecated since 3.2. Use Box machine.vala:543.9-543.23: warning: Gtk.Widget.modify_fg has been deprecated since 3.0. Use override_color topbar.vala:48.24-48.31: warning: Gtk.HBox has been deprecated since 3.2. Use Grid topbar.vala:85.20-85.27: warning: Gtk.HBox has been deprecated since 3.2. Use Grid topbar.vala:112.20-112.27: warning: Gtk.HBox has been deprecated since 3.2. Use Grid topbar.vala:116.20-116.27: warning: Gtk.HBox has been deprecated since 3.2. Use Grid properties.vala:209.23-209.30: warning: Gtk.HBox has been deprecated since 3.2. Use Grid sidebar.vala:92.24-92.31: warning: Gtk.VBox has been deprecated since 3.2. Use Box sidebar.vala:96.20-96.27: warning: Gtk.VBox has been deprecated since 3.2. Use Box sidebar.vala:101.20-101.27: warning: Gtk.VBox has been deprecated since 3.2. Use Box unattended-installer.vala:305.26-305.33: warning: Gtk.HBox has been deprecated since 3.2. Use Grid notificationbar.vala:100.13-100.29: warning: Gtk.Button.pressed has been deprecated since 2.8. Use Gtk.Widget.button_press_event properties.vala:101.25-101.44: warning: Gtk.Widget.modify_fg has been deprecated since 3.0. Use override_color wizard-source.vala:238.24-238.31: warning: Gtk.VBox has been deprecated since 3.2. Use Box wizard.vala:735.13-735.32: warning: Gtk.Widget.modify_fg has been deprecated since 3.0. Use override_color wizard.vala:740.13-740.33: warning: Gtk.Widget.modify_fg has been deprecated since 3.0. Use override_color wizard.vala:754.13-754.28: warning: Gtk.Widget.modify_fg has been deprecated since 3.0. Use override_color wizard.vala:123.17-123.31: warning: Gtk.Widget.modify_fg has been deprecated since 3.0. Use override_color wizard.vala:127.13-127.38: warning: Gtk.Widget.modify_fg has been deprecated since 3.0. Use override_color wizard.vala:528.24-528.31: warning: Gtk.HBox has been deprecated since 3.2. Use Grid wizard.vala:543.24-543.31: warning: Gtk.VBox has been deprecated since 3.2. Use Box wizard.vala:567.20-567.27: warning: Gtk.VBox has been deprecated since 3.2. Use Box wizard.vala:579.20-579.27: warning: Gtk.HBox has been deprecated since 3.2. Use Grid wizard.vala:587.29-587.36: warning: Gtk.VBox has been deprecated since 3.2. Use Box wizard.vala:603.26-603.33: warning: Gtk.VBox has been deprecated since 3.2. Use Box wizard.vala:610.20-610.27: warning: Gtk.VBox has been deprecated since 3.2. Use Box
Created attachment 232676 [details] [review] Remove now redundant upstream gdk-pixbuf-2.0.vapi Vala 0.17.3 already has the updated VAPI that we were working around with our own copy from vala's git master.
Created attachment 232699 [details] [review] Adapt to Gtk+ 3.2 Drop usage of API deprecated in Gtk+ 3.2 * HScale -> Scale * VBox -> Box * HBox -> Box * Widget.modify_fg replaced by theme in most case but by Widget.override_color in 2. * Button.pressed -> Widget.button_press_event
It makes sense to fix the deprecation warnings before the vapi files are removed, as they really cripple the compilation. Afaik, we kept the current vapi files only for this reason.
(In reply to comment #8) > It makes sense to fix the deprecation warnings before the vapi files are > removed, as they really cripple the compilation. I don't have strong feelings either way but as you can see from my last patch, I'm already doing the porting task so we can wait with these patches till I'm done. > Afaik, we kept the current > vapi files only for this reason. I think the main reason is that its easy to forget about them and that was my worry when we decided to go with this approach vs. prefering specific workarounds in the code itself with FIXME comments. I still prefer the latter as they are harder to forget about and you don't get stuck with very old vapi when you do forget.
(In reply to comment #9) > (In reply to comment #8) > > It makes sense to fix the deprecation warnings before the vapi files are > > removed, as they really cripple the compilation. > > I don't have strong feelings either way but as you can see from my last patch, > I'm already doing the porting task so we can wait with these patches till I'm > done. > > > Afaik, we kept the current > > vapi files only for this reason. > > I think the main reason is that its easy to forget about them and that was my > worry when we decided to go with this approach vs. prefering specific As long as something work, I don't see why you would want to be reminded. as long as there is no limitation or bug in the current vapi, I don't see the rush, and I would keep a more conservative approach. However, fixing usage of deprecation APIs is a good goal.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > It makes sense to fix the deprecation warnings before the vapi files are > > > removed, as they really cripple the compilation. > > > > I don't have strong feelings either way but as you can see from my last patch, > > I'm already doing the porting task so we can wait with these patches till I'm > > done. > > > > > Afaik, we kept the current > > > vapi files only for this reason. > > > > I think the main reason is that its easy to forget about them and that was my > > worry when we decided to go with this approach vs. prefering specific > > As long as something work, I don't see why you would want to be reminded. > > as long as there is no limitation or bug in the current vapi, I don't see the > rush, and I would keep a more conservative approach. > > However, fixing usage of deprecation APIs is a good goal. We'll not notice us using deprecated APIs either with this approach. Thats exactly what happened now. We remove the vapi and all of a sudden we get tons of warnings about deprecated APIs. We'd have noticed them gradually over the time and fixed them gradually. Anyways, I'm speaking from my experience with both approaches, one in rygel and the other in boxes that the latter is better. Its now up to other developers if they want to trust me on this or not. I've nothing further to say about this.
Created attachment 232783 [details] [review] css: Explicitly specify units for font size This rids of the following runtime warnings against latest gtk+: (gnome-boxes:15902): Gtk-WARNING **: Theme parsing error: gtk-style.css:92:17: Not using units is deprecated. Assuming 'px'.
Created attachment 232785 [details] [review] Adapt to Clutter 1.12 API Issues: * It introduces a ton of runtime warnings about dimensions being set to negative numbers. * searchbar animations seems to go slower with this patch. * Still does not fix most of the warnings about Clutter.BinAlignment. According to ebassi, we still need to use Clutter.BinLayout, yet its new function still requires this deprecated type as arguments. Perhaps a workaround would be undeprecate Clutter.BinAlignment in upstream vapi. Although that will only work with future vala releases but perhaps we could live with these warnings till then?
(In reply to comment #13) > Created an attachment (id=232785) [details] [review] > Adapt to Clutter 1.12 API > > Issues: > > * It introduces a ton of runtime warnings about dimensions being set to > negative numbers. > * searchbar animations seems to go slower with this patch. > * Still does not fix most of the warnings about Clutter.BinAlignment. > According to ebassi, we still need to use Clutter.BinLayout, yet its > new function still requires this deprecated type as arguments. Perhaps > a workaround would be undeprecate Clutter.BinAlignment in upstream > vapi. Although that will only work with future vala releases but > perhaps we could live with these warnings till then? OK, I already spent plenty of time on this so if anyone (perhaps with a better grasp on clutter and gtk) could help me resolve the above issues, that would be lovely.
Review of attachment 232672 [details] [review]: ack
Review of attachment 232673 [details] [review]: ack, but we should fix warnings before landing.
Review of attachment 232674 [details] [review]: ack
Review of attachment 232675 [details] [review]: ack, but we should fix warnings before landing
Review of attachment 232676 [details] [review]: ack
Review of attachment 232699 [details] [review]: ::: src/notificationbar.vala @@ +98,3 @@ grid.add (ok_button); + ok_button.button_press_event.connect ( () => { This seems wrong. It should use "clicked" as we want to handle any sort of button press. ::: src/wizard.vala @@ +126,2 @@ /* highlight in white current page label */ + steps.get (page).override_color (Gtk.StateFlags.NORMAL, get_color ("white")); Why are you not using a temporary style class here?
Review of attachment 232783 [details] [review]: ack
Review of attachment 232785 [details] [review]: working on better changes for some of these
Review of attachment 232699 [details] [review]: ::: src/notificationbar.vala @@ +98,3 @@ grid.add (ok_button); + ok_button.button_press_event.connect ( () => { i didn't want to take a chance so I just replaced the deprecated 'pressed' signal with what the docs recommend. ::: src/wizard.vala @@ +126,2 @@ /* highlight in white current page label */ + steps.get (page).override_color (Gtk.StateFlags.NORMAL, get_color ("white")); because thats what existing code is doing(?). Now that you mentioned it, i guess the same could be achieved through gtk_style_context_add_class and gtk_style_context_remove_class.
Review of attachment 232699 [details] [review]: ::: src/notificationbar.vala @@ +98,3 @@ grid.add (ok_button); + ok_button.button_press_event.connect ( () => { Yeah, but thats clearly wrong. Means you can't e.g. activate the button via keyboard or a11y. ::: src/wizard.vala @@ +126,2 @@ /* highlight in white current page label */ + steps.get (page).override_color (Gtk.StateFlags.NORMAL, get_color ("white")); Well, you changed the other modify_fg to use add_class, which is why i asked why you didn't here too.
Created attachment 233138 [details] [review] Remove deprecated clutter stuff from Revealer This switches from using direct "animate" calls to using implicit animations. Also, we animate the translation rather than the position of the items as this is unrelated to the allocation and thus avoids lots of resize calculations. We also use a new custom layout manager instead of the bin as binlayout allocates a smaller child when we change the height (if resize==true) rather than clipping it.
Created attachment 233139 [details] [review] Remove deprecated clutter stuff from Revealer This switches from using direct "animate" calls to using implicit animations. Also, we animate the translation rather than the position of the items as this is unrelated to the allocation and thus avoids lots of resize calculations. We also use a new custom layout manager instead of the bin as binlayout allocates a smaller child when we change the height (if resize==true) rather than clipping it.
Created attachment 233140 [details] [review] Adapt to Clutter 1.12 API This gets rid of most deprecation warnings for clutter 1.12, but we still get a few warnings about Clutter.BinAlignment that we can't really get rid of (as its needed for the BinLayout constructor).
The last patch is just a rebase of zeeshans patch without the revealer part. I think we're now (sans my comments about the adapt to gtk+ 3.2 patch) are as ready as we can. We should probably work upstream to undeprecate Clutter.BinAlignment.
I chatted with clutter/vala people about this, and its hard atm to undeprecate the enum in C, but the vala people will change the bindings such that the enum itself is not deprecated but any values other than BinAlignment.START is deprecated, as BinAlignment.START is the behaviour that clutter 2.0 will use when the enum is removed.
(In reply to comment #32) > I chatted with clutter/vala people about this, and its hard atm to undeprecate > the enum in C, but the vala people will change the bindings such that the enum > itself is not deprecated but any values other than BinAlignment.START is > deprecated, as BinAlignment.START is the behaviour that clutter 2.0 will use > when the enum is removed. I should have provided you more details: I already had the same discussion with clutter & vala folks leading to the same conclusion. Would have saved you some time.
Created attachment 233150 [details] [review] Adapt to Gtk+ 3.2 Drop usage of API deprecated in Gtk+ 3.2 * HScale -> Scale * VBox -> Box * HBox -> Box * Widget.modify_fg replaced by theme. * Button.pressed -> Button.clicked
Review of attachment 233139 [details] [review]: Looks good. Also makes the code more readable imho.
Review of attachment 233140 [details] [review]: ACK
Review of attachment 233150 [details] [review]: looks good
Attachment 232672 [details] pushed as 104fbbb - Remove now redundant upstream cairo.vapi Attachment 232673 [details] pushed as 531544c - Remove now redundant upstream clutter-1.0.vapi & clutter-gtk-1.0.vapi Attachment 232674 [details] pushed as 9e62e89 - Remove now redundant upstream cogl-1.0.vapi Attachment 232675 [details] pushed as 6b534a0 - Remove now redundant upstream gtk+-3.0.vapi & gdk-3.0.vapi Attachment 232676 [details] pushed as 7eb42d3 - Remove now redundant upstream gdk-pixbuf-2.0.vapi Attachment 232783 [details] pushed as 58f256e - css: Explicitly specify units for font size Attachment 233139 [details] pushed as e3e4c45 - Remove deprecated clutter stuff from Revealer Attachment 233140 [details] pushed as b3ea499 - Adapt to Clutter 1.12 API Attachment 233150 [details] pushed as ea48c29 - Adapt to Gtk+ 3.2
Created attachment 233545 [details] [review] Fix the last of clutter deprecated API warnings
Review of attachment 233545 [details] [review]: ::: src/app.vala @@ +809,3 @@ actor.y_align = Clutter.ActorAlign.CENTER; + actor.x_expand = true; + actor.y_expand = true; Is this really right? This is in overlay_bin which used to be CENTER,CENTER, not fill.
Review of attachment 233545 [details] [review]: ::: src/app.vala @@ +809,3 @@ actor.y_align = Clutter.ActorAlign.CENTER; + actor.x_expand = true; + actor.y_expand = true; Yeah since the x and y alignment is CENTER?
Created attachment 234081 [details] [review] Fix the last of clutter deprecated API warnings
Review of attachment 232783 [details] [review]: This breaks parsing of gtk-style.css (and thus makes boxes ugly) with gtk+ 3.6: (gnome-boxes:11948): Boxes-WARNING **: main.vala:115: gtk-style.css:6:17Junk at end of value I think the deprecation warning appeared some time during gtk+ 3.7 development cycle.
Hmm, not sure what to do there? Do we require a new gtk? or keep the runtime warnings for another cycle?
(In reply to comment #44) > Hmm, not sure what to do there? Do we require a new gtk? or keep the runtime > warnings for another cycle? There are not that many of them and I think we can live with them.
Maybe we can get the gtk+ folks to only deprecate this in the next cycle, or to fix 'px' parsing in a stable 3.6 release? Saying in 3.7 that something is deprecated while the non-deprecated way does not work in 3.6 is suboptimal imo.
Review of attachment 234081 [details] [review]: ACK
(In reply to comment #46) > Maybe we can get the gtk+ folks to only deprecate this in the next cycle, or to > fix 'px' parsing in a stable 3.6 release? Saying in 3.7 that something is > deprecated while the non-deprecated way does not work in 3.6 is suboptimal imo. Yeah, that would be better indeed.
Comment on attachment 234081 [details] [review] Fix the last of clutter deprecated API warnings Attachment 234081 [details] pushed as 8e23973 - Fix the last of clutter deprecated API warnings
(In reply to comment #48) > (In reply to comment #46) > > Maybe we can get the gtk+ folks to only deprecate this in the next cycle, or to > > fix 'px' parsing in a stable 3.6 release? Saying in 3.7 that something is > > deprecated while the non-deprecated way does not work in 3.6 is suboptimal imo. > > Yeah, that would be better indeed. Here is what gtk+ people said about this on IRC: 16:02 <@Company> alex: but you can g_signal_connect an error handler that ignores deprecations 16:31 <@Company> teuf: though i suggested to alex to g_signal_connect() an error handler to your CssProvider 16:36 <@Company> teuf: in any case, either g_signal_connect() your own handler to get rid of the stderr spam, or write a patch to accept px for font sizes - both of these seem reasonable to me