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 586476 - Use accessor functions instead direct access
Use accessor functions instead direct access
Status: RESOLVED FIXED
Product: empathy
Classification: Core
Component: General
2.29.x
Other All
: Normal normal
: ---
Assigned To: empathy-maint
empathy-maint
Depends on: 69872 163251 585211 612409 616053
Blocks: 585391
 
 
Reported: 2009-06-20 16:26 UTC by Jonny Lamb
Modified: 2011-08-29 10:12 UTC
See Also:
GNOME target: 3.0
GNOME version: ---


Attachments
Use accessor functions instead direct access (20.09 KB, patch)
2009-09-03 23:57 UTC, Javier Jardón (IRC: jjardon)
reviewed Details | Review
Use accessor functions instead direct access.v2 (17.82 KB, patch)
2009-11-13 05:33 UTC, Javier Jardón (IRC: jjardon)
committed Details | Review
Use accessor functions instead direct accessv. Second patch (1.95 KB, patch)
2010-02-11 02:15 UTC, Javier Jardón (IRC: jjardon)
reviewed Details | Review
Use accessor functions instead direct accessv. Second patch.v2 (2.15 KB, patch)
2010-04-06 19:25 UTC, Javier Jardón (IRC: jjardon)
none Details | Review

Description Jonny Lamb 2009-06-20 16:26:52 UTC
See http://live.gnome.org/GnomeGoals/UseGseal for more details
Comment 1 Jonny Lamb 2009-06-20 16:29:23 UTC
See the following branch:

http://git.collabora.co.uk/?p=user/jonny/empathy.git;a=shortlog;h=refs/heads/gseal
Comment 2 Jonny Lamb 2009-06-21 21:11:06 UTC
So, I cocked it up originally and should've re-run configure.

My branch contains an updated commit which half fixes this problem.

I say half fixes for the following reasons:

 * There are no GtkWidget allocation accessors: see bug #585211
 * There are no GtkObject flag accessors: see bug #562937
