After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 594716 - Use accessor functions instead direct access (use GSEAL GnomeGoal)
Use accessor functions instead direct access (use GSEAL GnomeGoal)
Status: RESOLVED FIXED
Product: brasero
Classification: Applications
Component: general
git master
Other All
: Normal normal
: 2.26
Assigned To: Brasero maintainer(s)
Brasero maintainer(s)
Depends on: 69872 163251 596019
Blocks: 585391
 
 
Reported: 2009-09-10 05:57 UTC by Javier Jardón (IRC: jjardon)
Modified: 2010-05-12 00:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Use accessor functions instead direct acces (67.48 KB, patch)
2009-09-10 06:02 UTC, Javier Jardón (IRC: jjardon)
none Details | Review
Use accesor functions instead direct access.v2 (67.48 KB, patch)
2009-09-24 11:41 UTC, Javier Jardón (IRC: jjardon)
none Details | Review
Use accessor functions instead direct access.v3 (66.60 KB, patch)
2009-11-04 06:26 UTC, Javier Jardón (IRC: jjardon)
committed Details | Review
Use accessor functions instead direct accessv. Second patch (4.95 KB, patch)
2010-02-10 23:18 UTC, Javier Jardón (IRC: jjardon)
none Details | Review
Use accessor functions instead direct accessv.Final patch (3.58 KB, patch)
2010-05-11 23:16 UTC, Javier Jardón (IRC: jjardon)
committed Details | Review

Description Javier Jardón (IRC: jjardon) 2009-09-10 05:57:07 UTC
To be ready for GNOME 3 should be able to build with -DGSEAL_ENABLE

See http://live.gnome.org/GnomeGoals/UseGseal for more details
Comment 1 Javier Jardón (IRC: jjardon) 2009-09-10 06:02:20 UTC
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
Comment 2 Javier Jardón (IRC: jjardon) 2009-09-10 16:35:01 UTC
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 ()
Comment 3 Philippe Rouquier 2009-09-14 18:06:30 UTC
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.
Comment 4 Philippe Rouquier 2009-09-24 10:55:58 UTC
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.
Comment 5 Javier Jardón (IRC: jjardon) 2009-09-24 11:41:56 UTC
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
Comment 6 Philippe Rouquier 2009-09-26 13:06:13 UTC
(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.
Comment 7 Javier Jardón (IRC: jjardon) 2009-11-04 06:26:37 UTC
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
Comment 8 Philippe Rouquier 2009-11-22 20:00:42 UTC
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?
Comment 9 Javier Jardón (IRC: jjardon) 2009-11-22 23:19:13 UTC
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.
Comment 10 Javier Jardón (IRC: jjardon) 2010-02-10 23:18:49 UTC
Created attachment 153490 [details] [review]
Use accessor functions instead direct accessv. Second patch

Substitute GTK_WIDGET_REALIZED() and GTK_WIDGET_MAPPED()
Comment 11 Javier Jardón (IRC: jjardon) 2010-03-13 05:20:01 UTC
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);
Comment 12 Luis Medinas 2010-03-15 01:22:31 UTC
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
Comment 13 Javier Jardón (IRC: jjardon) 2010-03-15 11:19:07 UTC
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)
Comment 14 Luis Medinas 2010-03-15 12:36:09 UTC
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
Comment 15 Luis Medinas 2010-04-20 21:05:26 UTC
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’
Comment 16 Luis Medinas 2010-04-20 21:36:52 UTC
Just fixed the last one in git master, it's still missing the GtkTextView and GtkTextTag
Comment 17 André Klapper 2010-05-10 18:22:54 UTC
(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.
Comment 18 Javier Jardón (IRC: jjardon) 2010-05-10 20:52:25 UTC
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 ?
Comment 19 Luis Medinas 2010-05-10 21:11:51 UTC
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.
Comment 20 Philippe Rouquier 2010-05-11 19:04:43 UTC
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.
Comment 21 Javier Jardón (IRC: jjardon) 2010-05-11 23:16:43 UTC
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! :)
Comment 22 Luis Medinas 2010-05-12 00:05:38 UTC
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 23 Javier Jardón (IRC: jjardon) 2010-05-12 00:34:17 UTC
Comment on attachment 160879 [details] [review]
Use accessor functions instead direct accessv.Final patch

commit 7d79bbdf0548f1180f85c1713b4bba48c2d54dd9
Comment 24 Javier Jardón (IRC: jjardon) 2010-05-12 00:35:37 UTC
This problem has been fixed in the development version. The fix will be available in the next major software release. 

Congrats!