Created attachment 142095 [details] [review]
Use accessor functions instead direct access

I've used all the available GTK+api up to actual master (2.17.10 version)

Remaining functions: 

GTK_WIDGET_REALIZED
GTK_MENU_SHELL (option_menu->menu)->children
GTK_NOTEBOOK (notebook)->first_tab
GTK_WIDGET_MAPPED (GTK_WIDGET (tab))
GTK_WIDGET_SET_FLAGS (GTK_WIDGET (spinner), GTK_NO_WINDOW)
GTK_WIDGET_SET_FLAGS (widget, GTK_HAS_FOCUS);
text_view->need_im_reset

This means we have to bump gtk to 2.17.10? If so I'd prefer to keep this patch in bugzilla for now because of win32.

(In reply to comment #2)
> This means we have to bump gtk to 2.17.10? If so I'd prefer to keep this patch
> in bugzilla for now because of win32.

No, I've used #if GTK_CHECK_VERSION for functions that require a version of GTK+ >= 2.16, so no bump required

Some of these are already there:

GTK_MENU_SHELL (option_menu->menu)->children
-> gtk_container_get_children()

GTK_WIDGET_SET_FLAGS (GTK_WIDGET (spinner), GTK_NO_WINDOW)
-> gtk_widget_set_has_window()

GTK_WIDGET_SET_FLAGS (widget, GTK_HAS_FOCUS);
-> you should never need to use this

Created attachment 142119 [details] [review]
Use accessor functions instead direct acces

Updated patch.

I've substituted GTK_MENU_SHELL (option_menu->menu)->children and GTK_WIDGET_SET_FLAGS (GTK_WIDGET (spinner), GTK_NO_WINDOW) with Michael suggestions.

I'm not very sure how to remove GTK_WIDGET_SET_FLAGS (widget, GTK_HAS_FOCUS);
Here the code: http://git.gnome.org/cgit/gedit/tree/gedit/gedit-view.c#n932

Remaining functions:

GTK_WIDGET_REALIZED
GTK_NOTEBOOK (notebook)->first_tab
GTK_WIDGET_MAPPED (GTK_WIDGET (tab))
text_view->need_im_reset

Is it asking too much, add a gedit-gtk-compat.[ch]? where we add all new gtk funcs so we don't need to ifdef everywhere.
Another thing is that you should use some local variables if you call the same func more than once.

About the focus thing: I've filed a bug for it #593671

(In reply to comment #6)
> Is it asking too much, add a gedit-gtk-compat.[ch]?

Could such a proposal also cover bug 562052?

(In reply to comment #7)
> (In reply to comment #6)
> > Is it asking too much, add a gedit-gtk-compat.[ch]?
> 
> Could such a proposal also cover bug 562052?

Andre, the main problem of that bug is the ui decision that we have to make. We are not sure yet if we want to use GtkComboBox or we want to use our own widget for that.

I have committed part of the patch with some fixes (in particular be careful that gtk_container_get_children returns a list that must be freed).

I left out all the ifdefed parts. As nacho said, I'd really prefer that we provide a compat header file with the cut and pasted implementations of those functions instead of having ifdefs everywhere. Or we can wait to add those accessor when we bump gtk requirements.

Created attachment 142321 [details] [review]
Use accessor functions instead direct access. part2

I've created a gtk-compat-file.[h|c] as suggested here.

Please test as I don't have a GTK+ version <= 2.17.10 here.

Regards

At a quick look I found this:

-	
-		if (!GTK_WIDGET_TOPLEVEL (toplevel))
-			toplevel = NULL;	
+
+		if (gtk_widget_is_toplevel (toplevel))
+			toplevel = NULL;

I think it should be !gtk_widget_is_toplevel


-	GTK_WIDGET_SET_FLAGS (secondary_label, GTK_CAN_FOCUS);
+        gtk_widget_set_can_focus (secondary_label, TRUE);

Please use a tab instead of spaces there

-	GTK_WIDGET_SET_FLAGS (button, GTK_CAN_DEFAULT);
+        gtk_widget_set_can_default (button, TRUE);

Same here

Created attachment 142322 [details] [review]
Use accessor functions instead direct access.part2.v2

Solved the previous mentioned problems

Created attachment 142323 [details] [review]
Use accessor functions instead direct access.part2.v2

Oops, here the correct patch

patch looks good to me, the only comment I have on the code is:

there are two or three places where you do:

-	if (GTK_WIDGET_VISIBLE (origin->priv->side_panel))
+	if (gtk_widget_get_visible (origin->priv->side_panel))
 		gtk_widget_show (window->priv->side_panel);
 	else
 		gtk_widget_hide (window->priv->side_panel);

it should be changed to simply use "gtk_widget_set_visible()" instead of "if visible show else hide"




However a couple of bigger doubts came to my mind:
1 - what happens if you compile gedit with a version of gtk that doesn't have a symbol and then you upgrade your gtk to a version with that symbol? there would be two definitions of gtk_foo_bar()... does the dynamic linker manage that or crashes?

2 - should the ifdefs be just in the header or also in the c file?

Created attachment 142594 [details] [review]
Use accessor functions instead direct access.part2.v3

I've used gtk_widget_set_visible() as you requested

About your doubts, I'm not very sure either, sorry.

I think you can manage GTK_NOTEBOOK (notebook)->first_tab with:
gtk_notebook_get_nth_page

Created attachment 143535 [details] [review]
Get rid of NOTEBOOK->first_tab

Get rid of initial test completely in gedit-notebook.c function

Since gtk_notebook_get_nth_page, used just after that, returns NULL
for out-of-bounds pages.

Remaining functions (No GTK+ api exist):

GTK_WIDGET_REALIZED
GTK_WIDGET_MAPPED (GTK_WIDGET (tab))
text_view->need_im_reset

 
> GTK_WIDGET_SET_FLAGS (widget, GTK_HAS_FOCUS);
> -> you should never need to use this

Why? How do you set focus to a child widget in a derived container class?

Created attachment 151733 [details] [review]
Use accessor functions instead direct accessv3

I've used a compatibility header file: gseal-compat-gtk.h, the same file used in the rhythmbox patch: http://git.gnome.org/browse/rhythmbox/commit/?id=159fa13dee27c323a8872c0aec5cfa65b0176aae

Remaining functions (No GTK+ api exist):

GTK_WIDGET_REALIZED()
GTK_WIDGET_MAPPED()
text_view->need_im_reset

Also, these cases should be fixed:

GTK_WIDGET_SET_FLAGS (widget, GTK_HAS_FOCUS); -> as mitch said you should never need to use this
text_view->im_context (Should be fixed, It's not public api)

Created attachment 151734 [details] [review]
Use accessor functions instead direct accessv4

Sorry, I forgot to add to git the compatibility header. Here the correct patch

Hello, could this get a review?

Hi Javier, I know the compat header was my idea some months ago, but I recall we disussed on irc about it and we had a concern that no one was able to answer:

if a gedit binary is compiled with his own copy of gtk_foo_bar, but then gtk on the system is updated and provides gtk_foo_bar, how does the dynamic linker behaves for the conflicting symbols?

As you know just bumping gtk requirements is not an option since newer gtk on windows do not work properly :(

idfdef everywhere is just too ugly.



I'll hold off for now :(

(In reply to comment #23)
> Hi Javier, I know the compat header was my idea some months ago, but I recall
> we disussed on irc about it and we had a concern that no one was able to
> answer:
> 
> if a gedit binary is compiled with his own copy of gtk_foo_bar, but then gtk on
> the system is updated and provides gtk_foo_bar, how does the dynamic linker
> behaves for the conflicting symbols?
> 
> As you know just bumping gtk requirements is not an option since newer gtk on
> windows do not work properly :(
> 
> idfdef everywhere is just too ugly.
> 
> 
> 
> I'll hold off for now :(

You can use macros which conditionally use the older or newer interfaces. That's what I did in Midori to avoid #ifdefs for common functions. See http://git.xfce.org/apps/midori/commit/?id=f2f0f16dd2cfcfe08fc07715dbb7a314b98f9f1e

The patch already does that. Since they're #defines, not new functions, the compiled binary will in fact not reference these functions at all.

@pbor: ping. This looks fine to me. The compats are just defines so do not define any symbols. I say we can push this?

Ok, let's go for it

Created attachment 159799 [details] [review]
Use accessor functions instead direct access.v5

Updated patch against latest master

Only missing:
text_view->need_im_reset
text_view->im_context <- This should be removed as It's not public api, see comment 3 of bug #163251

This is the code:
/* There is no "official" way to reset the im context in GtkTextView */
static void
reset_im_context (GtkTextView *text_view)
{
        if (text_view->need_im_reset)
        {
                text_view->need_im_reset = FALSE;
                gtk_im_context_reset (text_view->im_context);
        }
}

Review of attachment 159799 [details] [review]:

This patch looks really good. Just 2 minor comments from my part.

::: gedit/gedit-notebook.c
@@ -251,3 @@
-		return AFTER_ALL_TABS;
-	{
-	if (GTK_NOTEBOOK (notebook)->first_tab == NULL)

Shouldn't this have some equivalent?

::: plugins/checkupdate/gedit-check-update-plugin.c
@@ +196,3 @@
 	gtk_misc_set_alignment (GTK_MISC (primary_label), 0, 0.5);
 	gtk_label_set_line_wrap (GTK_LABEL (primary_label), TRUE);
+	gtk_widget_get_can_focus (primary_label, TRUE);

This is wrong, it should be set

(In reply to comment #29)
> Review of attachment 159799 [details] [review]:
> 
> This patch looks really good. Just 2 minor comments from my part.
> 
> ::: gedit/gedit-notebook.c
> @@ -251,3 @@
> -        return AFTER_ALL_TABS;
> -    {
> -    if (GTK_NOTEBOOK (notebook)->first_tab == NULL)
> 
> Shouldn't this have some equivalent?

Nope, see bug #594537
Also, after discussion in the IRC, these line can be safely removed
 
> ::: plugins/checkupdate/gedit-check-update-plugin.c
> @@ +196,3 @@
>      gtk_misc_set_alignment (GTK_MISC (primary_label), 0, 0.5);
>      gtk_label_set_line_wrap (GTK_LABEL (primary_label), TRUE);
> +    gtk_widget_get_can_focus (primary_label, TRUE);
> 
> This is wrong, it should be set

Ops, fixed

Created attachment 159804 [details] [review]
Use accessor functions instead direct access.v6

Review of attachment 159804 [details] [review]:

Looks good to me.

Comment on attachment 159804 [details] [review]
Use accessor functions instead direct access.v6

commit d17185bb01fd3b89da52151bd2fbd5b8c3edeafb

gedit/gseal-gtk-compat.h was not added

(In reply to comment #34)
> gedit/gseal-gtk-compat.h was not added

Pushed in commit ec64bcf3c238ac28c3513a5e3fe876efadc79a7f

I've noticed that this commit and also other GSEAL commits in other modules do something like:

-  if (GTK_WINDOW (parent)->group)
-    gtk_window_group_add_window (GTK_WINDOW (parent)->group,
+  if (gtk_window_get_group (GTK_WINDOW (parent)))
+    gtk_window_group_add_window (gtk_window_get_group (GTK_WINDOW (parent)),

which is wrong, because gtk_window_get_group() always returns a valid pointer, if the window doesn't have a group, the default group is returned, see:

GtkWindowGroup *
gtk_window_get_group (GtkWindow *window)
{
  if (window && window->group)
    return window->group;
  else
    {
      static GtkWindowGroup *default_group = NULL;

      if (!default_group)
        default_group = gtk_window_group_new ();

      return default_group;
    }
}

So, in order to check whether the window has a group, we need to get the default group with gtk_window_get_group (NULL) and compare with the window group. Or maybe we can just add a new method to GtkWindow, gtk_window_get_has_group() or something like that.

Created attachment 160796 [details] [review]
Remove unneeded include of gtk/gtkoptionmenu.h

I've pushed a patch for the im_context.
So now we are missing the has_group thingie. Do we have any info about that?

(In reply to comment #36)
> So, in order to check whether the window has a group, we need to get the
> default group with gtk_window_get_group (NULL) and compare with the window
> group. Or maybe we can just add a new method to GtkWindow,
> gtk_window_get_has_group() or something like that.

So how to proceed? Is there a bug report for the latter to track (and this bug to depend on)?

AFAIK to close this, we just need a: gtk_window_get_has_group

gtk_window_get_has_group() is in gtk-2-22 branch and master branch now.

A new 2.21.x release will be availabel soon with the new api.

See bug #618271

Created attachment 162250 [details] [review]
Use gtk_window_has_group

Created attachment 162251 [details] [review]
Fix building with -DGSEAL_ENABLE

We should start building with -DGSEAL_ENABLE to avoid this.

Review of attachment 162250 [details] [review]:

Looks good

Review of attachment 162251 [details] [review]:

Looks good

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.