Deprecation warning cleanup

Submitted by Francois Gouget on Oct. 18, 2016, 5:46 p.m.

Details

Reviewer None
Submitted Oct. 18, 2016, 5:46 p.m.
Last Updated Dec. 8, 2016, 6:29 a.m.
Revision 11

Cover Letter(s)

Revision 1
      This is another take on trying to clean up the deprecation warning 
situation in Spice-Gtk.

Let me know if it goes in the right direction.


With this patchset deprecation warnings are enabled again and there are 
no remaining warnings, except those in src/controller since they happen 
in generated code and I don't know how to modify it so it does not use 
the deprecated GSimpleAsyncResult class (and I don't really want to do 
the conversion to GTask either). Of course those could also be avoided 
by keeping the -Wno-deprecated-declarations in src/controller but then 
nobody will fix that code.


Francois Gouget (10):
  build-sys: Enable deprecation warnings instead of ignoring them entirely

First the Spice-Gtk situation is different from the Spice server one.

In Spice server the only deprecated APIs we use are Spice's own. So new 
uses will only result from changes in Spice's code. Thus it's ok to have 
them produce an error to prevent their use from spreading.

In Spice-Gtk the deprecated APIs are almost all GTK APIs. Those can be 
deprecated at any time independently of Spice-Gtk's code. Thus we don't 
want deprecation warnings to break the build.

But so far they have be completely disabled, thus ensuring no one 
will ever fix them. So the first patch turns them back to regular 
warnings with -Wno-error.


  gtk: Avoid gtk_vbox_new(); it is deprecated
  gtk: Avoid gtk_hbutton_box_new(); it is deprecated
  gtk: GtkTable is deprecated so use GtkGrid instead

These patches just fix usage of some deprecated APIs. Fixes that would 
likely have happened not long after GTK+ 2.0 support was dropped if 
deprecation warnings had not been disabled. Those should clearly be 
applied even if we don't enable deprecation warnings.


  gtk: Ignore GLib's too-new warnings where we explicitly check its version

GLib also issues warnings for APIs that are more recent than 
GLIB_VERSION_MIN_REQUIRED. But these happen in suitable #if sections and 
thus should be ignored.


  build-sys: Remove SPICE_NO_DEPRECATED
  build-sys: Use spice-protocol's deprecation macros

This one may be somewhat questionable since it spice-protocol's macros 
don't use GLib's helpers and thus don't support Clang and Visual C++. 
This means we may locally disable these warnings with 
G_GNUC_BEGIN_IGNORE_DEPRECATIONS when they would not have in fact been 
issued at all.
* It's not like this issue would cause compilation errors or 
  warnings (the opposite) so maybe it should just be ignored.
* Or maybe it means Spice-Gtk should keep using its own deprecation
  macros, but it would be a bit of a shame that spice-protocol's macros 
  cannot even be reused by all Spice's codebase.
* Or maybe  spice-protocol should use GLib's macros if available, 
  even if it is a bit distasteful.


  session: Disable the spice_audio_new() deprecation warning

I donsider this to be a "false positive", although maybe the code should 
be rewritten to avoid it.


  RFC: gtk: Temporarily ignore the keyboard/mouse grabbing deprecation warnings
  RFC: spicy: Temporarily ignore deprecation warnings

These two really just silence warnings that should be fixed. So if 
someone fixes these warnings they will not be needed. Otherwise I think 
it's still better than globally disabling the deprecation warnings 
because these patches are local and add nice FIXME warnings in the 
relevant places.


 m4/spice-compile-warnings.m4 |  2 +-
 src/Makefile.am              |  1 -
 src/controller/Makefile.am   |  1 -
 src/spice-channel.h          |  5 +++--
 src/spice-gtk-session.c      |  5 +++++
 src/spice-session.c          |  2 ++
 src/spice-util.h             | 14 +++-----------
 src/spice-widget-egl.c       |  3 +++
 src/spice-widget.c           | 30 ++++++++++++++++++++++++++++++
 src/spicy-connect.c          | 22 +++++++++++-----------
 src/spicy.c                  |  3 +++
 src/usb-device-manager.h     |  2 +-
 12 files changed, 62 insertions(+), 28 deletions(-)
    
Revision 2
      This is v2 of the following patchset:

https://lists.freedesktop.org/archives/spice-devel/2016-October/032797.html

Changes since v1:
 * I have put the patch that enables the deprecation warnings last so 
   compiling does not generate warnings at any point. But really all the 
   patches are independent and none will cause compilation errors 
   (afaics) so you should feel free to reorder them as you wish.
 * I also modified that patch to keep the controller warnings disabled 
   since, if I understand correctly, we have no control over the 
   valac-generated code.
 * I changed the spice_audio_new() patch to remove it from the public 
   header as suggested by Victor Toso:
   https://lists.freedesktop.org/archives/spice-devel/2016-October/032829.html
 * I hope I got the 'spicy:' prefix right.
 * I removed the 'RFC' prefix from the patches for temporarily ignoring 
   deprecation warnings in select files. My understanding is that if no 
   better patch shows up we will want to apply those to avoid warnings 
   (or skip the last three patches in the set).


