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

GTK+ 2.17.10 is now the required version
I've used all the GTK+ 2.17.11 api available, still missing:
GTK_WIDGET_SET_FLAGS (widget, GTK_MAPPED)
GTK_WIDGET_MAPPED (container)
GTK_WIDGET_REALIZED (widget)
GtkTextView->need_im_reset
GTK_COLOR_SELECTION_DIALOG (dialog)->ok_button
GTK_COLOR_SELECTION_DIALOG (dialog)->cancel_button
widget->requisition
GTK_CELL_RENDERER(cellprogress)->mode

GtkTextView->im_context, but this should not be used, see bug #163251

For

GTK_COLOR_SELECTION_DIALOG (dialog)->ok_button
GTK_COLOR_SELECTION_DIALOG (dialog)->cancel_button

they should be using ::response, not connecting to the clicked signal of the buttons, and connecting to the ::destroy as well

GTK_CELL_RENDERER(cellprogress)->mode can be substituted with g_object_set ()

Thanks a lot for this great work. As soon as we branches for 2.30, I'll commit the patch and follow the pointers you mention in comment #2 to complete your work.

Again thanks.

I started to work on that and committed a few patches to fix comment #2.
I'm waiting for fedora 12 to be a bit more stable to switch to the new stable version of GTK+ and I'll commit your patch.

Created attachment 143904 [details] [review]
Use accesor functions instead direct access.v2

Hello Philippe, 

Great! here a new patch rebased from master.

I'm sorry, but I've reverted some of your latest changes ;)
There are an accesor for padding and color-selection, so I think that is not necessary to use g_object_get/set for these properties.

Regards

(In reply to comment #5)

> I'm sorry, but I've reverted some of your latest changes ;)

no problem ;). Again thanks for your patch and sorry for not committing it right now, I'm still waiting for fedora 12 to become more stable.

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

Here a new rebased patch from master ;)

Only missing (no Gtk+ api yet):

GTK_WIDGET_SET_FLAGS (widget, GTK_MAPPED)
GTK_WIDGET_MAPPED (container)
GTK_WIDGET_REALIZED (widget)
GtkTextView->need_im_reset

Thanks a lot for your patches. I finally installed fedora 12 and committed your latest patch to master.
Reading what your last reply makes me think I should not close this bug; right?

Right,

As soon as the new api were available I'll make a new patch to fix this bug 
;)

Thank you for apply the patch.

Created attachment 153490 [details] [review]
Use accessor functions instead direct accessv. Second patch

Substitute GTK_WIDGET_REALIZED() and GTK_WIDGET_MAPPED()

Hello Philippe, could this get a review?

At least, you should apply this change because the actual code is wrong:

-	gtk_widget_set_style (widget, gtk_style_attach (gtk_widget_get_style (widget), window));
+	gtk_widget_style_attach (widget);

Javier i'm sorry for taking so long to review this patch but i just committed Andre patch[1] to git master so it makes this bug obsolete.

Thanks!

1 - https://bugzilla.gnome.org/show_bug.cgi?id=612293

Hello Luis,

no problem, but this bug is still not fixed ;)
Still remaining:

GtkTextView->need_im_reset (see blocker bug #163251)
GtkTextTag accessors in brasero-jacket-buffer.c (This one was recently added)

Ok please notify me about the new accessors then i'll cook a patch. I'll try to get GtkTextTag soon on brasero-jacket-buffer

Just a reminder for this one too:
  CC     brasero-io.lo
brasero-io.c: In function ‘brasero_io_xid_for_metadata’:
brasero-io.c:2403: error: ‘GtkWidget’ has no member named ‘window’

Just fixed the last one in git master, it's still missing the GtkTextView and GtkTextTag

(In reply to comment #16)
> it's still missing the GtkTextView and GtkTextTag

Do accessor functions exist for this in GTK+?
If not a bug should be filed and marked as blocking this report and bug 597610.

For the record:

The GtkTextView->need_im_reset use case:

http://git.gnome.org/browse/brasero/tree/libbrasero-utils/brasero-jacket-view.c#n676

The GtkTextTag use case:

http://git.gnome.org/browse/brasero/tree/libbrasero-utils/brasero-jacket-buffer.c#n70


Seems that brasero copied some code from GtkTextView internals. The question here is: why is brasero shipping a reimplementation of gtktextview ?

It's not really a reimplementation is more a tweaked textview for brasero own need.
I will fix this last bites after GTK+ unstable release. Also i plan to port to gsettings around that time too.

It turned out we did not need GtkTextView->need_im_reset after all and so I removed it.
I also rewrote the offending function that "summed up" a list of GtkTextTag.
I finally removed some GTK_WIDGET_REALIZED lying around at the same time.

So all in all we should be nearing completion for this bug. I'm just waiting for your green light before closing this bug for good (hopefully =) ).

@Javier: as for your question. Brasero is not re-implementing anything it just extends the current GtkTreeView to allow setting images or colors in the background which required a few tricks.
The specific code you mentions is used to know what the values/properties (fg/bg color, size, ...) are for the text where the cursor is and then update the toolbar buttons. We need to "sum up" all tags applied to this portion of text to get what the values are. At the time I wrote the code, I could not find any function that did that for me so I "stole" it.

Created attachment 160879 [details] [review]
Use accessor functions instead direct accessv.Final patch

Hey Philippe, one last patch ;)

I think you can close this bug after apply this patch. Also, I've added a -DGSEAL_ENABLE to configure.in to not use direct access again.

Wee! :)

Review of attachment 160879 [details] [review]:

Feel free to commit to git master, just make sure we have the right GTK+ version in the requirements.

Comment on attachment 160879 [details] [review]
Use accessor functions instead direct accessv.Final patch

commit 7d79bbdf0548f1180f85c1713b4bba48c2d54dd9

This problem has been fixed in the development version. The fix will be available in the next major software release. 

Congrats!