set_default_widget() would be _better_, but I don't think
is worth a rename at the point.

The getter is a future milestone issue.

Copying from the GtkLove wiki:
"This and the latter should have been addressed in http://bugzilla.gnome.org/show_bug.cgi?id=55767 where using a script to find missing getters was suggested and functions not implemented were shown in http://mail.gnome.org/archives/gtk-devel-list/2001-June/msg00420.html"

Created attachment 107357 [details] [review]
20080315_bgo_65818_getter.diff: Implements the getter

Enough?

> + * Return value: the default_widget for the @window, or %NULL if there's none.

That should be "default widget", not "default_widget".

Also, may be better to call the getter get_default_widget() at least, and reference set_default() in the docs.

Patch also should hook it into docs.  Edit docs/reference/gtk/gtk-sections.txt.

I *think* you also need to edit gtk/gtk.symbols.

I know, it's a pain to add new API...

I called it get_default to be consistent with set_default. But I agree that _widget() would be much more obvious. How do we name it then?
I'll add a line about set_default in the docs. The "Since:" must be 2.13 or 2.14?

2.14

Created attachment 112347 [details] [review]
Update with behdad's comments

Created attachment 112348 [details] [review]
Update with behdad's comments

I don't think it's a good idea to add gtk_window_get_default. It looks like gdk_screen_get_default and gtk_settings_get_default but is something totally different.

Instead gtk_window_get_default_widget should be introduced along with a GtkWindow:default-widget property which is still missing.

I also suggest that gtk_window_set_default should be superseded by gtk_window_set_default_window, but that's sort of optional.

Created attachment 112424 [details] [review]
Implemented gtk_window_{get,set}_default and property

I took the liberty to implement my suggestions.

Note that I am not sure what the proper way is to deprecate a function, this may need to be adjusted.

Created attachment 112872 [details] [review]
Implemented gtk_window_{get,set}_default, take 2

Updated patch, with proper deprecation hint.

Looks good. But it is probably a good idea to try and get buy-in on the mailing list about the deprecation.

small nitpick in gtk_window_set_default() documentation:

+ * Deprecated: gtk_window_set_default() is deprecated and
+ *   should not be used in newly-written code. Use
+ *   gtk_window_set_default_widget() instead.

the blurb is not needed - gtk-doc will do it for you. the deprecation should look like:

+ * Deprecated: 2.14: Use gtk_window_set_default_widget() instead

Created attachment 115193 [details] [review]
Fix docs and remove wrong getter

I updated the doc comment.

Apparently during sealing gtk_window_get_default was added, so I updated the patch to also remove that again. Plus in two places where the old function is used in the same file, I replaced it with the new one.

Could this please be reviewed before 2.14 is frozen? The gtk_window_get_default from GSEAL is badly missnamed and we should not keep it only to deprecate it again in 2.16.

I'm kinda torn on the deprecation. It would be easier if deprecations would just spew warnings, instead of breaking the build. As Owen said 7 years ago:

set_default_widget() would be _better_, but I don't think
is worth a rename at the point.


Renaming the getter to gtk_window_get_default_widget() seems like the right thing to do, though. set_default_widget() would be _better_, but I don't think
is worth a rename at the point.

I can't quite follow the argumentation here. The fact that DISABLE_DEPRECATED style macros break builds is nothing particularly special in this case. Whoever turns them on knows what to expect. If you dislike the way this mechanism works, please address that, raise it in a meeting, but please don't reject api development only because of this.

Incidentally I have seen a patch to use gcc deprecation syntax to make deprecated functions warn without actually breaking the build. I'm not sure if they were in bugzilla, or just proposed somewhere else, I might try to find out if you are interested in this.

When you write "I don't think is worth a rename at the point" I read it as "I don't think it's currently a good idea". So when is a good point in time to fix badly named api? What is worthy of improvement?



That said...
for all that matters to me, we can defer the question of whether or not to deprecate gtk_window_set_default and only go for gtk_window_get_default_widget for this release. If you are okay with this, I'll make a separate patch just for this.

> If you dislike the way this mechanism
> works, please address that, raise it in a meeting, but please don't reject api
> development only because of this.

Blargh. Bugzilla is a valid communication channel for this kind of feedback, don't you think ? 

My concern is that this is exactly the kind of 'a new deprecation every week' style of 'api development' that application developers are complaining about. 

(In reply to comment #19)
> My concern is that this is exactly the kind of 'a new deprecation every week'
> style of 'api development' that application developers are complaining about. 

This bug is 7 years old, so it's not exactly every week. There are surely enough missnamers, but I don't plan to address all of them all of a sudden.

Anyways, your call, if you consider it a bad idea, I'm okay with fixing only the getter, which disturbs me considerably more than the setter.

Created attachment 115648 [details] [review]
Rename gtk_window_get_s/default/default_widget

This patch *only* renames the getter, nothing else.

Lets get the rename in before the release on Monday.

2008-08-02  Matthias Clasen  <mclasen@redhat.com>

        Bug 65818 – rename gtk_window_set_default() and add getter for it

        * gtk/gtk.symbols:
        * gtk/gtkwindow.[hc]: Rename gtk_window_get_default to
        gtk_window_get_default_widget. Patch by Christian Dywan