Francois Gouget (10):
  gtk: Ignore GLib's too-new warnings where we explicitly check its version
  spicy: Avoid gtk_vbox_new(); it is deprecated
  spicy: Avoid gtk_hbutton_box_new(); it is deprecated
  spicy: GtkTable is deprecated so use GtkGrid instead
  build-sys: Remove SPICE_NO_DEPRECATED
  build-sys: Use spice-protocol's deprecation macros
  audio: Remove spice_audio_new() from the public header
  gtk: Temporarily ignore the keyboard/mouse grabbing deprecation warnings
  spicy: Temporarily ignore deprecation warnings
  build-sys: Enable deprecation warnings instead of ignoring them entirely

 m4/spice-compile-warnings.m4 |  2 +-
 src/Makefile.am              |  1 -
 src/controller/Makefile.am   |  2 ++
 src/spice-audio-priv.h       |  2 ++
 src/spice-audio.h            |  5 -----
 src/spice-channel.h          |  5 +++--
 src/spice-gtk-session.c      |  5 +++++
 src/spice-session.c          |  2 +-
 src/spice-util.h             | 14 +++-----------
 src/spice-widget-egl.c       |  3 +++
 src/spice-widget.c           | 30 ++++++++++++++++++++++++++++++
 src/spicy-connect.c          | 22 +++++++++++-----------
 src/spicy.c                  |  3 +++
 src/usb-device-manager.h     |  2 +-
 14 files changed, 65 insertions(+), 33 deletions(-)
    
Revision 5
      This is v3 of the following patchset:

https://lists.freedesktop.org/archives/spice-devel/2016-October/032797.html


Changes since v2:
 * Jonathon Jongsma pointed out that the 'too-new warnings' patch had an 
   unrelated hunk. It is now the clipboard_get() patch.
 * And I updated the clipboard_get() patch based on Jonathon Jongsma's 
   comments:
   https://lists.freedesktop.org/archives/spice-devel/2016-October/032923.html
 * Christophe Fergeau said the vncdisplaykeymap GTK+ 2 compatibility 
   macros can be dropped.
   https://lists.freedesktop.org/archives/spice-devel/2016-October/032995.html


Francois Gouget (10):
  gtk: Remove an obsolete comment
  vncdisplaykeymap: Remove obsolete GTK+ 2 compatibility macros
  gtk: Ignore GLib's too-new warnings where we explicitly check its version
  gdk: Ignore clipboard_get()'s deprecation warnings
  audio: Remove spice_audio_new() from the public header
  build-sys: Remove SPICE_NO_DEPRECATED
  build-sys: Use spice-protocol's deprecation macros
  gtk: Temporarily ignore the keyboard/mouse grabbing deprecation warnings
  spicy: Temporarily ignore deprecation warnings
  build-sys: Enable deprecation warnings instead of ignoring them entirely

 m4/spice-compile-warnings.m4 |  2 +-
 src/Makefile.am              |  1 -
 src/controller/Makefile.am   |  2 ++
 src/spice-audio-priv.h       |  2 ++
 src/spice-audio.h            |  5 -----
 src/spice-channel.h          |  5 +++--
 src/spice-gtk-session.c      | 11 +++++++++--
 src/spice-session.c          |  2 +-
 src/spice-util.h             | 14 +++-----------
 src/spice-widget-egl.c       |  3 +++
 src/spice-widget.c           | 31 ++++++++++++++++++++++++++++++-
 src/spicy.c                  |  3 +++
 src/usb-device-manager.h     |  2 +-
 src/vncdisplaykeymap.c       | 27 +--------------------------
 14 files changed, 59 insertions(+), 51 deletions(-)
    
Revision 7
      I know the first two patches are really just sweeping issues under the 
rug but I really thing they should still get in:

 * As long as the policy is that spice-gtk must compile without warnings 
   then they are needed for the third patch which reenables deprecation 
   warnings.

 * So really this patchset trades one huge rug for two really small 
   ones. I.e. currently it's possible to use deprecated APIs anywhere in 
   the code and be none the wiser, whereas with this patch it's only 
   possible in 5% of the code.

 * Now if there are better replacements for these patches then sure they 
   should go in. But if there is no timeframe for when these 
   replacements will be ready then it may be better to commit these in 
   the meantime.

See:
https://lists.freedesktop.org/archives/spice-devel/2016-October/032830.html


Francois Gouget (3):
  gtk: Temporarily ignore the keyboard/mouse grabbing deprecation
    warnings
  spicy: Temporarily ignore deprecation warnings
  build-sys: Enable deprecation warnings instead of ignoring them
    entirely

 m4/spice-compile-warnings.m4 |  2 +-
 src/controller/Makefile.am   |  2 ++
 src/spice-widget.c           | 24 ++++++++++++++++++++++++
 src/spicy.c                  |  3 +++
 4 files changed, 30 insertions(+), 1 deletion(-)
    

Revisions