GNOME Bugzilla – Bug 737125
java-atk-wrapper still uses the gtk 2.0 atk-bridge module
Last modified: 2015-02-27 16:54:22 UTC
Java atk wrapper is still using the atk-bridge module: commit 209e429ca60fbc1ba51e56f3ba99342a25faaaa2 Author: Ke Wang <kewang@sun.com> Date: Thu May 13 12:13:57 2010 +0800 Use GTK_PATH to locate libatk-bridge.so Although the module is still available in order to keep giving support to GTK2 applications, right now the way to go is calling the method atk_bridge_adaptor_init. Additionally, that would allow to remove most of the current gtk references from the code.
Thanks for filing this. Just to add some information: The patch in question: https://git.gnome.org/browse/java-atk-wrapper/commit/?id=209e429ca60fbc1ba51e56f3ba99342a25faaaa2 atk_bridge_adaptor_init: https://git.gnome.org/browse/at-spi2-atk/tree/atk-adaptor/bridge.c#n972
Created attachment 290761 [details] [review] Patch to address problem I think this is the right way to go but just to be safe, would you mind giving it a review Alejandro?
Review of attachment 290761 [details] [review]: Note: Im not in the position of testing the patch, so Im just reviewing it based on the patch. I will let Magdalen test it. ::: configure.ac @@ +44,3 @@ ]) +ATK_BRIDGE_LIB_PATH="`pkg-config --libs gtk-2.0`" As far as I see, you are replacing ATK_BRIDGE_LIB_PATH. I thought that it was only used to get the path of the bridge module. It is used in other place? Why not just remove it?
Created attachment 290855 [details] [review] updated patch Yeah I was just erring on the side of caution but it seems like removing the references seems to work ok. It does seem like there is a lot to do before this module is going properly functional, to be honest. The memory allocation seems to be out of date and of course, so is the API so it's a question of fixing those issues respectively, really.
I'm going to close this now. Thanks for the review!
(In reply to comment #3) > Review of attachment 290761 [details] [review]: > > Note: Im not in the position of testing the patch, so Im just reviewing it > based on the patch. I will let Magdalen test it. > > ::: configure.ac > @@ +44,3 @@ > ]) > > +ATK_BRIDGE_LIB_PATH="`pkg-config --libs gtk-2.0`" > > As far as I see, you are replacing ATK_BRIDGE_LIB_PATH. I thought that it was > only used to get the path of the bridge module. It is used in other place? Why > not just remove it? Hmm Removing the reference seems to get rid of the deprecated warnings which makes me a bit concerned. I am not sure that's a good thing.
Ok fixed that separately, so nothing more need be done here. https://bugzilla.gnome.org/show_bug.cgi?id=740275
On further investigation it seem this was not actually a bug which is why I think calling atk_bridge_adaptor_init inside the wrapper may be a large part of the cause for the initialisation hang seen in bug 742157. There was already been a check for gnome_accessibility_module_init which in turn calls atk_bridge_adaptor_init so on balance that seems like it should have been a sufficient way to do things.[1,2] On reflection, I don't think the GTK stuff should have been removed. I am going to proceed to fix bug 742157 with that in mind. [1] https://git.gnome.org/browse/java-atk-wrapper/tree/jni/src/AtkWrapper.c#n116 [2] https://git.gnome.org/browse/at-spi2-atk/tree/atk-adaptor/gtk-2.0/module.c#n48
(In reply to comment #8) > On further investigation it seem this was not actually a bug FWIW, when the bug was opened, it was not about solving something that was not working. It was a bug in the sense that loading the atk bridge using gmodule was being replaced. But as mentioned on the description, the old way was still working. GTK2 is still using it. In summary, the bug was about replacing a deprecating method to load the bridge, as most other modules ares doing. If Gtk2 didn't do that yet, is because it is in maintenance mode, and can't add new dependencies. Having said so ... > think calling atk_bridge_adaptor_init inside the wrapper may be a large part of > the cause for the initialisation hang seen in bug 742157. ... if this change is causing problems, it is not really problematic to just revert to the previous state, and do the change at any other moment. > There was already been a check for gnome_accessibility_module_init which in > turn calls atk_bridge_adaptor_init so on balance that seems like it should have > been a sufficient way to do things.[1,2] It is enough, but right now there are better ways to do that. The module approach was problematic and adds not-needed complexity. So if the change is reverted, instead of NOTABUG, the bug should just be reopened.
(In reply to comment #9) > (In reply to comment #8) > > On further investigation it seem this was not actually a bug > > FWIW, when the bug was opened, it was not about solving something that was not > working. It was a bug in the sense that loading the atk bridge using gmodule > was being replaced. But as mentioned on the description, the old way was still > working. GTK2 is still using it. > > In summary, the bug was about replacing a deprecating method to load the > bridge, as most other modules ares doing. If Gtk2 didn't do that yet, is > because it is in maintenance mode, and can't add new dependencies. > > Having said so ... So you are saying that gnome_accessibility_module_init is being deprecated?
Created attachment 293622 [details] [review] initialise using gnome_accessibility_module_init That didn't help fix the bug in the end, but I have put it back for the time being because I am pretty sure the previous implementation was wrong and I don't want that to further confuse the debug experience. Essentially, I don't really get why there's a need to call atk_bridge_adaptor_init unless the call to gnome_accessibility_module_init is removed (and in that last patch it wasn't). I can am happy to reopen this patch but I will have to need-info it because the relationship between the gnome_accessibility_module_init call and the gtk setup around it aren't totally clear to me.
s/patch/bug :-)
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > On further investigation it seem this was not actually a bug > > > > FWIW, when the bug was opened, it was not about solving something that was not > > working. It was a bug in the sense that loading the atk bridge using gmodule > > was being replaced. But as mentioned on the description, the old way was still > > working. GTK2 is still using it. > > > > In summary, the bug was about replacing a deprecating method to load the > > bridge, as most other modules ares doing. If Gtk2 didn't do that yet, is > > because it is in maintenance mode, and can't add new dependencies. > > > > Having said so ... > > So you are saying that gnome_accessibility_module_init is being deprecated? In the practice, yes. After all the discussion about setting the accessibility by default, and avoid to enable the accessibility as a plugin (as gmodule is basically a plugin-support library), it was agreed that the best/more straightforward way to enable accessibility (equivalent to load the atk bridge) was calling directly the libray. This was done on gtk 3.0 and gnome-shell for example. It was still maintained the support for module-loading (so the gnome_accessibility_module_init) for gtk 2.0 for two reasons: * gtk 2.0 was in maintenance mode, and using the library would need a new dependency * accessibility by default was possible in part thanks to the improvements to the accessibility libraries and implementations. But most of the work on gtk was done on gtk 3.0 (again, gtk 2.0 was in maintenance mode). So it was safer to keep loading the accessibility support conditionally on gtk 2.0. So as gnome_accessibility_module_init is intended to be used only for gtk 2.0 applications. I personally thought that having the the source code placed on the directory atk-adaptor/gtk-2.0, and installed on the directory <lib>/gtk-2.0/modules/ was enough to make it clear that was intended for gtk 2.0 applications only. But we could add some documentation if needed, for example a README under atk-adaptor/gtk-2.0.
(In reply to comment #11) > Created an attachment (id=293622) [details] [review] > initialise using gnome_accessibility_module_init > > That didn't help fix the bug in the end, but I have put it back for the time > being because I am pretty sure the previous implementation was wrong and I > don't want that to further confuse the debug experience. Well, after a quick look, not exactly wrong, just incomplete (see below). > Essentially, I don't really get why there's a need to call > atk_bridge_adaptor_init unless the call to gnome_accessibility_module_init is > removed (and in that last patch it wasn't). I agree. Looking at this: https://git.gnome.org/browse/java-atk-wrapper/tree/jni/src/AtkWrapper.c#n118 There are still some module stuff, that you are right that was not handled on your first patch. In summary, your first patch removed the code to locate and load the module. But the code to call a specific method on that module was still available. As you are saying, that code should be removed too. In summary, the call to atk_bridge_adaptor_init should replace loading the gtk 2.0 bridge module with gmodule (done with the patch) and calling gnome_accessibility_module_init (not handled) > I can am happy to reopen this patch but I will have to need-info it because the > relationship between the gnome_accessibility_module_init call and the gtk setup > around it aren't totally clear to me. I hope to have clarified the situation with my last two comments. I will set the bug to ASSIGNED. Don't hesitate on asking any new question if needed.
Created attachment 293863 [details] [review] Patch to address problem Thanks for clarifying. That helped. I have pushed the attached patch to update things since that seems to test out fine. Again, I've been a little cautious because I am not totally certain whether it is safe to remove the jaw_idle_dispatch stuff but I think it's fair to assume if that needs to be done, it doesn't need to be dealt with right away. [1] https://git.gnome.org/browse/java-atk-wrapper/tree/jni/src/AtkWrapper.c#n86
Sorry, I should have closed this already.
[Moving at-spi/java-atk-wrapper bugs to separate product. See bug 740075]