GNOME Bugzilla – Bug 618327
GtkNotebookPage should be deprecated
Last modified: 2010-07-13 15:07:58 UTC
+++ This bug was initially created as a clone of Bug #613132 +++ GtkNotebook page should also be deprecated: http://library.gnome.org/devel/gtk/unstable/GtkNotebook.html#GtkNotebookPage It's passed as a parameter of the switch-page signal but I don't see how anybody could be using it: http://library.gnome.org/devel/gtk/unstable/GtkNotebook.html#GtkNotebook-switch-page
I agree. It should be deprecated
"switch-page" signal should be deprecated too, then
No, the signal should stay, but that parameter should be removed from it.
Created attachment 162226 [details] [review] Deprecated GtkNotebookPage I deprecated GtkNotebook page and changed the argument in "switch-page" into a gpointer for now. I wonder if this should be a GtkWidget* in 2.90? That way, nothing will break unexpectedly and it might actually be useful.
Review of attachment 162226 [details] [review]: Makes sense to me in general. And making it a GtkWidget * sounds good to me. ::: gtk/gtknotebook.c @@ +6019,3 @@ gboolean child_has_focus; + if (notebook->cur_page == page || !child) Is this a behavior change here ? ::: gtk/gtknotebook.h @@ +67,3 @@ +#else + gpointer GSEAL (cur_page); +#endif In master, we should just go gpointer without ifdef, I guess.
How does making it a (null) GtkWidget* help? GtkNotebookPage* wasn't a GtkWidget, right, so nobody can be doing anything widgety with it.
(In reply to comment #11) > How does making it a (null) GtkWidget* help? GtkNotebookPage* wasn't a > GtkWidget, right, so nobody can be doing anything widgety with it. It is not NULL, it is actually the GtkWidget* contained in the page. But as it would be an ABI change, it can't actually be a GtkWidget* in 2.22. So the remaining option is to pass a pointer that is as opaque as GtkNotebookPage was.
(In reply to comment #10) > Review of attachment 162226 [details] [review]: > > Makes sense to me in general. And making it a GtkWidget * sounds good to me. > > ::: gtk/gtknotebook.c > @@ +6019,3 @@ > gboolean child_has_focus; > > + if (notebook->cur_page == page || !child) > > Is this a behavior change here ? No, just used the child variable directly since it is given as a function argument now. > ::: gtk/gtknotebook.h > @@ +67,3 @@ > +#else > + gpointer GSEAL (cur_page); > +#endif > > In master, we should just go gpointer without ifdef, I guess. The patch above is written with 2.22 in mind. In master we still need the real GtkNotebookPage* cur_page because gtknotebook.c uses it for internal book-keeping. Of course it would be hidden in a .c file or private header then.
(In reply to comment #13) > (In reply to comment #10) > > Review of attachment 162226 [details] [review] [details]: > > > > Makes sense to me in general. And making it a GtkWidget * sounds good to me. > > > > ::: gtk/gtknotebook.c > > @@ +6019,3 @@ > > gboolean child_has_focus; > > > > + if (notebook->cur_page == page || !child) > > > > Is this a behavior change here ? > > No, just used the child variable directly since it is given as a function > argument now. But the code used to check gtk_widget_get_visible, not it just checks != NULL. That smells like a behaviour change to me...
Created attachment 165129 [details] [review] Deprecated GtkNotebookPage #2 You are absolutely right, the quoting from the review confused me. I updated the patch.
Looks ok to me now, assuming you've tested that things still work with that.
I tested it, committed to gtk-2-22, (accidentally also master) and also change it in master to use GtkWidget* now.