GNOME Bugzilla – Bug 705810
Deprecate class initializer overrides
Last modified: 2018-10-23 16:56:08 UTC
There are currently around 50 class overrides in Gtk half which do nothing more than explicitly list a construction properties. These are in the form: class HScrollbar(Gtk.HScrollbar): def __init__(self, adjustment=None, **kwds): Gtk.HScrollbar.__init__(self, adjustment=adjustment, **kwds) The proposal is to print a deprecation message when the keyword arg is not specified: scrollbar = HScrollbar(adjust) And instead require adjustment as a keyword arg (all GObject properties are valid keyword arguments to initializers): scrollbar = HScrollbar(adjustment=adjust) There are a few which rely on non-standard defaults. For instance, with SizeGroup the GTK+ default is HORIZONTAL, whereas we override it with the default as VERTICAL. These can also be marked as deprecated with a bit of special care. Essentially, if the non-standard default is used, also give a deprecation message: class SizeGroup(Gtk.SizeGroup): def __init__(self, mode=Gtk.SizeGroupMode.VERTICAL): super(SizeGroup, self).__init__(mode=mode)
Most of those overrides were added to reproduce pygtk api. While I understand the will of cutting down overrides, it is also true that relying on those defaults makes code way cleaner and more pythonic. I am not sure that the churn of deprecating them gains us anything. Beside the ability to mark args as "default" in g-i is a long standing issue and the idea was to keep the overrides around until that was properly fixed
Hi Paolo! (In reply to comment #1) > While I understand the will of cutting down overrides, it is also true that > relying on those defaults makes code way cleaner and more pythonic. In the case of the HScrollbar example (and this seems to be the majority), a default of None will be implied (or whatever the class default for the property is). We could also make this part of the pydoc by listing G_PARAM_CONSTRUCT properties as keyword args. > I am not sure that the churn of deprecating them gains us anything. This is mostly a reaction to performance related problems with overrides. Basically each of these adds overhead because they register the type and load all the methods for the class being subclassed. There is some work to make this lazy but it will require obfuscation to the overrides and this work started to make these overrides look somewhat unnecessary. > Beside the ability to mark args as "default" in g-i is a long standing issue > and the idea was to keep the overrides around until that was properly fixed Initializers/constructors are a different case than regular argument defaults. They basically pass anything given as a keyword to g_object_newv which accepts a list of keyed GParameters that are interpreted as property setters. So the class will already have defaults for these as property defaults. There are only a couple which use different defaults than the class (SizeGroup). If we can add lazy loading of these, we won't need to remove them, but I still think a deprecation message won't hurt where the default is already setup for us. And the explicit usage of the argument with a keyword is both backwards and forwards compatible and pythonic :)
Created attachment 251563 [details] [review] tests: Use explicit keywords args when calling initializers Replace all usage of GObject creation that relies on positional arguments from overrides. Positional initializer args will be deprecated, updating the tests as a first pass proves backwards and forwards compatibility of the deprecation.
Created attachment 251564 [details] [review] Add deprecation warnings and cleanup class initializer overrides Print deprecation warnings for calls to class initializers which don't explicitly specify keywords. Print deprecation warning for overrides that have renamed keywords (Gtk.Table.rows should be n_rows). Additionally deprecate non-standard defaults with initializers (Gtk.SizeGroup.mode defaults to HORIZONTAL in GTK+ and VERTICAL in PyGI). Remove AboutDialog override because it doesn't do anything. Pass keyword args through TreeStore.__init__.
Created attachment 251584 [details] [review] Add deprecation warnings and cleanup class initializer overrides Fixed up a few grammer mistakes.
Created attachment 251586 [details] [review] Add deprecation warnings and cleanup class initializer overrides Fixed a few grammer mistakes.
Some extra notes: All deprecations link to the following wiki page giving a longer description and rational: https://wiki.gnome.org/PyGObject/InitializerDeprecations All defaults that were previously hard-coded in the overridden initializers have been verified as already a default for the property. The only exception was SizeGroup.mode which defaults to Gtk.SizeGroupMode.HORIZONTAL in GTK+ and Gtk.SizeGroupMode.VERTICAL in PyGTK. A deprecation message is given when this is created without arguments telling you to explicitly specify the mode.
Created attachment 251780 [details] [review] Add deprecation warnings and cleanup class initializer overrides Fixed incorrect base class __init__ calls.
Created attachment 251781 [details] [review] Cleanup overzealous new and init implementations Remove PyGObject initializer code attempting to set properties on GObjects that have already been created. There were a number of overridden __new__ and __init__ methods that stripped away arguments before calling the base class to work around attempted property sets and argument count errors (fixing the symptom not the problem).
Created attachment 251782 [details] [review] Add deprecation warnings and cleanup class initializer overrides Fix syntax error that somehow made its way into the last patch.
Created attachment 251783 [details] [review] Deprecate Gdk.Cursor constructor dispatching Give deprecation warning for the overridden __new__ method on Gdk.Cursor when more than one argument is used. Recommend using Gdk.Cursor.new_for_display, new_from_pixbuf, and new_from_pixmap instead.
Created attachment 251962 [details] [review] Add deprecation warnings and cleanup class initializer overrides Removed __init__ updates because they are replaced by __new__ in a later patch.
Created attachment 251963 [details] [review] Cleanup overzealous new and init implementations Updated to use Gtk.ListStore/TreeStore.new with __new__ override instead of __init__ with set_column_types.
Created attachment 251964 [details] [review] Deprecate Gdk.Cursor constructor dispatching Rebase on prior patches.
Created attachment 251965 [details] [review] Deprecate Gtk.TreePath constructor dispatching Give deprecation warning for the overridden __new__ method on Gtk.TreePath with various argument types. Recommend using Gtk.TreePath.new_from_string and Gtk.TreePath.new_from_indices.
*** Bug 698711 has been marked as a duplicate of this bug. ***
(In reply to comment #1) > While I understand the will of cutting down overrides, it is also true that > relying on those defaults makes code way cleaner and more pythonic. I am not > sure that the churn of deprecating them gains us anything. As the API would become a lot stricter, it avoids confusion and silent bugs. E. g. trying to pass an enum value where the API expects an int of something completely different, like in bug 711487: Gtk.FileChooserWidget(Gtk.FileChooserAction.SELECT_FOLDER) Or Gtk.Box(Gtk.Orientation.HORIZONTAL) They both have some intended meaning, but the arguments are silently ignored. This is especially bad because e. g. the Gtk.Box overridden constructor (which specifies "homogenous" as the first positional argument) is also applied to subclasses like Gtk.HBox or Gtk.FileChooserWidget which leads to totally counter-intuitive and hard to debug failues like the ones above. So at the very least we need to change the overrides to provide a non-magic constructor for all subclasses of these cases (Gtk.Box, Gtk.Window, etc.). But I'm much more in favor of dropping them altogether as they make an otherwise nice and consistent API quite arbitrary, error prone, and inconsistent with how GI works with every other library.
Comment on attachment 251563 [details] [review] tests: Use explicit keywords args when calling initializers This actually proves the point even further: Gtk.Adjustment(1, 0, 6, 4, 5, 3) is not readable/understandable at all, and highly error prone. Gtk.Adjustment(value=1, lower=0, upper=6, step_increment=4, page_increment=5, page_size=3) is more verbose, but much more robust and comprehensible.
Review of attachment 251962 [details] [review]: Very nice, thanks! I would recommend applying this patch *before* the first one ("tests: Use explicit keywords args when calling initializers") so that we make sure that all the existing positional arguments still work. I munged this patch to apply on current trunk (but reverted 7193f05 before), and the tests all pass. I also tried running some existing PyGI apps against this, and there seems to be something wrong with labels: $ PYTHONPATH=. python -c 'from gi.repository import Gtk; Gtk.Label("content")' -c:1: PyGTKDeprecationWarning: Using positional arguments with initializers has been deprecated. Please specify keywords for: l. See: https://wiki.gnome.org/PyGObject/InitializerDeprecations Traceback (most recent call last):
+ Trace 232744
return super_init_func(self, **new_kwargs)
I can't see anything obviously wrong with the Gtk.Label constructor, but it looks like it would take the first element of the "label" string instead of the first element of the positional argumuent list? ::: gi/overrides/Gio.py @@ +65,3 @@ + __init__ = deprecated_init(Gio.Settings.__init__, + arg_names=('schema', 'path', 'backend')) How does this know that schema is a mandatory positional arg, and backend/path have "None" defaults? ::: gi/overrides/Gtk.py @@ +342,3 @@ + __init__ = deprecated_init(Gtk.Box.__init__, + arg_names=('homogeneous', 'spacing'), + category=PyGTKDeprecationWarning) I think this needs some update wrt. the argument type checking from commit 7193f050? If that's too cumbersome to do with this schema, please feel free to revert that commit. @@ +476,3 @@ + add_buttons = old_kwargs.get('buttons', None) + if add_buttons is not None and not isinstance(add_buttons, Gtk.ButtonsType): + warnings.warn('The "buttons" argument is reserved Gtk.ButtonsType enum values. ' bad grammar, perhaps: The "buttons" argument must be a Gtk.ButtonsType enum value"? ::: gi/overrides/__init__.py @@ +134,3 @@ + 'See: https://wiki.gnome.org/PyGObject/InitializerDeprecations' % + ', '.join(arg_names[:len(args)]), + category, stacklevel=stacklevel) I would extend this to "Using positional arguments with the GObject constructur has been deprecated. Please specify keywords for %s or use a class specific constructor", as with e. g. Gtk.Button.new_with_label() we of course continue to support positional arguments.
Comment on attachment 251965 [details] [review] Deprecate Gtk.TreePath constructor dispatching GtkTreePath is a leaf class, there is little possibility of confusing data types there, and the current overrides indeed make these much easier to use. So I'm less sure whether we should really remove these convenience syntaxes there. Technically the patch looks okay, but I'm not convinced we should really take this away.
Review of attachment 251962 [details] [review]: ::: gi/overrides/Gio.py @@ +65,3 @@ + __init__ = deprecated_init(Gio.Settings.__init__, + arg_names=('schema', 'path', 'backend')) This removes schema as a mandatory arg as we are just papering over what should be in GLib. Basically the idea is we shouldn't really be adding these to PyGObject but rather we should let GI/GObject provide an error (bug 649662). For path and backend they will be set to whatever the default for the property is (NULL and NULL): https://developer.gnome.org/gio/2.38/GSettings.html#GSettings.properties (constructor keyword args to g_object_newv are just property setting conveniences). ::: gi/overrides/Gtk.py @@ +342,3 @@ + __init__ = deprecated_init(Gtk.Box.__init__, + arg_names=('homogeneous', 'spacing'), + category=PyGTKDeprecationWarning) It looks like that only went into 3.10 and was already backed out? @@ +476,3 @@ + add_buttons = old_kwargs.get('buttons', None) + if add_buttons is not None and not isinstance(add_buttons, Gtk.ButtonsType): + old_kwargs = dict(zip(self._old_arg_names, args)) Nice catch. ::: gi/overrides/__init__.py @@ +134,3 @@ + 'See: https://wiki.gnome.org/PyGObject/InitializerDeprecations' % + ', '.join(arg_names[:len(args)]), + keyword is not explicitly passed. Sounds good.
> ::: gi/overrides/Gtk.py > @@ +342,3 @@ > + __init__ = deprecated_init(Gtk.Box.__init__, > + arg_names=('homogeneous', 'spacing'), > + category=PyGTKDeprecationWarning) > > It looks like that only went into 3.10 and was already backed out? Never mind. Yes, 7193f0509a will need to be backed out.
(In reply to comment #19) > I also tried running some existing PyGI apps against this, and there seems to > be something wrong with labels: > > $ PYTHONPATH=. python -c 'from gi.repository import Gtk; Gtk.Label("content")' > -c:1: PyGTKDeprecationWarning: Using positional arguments with initializers has > been deprecated. Please specify keywords for: l. See: > https://wiki.gnome.org/PyGObject/InitializerDeprecations > Traceback (most recent call last): Indeed, a tricky to see tuple bug: arg_names=('label'), should be: arg_names=('label',),
Created attachment 259653 [details] [review] Add deprecation warnings and cleanup class initializer overrides Updated based on review comments.
(In reply to comment #20) > (From update of attachment 251965 [details] [review]) > GtkTreePath is a leaf class, there is little possibility of confusing data > types there, and the current overrides indeed make these much easier to use. So > I'm less sure whether we should really remove these convenience syntaxes there. > > Technically the patch looks okay, but I'm not convinced we should really take > this away. Sounds reasonable at least for the time being. I think a lot of the convenience can already occur by using tuples as we already accept them with overrides for everything which takes a TreePath and bug 686261 would make this even better. But ideally GI should also provide a singular default constructor for struct types anyhow and if that ever happens I think would make a more convincing case for deprecating this.
Comment on attachment 259653 [details] [review] Add deprecation warnings and cleanup class initializer overrides Splendid, thank you! Looks good now and works fine.
Attachment 251563 [details] pushed as d2e9be8 - tests: Use explicit keywords args when calling initializers Attachment 251963 [details] pushed as 2f2069c - Cleanup overzealous new and init implementations Attachment 251964 [details] pushed as 7952018 - Deprecate Gdk.Cursor constructor dispatching Attachment 259653 [details] pushed as 86a37d6 - Add deprecation warnings and cleanup class initializer overrides
Most initializers have been marked as deprecated.
If you do things like this, you should do it right and be consistent across the library. For example, Gtk.FileChooserDialog still has now "new()" method, even though it shoudl, and what you have done through the buttons arguments breaks existing code.