GNOME Bugzilla – Bug 586476
Use accessor functions instead direct access
Last modified: 2011-08-29 10:12:37 UTC
See http://live.gnome.org/GnomeGoals/UseGseal for more details
See the following branch: http://git.collabora.co.uk/?p=user/jonny/empathy.git;a=shortlog;h=refs/heads/gseal
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
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.
Oops. I merged my branch, but this bug isn't fixed.
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 ()
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
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.
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 ()
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.
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.
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 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?)
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?
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
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 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.
Created attachment 158074 [details] [review] Use accessor functions instead direct accessv. Second patch.v2 Here an updated patch without ifdefs
(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.
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
Please record the configure.ac change in a separated commit.
So can that branch be merged?
Split the configure.ac changes out from the rest of the changes.
Updated to include sealed GdkDragContext members.
http://git.collabora.co.uk/?p=user/danni/empathy.git;a=shortlog;h=refs/heads/gseal
You should depend on 2.21.2, 2.21.3 doesn't exist yet.
My mistake. Branch updated. Can this be merged?
Yeah go for it.
Done.