Comment 3 Sjoerd Simons 2009-07-10 12:21:30 UTC
looks good
Comment 4 Jonny Lamb 2009-07-10 12:26:08 UTC
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.
Comment 5 Jonny Lamb 2009-07-10 14:38:23 UTC
Oops. I merged my branch, but this bug isn't fixed.
Comment 6 Javier Jardón (IRC: jjardon) 2009-09-03 23:57:12 UTC
Created attachment 142443 [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_TEXT_VIEW ()->vadjustment
GTK_TEXT_VIEW ()->need_im_reset
view->im_context
GTK_WIDGET_REALIZED ()
GTK_WIDGET_NO_WINDOW ()
GTK_WIDGET_MAPPED ()
Comment 7 Javier Jardón (IRC: jjardon) 2009-09-08 23:20:26 UTC
About ->im_context seems that is not part of the public api ( see bug #163251 ).
The solution should be fix the input method in question.
About if input methods can prevent keypress signals from happening: "input method get a turn at handling key events, but that happens inside the keypress/release signal handler"
So sjoerd propose this:
<sjoerd> jjardon: i guess if we use _connect_after things should work properlay
Comment 8 Guillaume Desmottes 2009-10-27 10:59:02 UTC
Review of attachment 142443 [details] [review]:

Thanks for your bug report and sorry for the late review.

I don't like all these if GTK_CHECK_VERSION. As GTK+ 2.18 has been released and is now packaged in major distro, I'd prefer to depend on this version and remove all the checks.
Comment 9 Javier Jardón (IRC: jjardon) 2009-11-13 05:33:42 UTC
Created attachment 147636 [details] [review]
Use accessor functions instead direct access.v2

Here a new patch rebased from master.
There are not #ifdef in the code as you requested.

I've used all the available API in GTK+ 2.19.0

Still pending:
GTK_TEXT_VIEW ()->vadjustment
GTK_TEXT_VIEW ()->need_im_reset
view->im_context (It's really a bug, as commented above)
GTK_WIDGET_REALIZED ()
GTK_WIDGET_MAPPED ()
Comment 10 Xavier Claessens 2009-11-13 07:57:38 UTC
We probably won't depend on GTK 2.19 before Empathy 2.31.x. I think your patch should only focus on API available in GTK 2.18 so we can already commit that part. And this bug can be continued later.
Comment 11 Guillaume Desmottes 2009-11-13 10:10:00 UTC
I agree with Xavier on this. I'd be happy to merge a patch using only GTK+ 2.18 API for now and merge the remaining bits later.
Comment 12 Javier Jardón (IRC: jjardon) 2009-11-13 19:15:41 UTC
Oh, sorry

I don't use any function that needs GTK+ > 2.18. (No need to bump the required GTK+ version in this patch)
I only mention 2.19 to indicate the functions that are still missing in GTK+ and can not be substituted.

Sorry for the misunderstanding
Comment 13 Danielle Madeley 2009-11-16 23:55:45 UTC
Comment on attachment 147636 [details] [review]
Use accessor functions instead direct access.v2

Thanks for this patch. Committed to master.

Shall we leave this bug open to track the API that will come in GTK+ 2.19 ? (Perhaps we should have a branch for that?)
Comment 14 Guillaume Desmottes 2009-11-17 10:16:19 UTC
Do we already know the bits we'll have to change with 2.19?

Also, what do you think about building Empathy with GSEAL_ENABLE to be sure we don't re-introduce direct accesses?
Comment 15 Javier Jardón (IRC: jjardon) 2010-02-11 02:15:35 UTC
Created attachment 153498 [details] [review]
Use accessor functions instead direct accessv. Second patch

This patch substitute GTK_WIDGET_MAPPED() and GTK_WIDGET_REALIZED().

I've use #ifdef, so no version bump required
Comment 16 Javier Jardón (IRC: jjardon) 2010-02-11 02:22:28 UTC
For the record, still pending:

- GTK_TEXT_VIEW ()->im_context
It's really a bug, as commented above

- GTK_TEXT_VIEW ()->need_im_reset
see blocker bug #163251

- GTK_TEXT_VIEW ()->vadjustment:
The affected code is this: [1]
gtk-demo has an example of keeping a text view scrolled to the bottom:
Scroll to the end of the buffer [2] or scroll to the bottom of the buffer [3]

[1] http://git.gnome.org/browse/empathy/tree/libempathy-gtk/empathy-chat-text-view.c#n498
[2] http://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/textscroll.c#n11
[3] http://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/textscroll.c#n58
Comment 17 Guillaume Desmottes 2010-02-11 10:26:34 UTC
Comment on attachment 153498 [details] [review]
Use accessor functions instead direct accessv. Second patch

Thanks for working on this Javier. I'm not a huge fan of #if GTK_CHECK_VERSION so I'd prefer to wait that we bumps the GTK+ dep. We need it for other things (such as bug #598446) so it will happen during this cycle.
Comment 18 Javier Jardón (IRC: jjardon) 2010-04-06 19:25:38 UTC
Created attachment 158074 [details] [review]
Use accessor functions instead direct accessv. Second patch.v2

Here an updated patch without ifdefs
Comment 19 Xavier Claessens 2010-04-14 07:33:32 UTC
(In reply to comment #7)
> About if input methods can prevent keypress signals from happening: "input
> method get a turn at handling key events, but that happens inside the
> keypress/release signal handler"
> So sjoerd propose this:
> <sjoerd> jjardon: i guess if we use _connect_after things should work properlay

For the record, this does not work, since if we let the GtkTextView's implementation run first, then ENTER is interpreted as newline and our _connect_after callback is never called.
Comment 20 Xavier Claessens 2010-05-05 07:33:40 UTC
GTK+ master now has the needed API, here is an empathy branch that use it:
http://git.collabora.co.uk/?p=user/xclaesse/empathy.git;a=shortlog;h=refs/heads/gseal

Disclaimer: I don't have gtk master installed, so I didn't even compiled that code, but it should be fine. We can merge it when we decide to depend on GTK 2.22
Comment 21 Guillaume Desmottes 2010-05-05 13:08:32 UTC
Please record the configure.ac change in a separated commit.
Comment 22 André Klapper 2010-05-27 19:41:08 UTC
So can that branch be merged?
Comment 23 Danielle Madeley 2010-06-11 07:09:17 UTC
Split the configure.ac changes out from the rest of the changes.
Comment 24 Danielle Madeley 2010-06-11 07:52:38 UTC
Updated to include sealed GdkDragContext members.
Comment 26 Guillaume Desmottes 2010-06-11 09:02:16 UTC
You should depend on  2.21.2, 2.21.3 doesn't exist yet.
Comment 27 Danielle Madeley 2010-06-12 00:07:40 UTC
My mistake. Branch updated. Can this be merged?
Comment 28 Guillaume Desmottes 2010-06-15 11:12:59 UTC
Yeah go for it.
Comment 29 Danielle Madeley 2010-06-15 11:45:17 UTC
Done.