Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JBR-4578 Add Java ATK Wrapper #475

Open
wants to merge 38 commits into
base: jbr21
Choose a base branch
from
Open

Conversation

tanya011
Copy link

@tanya011 tanya011 commented Nov 6, 2024

  • java-atk-wrapper used to be part of OpenJDK (JDK-8204862, IJPL-136231), but it was removed.
  • java-atk-wrapper is essential for accessibility in Java applications, including IntelliJ IDEA: IJPL-58438, JBR-4578.
  • AccessibleAnnouncerTest.java test is passed.
  • java-atk-wrapper has an LGPL 2.1 license, which can be up-cast to GPL-2.0, allowing the source code to be used.

@OnePatchGuy OnePatchGuy self-requested a review November 6, 2024 13:45
@tanya011 tanya011 force-pushed the jbr21 branch 3 times, most recently from 4af7c31 to 081ca50 Compare November 19, 2024 16:56
@vprovodin
Copy link
Collaborator

All changes in JBR should be accompanied with tickets in YT related to the JBR project. Ticket-id should be placed at the beginning of the commit message. Within this JBR ticket we may make proper changes in docker files. Please note the libraries you aded to docker files actually are not available for Oracle Linux 8 that is build platform for jbr21.

@tanya011 tanya011 changed the title IJPL-58438 Add Java ATK Wrapper JBR-4578 Add Java ATK Wrapper Nov 28, 2024
Library to enable accessibility on gnome linux.
Copy link
Member

@OnePatchGuy OnePatchGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch! It's been a hard work, hasn't it? :)

I've done reviewing only the changes in the build scripts (everything under the make directory). All the questions and suggestions are below. Reviewing the rest is coming.

From now on, please don't rewrite the commits history so that I'll be able to observe only the changed parts in newly added commits (we'll later squash them during merging).

Comment on lines 123 to 127
NEEDS_LIB_ATK=true
NEEDS_LIB_AT_SPI2_ATK=true
NEEDS_LIB_GLIB=true
NEEDS_LIB_GOBJECT=true
NEEDS_LIB_GLIBCONFIG=true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch adds new build-time dependencies, but:

  1. Our CI doesn't respect them (yet), thus if we merge the patch as is, it'll immediately get broken. E.g. what I get if I try to make a build in a docker container as described in the README:

Package atspi-2 was not found in the pkg-config search path.
Perhaps you should add the directory containing 'atspi-2.pc'
to the PKG_CONFIG_PATH environment variable
Package 'atspi-2', required by 'virtual:world', not found
Creating support/modules_libs/jdk.accessibility/libatk-wrapper.so from 16 file(s)
/JetBrainsRuntime/src/jdk.accessibility/linux/native/libatk-wrapper/AtkWrapper.c:25:10: fatal error: glib.h: No such > file or directory
#include <glib.h>
^~~~~~~~
compilation terminated.

(I get the same on Ubuntu 22 before installing the dependencies).

I can suggest 2 following ways of solving the problem:

  • to make the corresponding changes to our build scripts, docker images, etc. and include them into this PR ;
  • to make the changes provided in this patch disabled by default (i.e. apply --disable-java-atk-wrapper by default) and deal with all this CI stuff later after merging the patch .

Anyway, please note @vprovodin noticed earlier that Oracle Linux 8 which we're using as a build platform (via a docker image) doesn't provide these new dependencies, so it may become a serious obstacle for this patch.

  1. The build instructions in README haven't been updated correspondingly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build instructions in README haven't been updated correspondingly.

I updated README, but not sure where to locate build instructions

Copy link
Member

@OnePatchGuy OnePatchGuy Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure where to locate build instructions

Our README is currently describing 2 ways of how to build a JBR for Linux:

  • Using a specially designed docker container - the chapter Linux (Docker)
  • Using an Ubuntu as a host system - the chapter Ubuntu Linux

For now there's probably nothing to change in the Docker chapter, but Ubuntu chapter should be supplemented with the new packages and instructions how to enable/disable ATK wrapper and how to pass custom paths to it. Examples could be Windows's chapters Enable optional NVDA screen reader support and Disable optional JAWS screen reader support.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at f83061c

make/autoconf/lib-at-spi2-atk.m4 Outdated Show resolved Hide resolved
make/autoconf/lib-atk.m4 Outdated Show resolved Hide resolved
@@ -113,7 +113,7 @@ jobs:
g++-${{ inputs.gcc-major-version }} \
gcc-${{ inputs.gcc-major-version }}-${{ matrix.gnu-arch }}-linux-gnu${{ matrix.gnu-abi}} \
g++-${{ inputs.gcc-major-version }}-${{ matrix.gnu-arch }}-linux-gnu${{ matrix.gnu-abi}} \
libxrandr-dev libxtst-dev libcups2-dev libasound2-dev
libxrandr-dev libxtst-dev libcups2-dev libasound2-dev libatspi2.0-dev libatk1.0-dev libglib2.0-dev
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libatk-bridge2.0-dev should also be added - the patch doesn't build without it on Ubuntu 22.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only partially done? #475 (comment)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at 3a9756d

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: here seems the full list of Ubuntu 22 packages that must be additionally installed to build the patch:

  • libatspi2.0-dev
  • libatk1.0-dev
  • libglib2.0-dev
  • libatk-bridge2.0-dev

make/autoconf/spec.gmk.in Outdated Show resolved Hide resolved
make/modules/jdk.accessibility/Lib.gmk Outdated Show resolved Hide resolved
-DATSPI_MINOR_VERSION=$(shell pkg-config --modversion atspi-2 | cut -d. -f2) \
-DATSPI_MICRO_VERSION=$(shell pkg-config --modversion atspi-2 | cut -d. -f3), \
LDFLAGS := $(LDFLAGS_JDKLIB), \
LIBS_linux := -ljvm $(ATK_LIBS) $(AT_SPI2_ATK_LIBS) $(GLIB_LIBS) $(GOBJECT_LIBS), \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding -ljava and/or -lawt as well if needed (I don't know).

@OnePatchGuy
Copy link
Member

OnePatchGuy commented Jan 15, 2025

My main question about the entire patch is the following: as you've written in the description, this functionality was removed some time ago due to a lot of bugs mentioned in e.g. IJPL-136231:

The problem is that the tool breaks the Swing threading rule and causes lots of exceptions & UI artefacts

and in JBR-4578:

Important! ATKWrapper contains bugs that can crash the JBR.

Also known issues:

  • Lists with complex renders work slowly;
  • Tree nodes are not voiced;
  • comboboxes are not spoken.

All these problems must be solved directly in ATKWrapper.

By putting all this logic back, do you state that all those problems have been fixed and now AWT wrapper is in production-ready quality?

UPD: still actual crash https://discourse.gnome.org/t/sigsegv-in-g-type-check-instance-is-fundamentally-a-0x11/24285

Copy link
Member

@OnePatchGuy OnePatchGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I seem to have done reviewing everything related to the Wrapper initialization.
Besides that, this "round" also contains a few comments about the resource management and some general notes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK since Java 9's modules, these files aren't needed as long as the provider is specified in the corresponding module-info.java. Anyway, the Windows' implementation of AccessibilityProvider (src/jdk.accessibility/windows/classes/com/sun/java/accessibility/internal/ProviderImpl.java) somehow works without such a file.

Could you please try to drop it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at 55629ed

@@ -0,0 +1,2 @@
exports org.GNOME.Accessibility;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is exporting actually required?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at 632683a

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's adjust the formatting of all the sources a bit according to the OpenJDK rules. Basically, there's a need to set:

  • Indentation - 4 columns
  • No tabs as indentation (there're a few places where they are used)

I believe it'll be enough to open the project generated by ./bin/idea.sh in IDEA and use the Code->Reformat Code/File actions

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at 6ae0668

Comment on lines 94 to 97
} catch (Exception e) {
e.printStackTrace();
e.getCause();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the logging should be done via sun.util.logging.PlatformLogger. You can find one of the most recent examples of how to use it in src/java.desktop/unix/classes/sun/awt/wl/WLToolkit.java.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at 04b8638

Comment on lines 1275 to 1277
jni_main_idle_add(key_dispatch_handler, (gpointer)global_key_event);
JAW_DEBUG_I("result saved = %d", key_dispatch_result);
if (key_dispatch_result == KEY_DISPATCH_CONSUMED)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I right that key_dispatch_handler will be invoked asynchronously on a background thread? If yes, I believe there have to be synchronization here.

Comment on lines 1162 to 1165
{
JAW_DEBUG_I("jniEnv == NULL");
return G_SOURCE_REMOVE;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few memory leaks are in this branch:

  • event doesn't get freed
  • the jAtkKeyEvent JNI global reference doesn't get deleted. It seems in this branch it's not possible to delete it. We can make the callers of the function to keep the ownership over the passed references, so that they delete them after this function returns.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GThread *thread;
GError *err;
const char * message;
message = "JNI main loop";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have a better describing name for the thread.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at 22f2e76

Comment on lines 741 to 770
if (toolkit.getSystemEventQueue() != null) {
toolkit.getSystemEventQueue().push(new EventQueue() {
boolean previousPressConsumed = false;

public void dispatchEvent(AWTEvent e) {
if (e instanceof KeyEvent) {
if (e.getID() == KeyEvent.KEY_PRESSED) {
boolean isConsumed = AtkWrapper.dispatchKeyEvent(new AtkKeyEvent((KeyEvent)e));
if (isConsumed) {
previousPressConsumed = true;
return;
}
} else if (e.getID() == KeyEvent.KEY_TYPED) {
if (previousPressConsumed) {
return;
}
} else if (e.getID() == KeyEvent.KEY_RELEASED) {
boolean isConsumed = AtkWrapper.dispatchKeyEvent(new AtkKeyEvent((KeyEvent)e));

previousPressConsumed = false;
if (isConsumed) {
return;
}
}
}

super.dispatchEvent(e);
}
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a real need to intercept all the key events? All the keyboard input gets dependent on third-party components' logic we don't control. In the worst cases we may be getting EDT freezes and the inability to use keyboards at all.

Also, I'm not certain how exactly the queues pushing/poping works, but this approach most likely displaces the IntelliJ's own EventQueue installed at https://github.com/JetBrains/intellij-community/blob/794ebfdcb71781022a1357533be9f5fc352408b0/platform/platform-impl/src/com/intellij/ide/IdeEventQueue.kt#L139 .

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is at least one key event that we need to dispatch: IJPL-164705, so we cannot remove it completely.

Yes, Intellij has its own EventQueue and to fix the issue above, we would like to add a new dispatcher using the JBR API: https://code.jetbrains.team/p/ij/repositories/ultimate/revision/a6886190ed6084964ab4b31f01b03b19ccf9c21d. (I will create a new pull request for the JBR API after this one is approved.) It is assumed that it will be execute the code from dispatchEvent(AWTEvent e) function.

This approach does not replace IntelliJ's queue, quite the opposite, but I think there is no guarantee that it will not. Can we remove the push of a new EventQueue in AtkWrapper() and delegate this responsibility to the swing project?

Copy link
Member

@OnePatchGuy OnePatchGuy Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is at least one key event that we need to dispatch: IJPL-164705

Could you please elaborate which one exactly? It's not clear from the issue description.
Also, if there's only a specific set of keystrokes, let's restrict the logic only to it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first, I thought the issue was only with pressing the right and left arrow keys, but on Ubuntu 22.04, orca doesn't announce any key events : caret movement, text typing, pressing delete, tab, etc. So, we need to process all key events.
I cannot reproduce this problem on Ubuntu 24.04. If we do not process key events, everything is announced. The difference is in the version of orca.

Also, is it true that we should not consume any events?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 0fd66b9

Comment on lines 188 to 191
{
JAW_DEBUG_I("Thread create failed: %s !", err->message);
g_error_free (err);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Do jni_main_context and jni_main_loop leak here?
  2. Shouldn't we undo the effect of jaw_accessibility_init performed a few lines above?
  3. Doesn't this case lead to impossibility of using the whole functionality? If yes, I think this case should end up with throwing a Java exception.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at 5ff70a6

@OnePatchGuy
Copy link
Member

BTW, I think each file that gets updated should also have its copyright header updated. One of the most recent examples of our header can be found e.g. in https://github.com/JetBrains/JetBrainsRuntime/blob/f89a6626d56dad71c3429c662bdad89f6b9a0591/src/java.desktop/unix/classes/sun/awt/wl/WLDisplay.java, but I don't know how to properly merge the existing headers with our one.

@@ -133,7 +133,7 @@ jobs:
sudo debootstrap
--arch=${{ matrix.debian-arch }}
--verbose
--include=fakeroot,symlinks,build-essential,libx11-dev,libxext-dev,libxrender-dev,libxrandr-dev,libxtst-dev,libxt-dev,libcups2-dev,libfontconfig1-dev,libasound2-dev,libfreetype-dev,libpng-dev
--include=fakeroot,symlinks,build-essential,libx11-dev,libxext-dev,libxrender-dev,libxrandr-dev,libxtst-dev,libxt-dev,libcups2-dev,libfontconfig1-dev,libasound2-dev,libfreetype-dev,libpng-dev,libatspi2.0-dev,libatk1.0-dev,libglib2.0-dev
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
--include=fakeroot,symlinks,build-essential,libx11-dev,libxext-dev,libxrender-dev,libxrandr-dev,libxtst-dev,libxt-dev,libcups2-dev,libfontconfig1-dev,libasound2-dev,libfreetype-dev,libpng-dev,libatspi2.0-dev,libatk1.0-dev,libglib2.0-dev
--include=fakeroot,symlinks,build-essential,libx11-dev,libxext-dev,libxrender-dev,libxrandr-dev,libxtst-dev,libxt-dev,libcups2-dev,libfontconfig1-dev,libasound2-dev,libfreetype-dev,libpng-dev,libatspi2.0-dev,libatk1.0-dev,libglib2.0-dev,libatk-bridge2.0-dev

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at 3a9756d

Comment on lines 80 to 83
ATK_WRAPPER_CFLAGS="$ATK_WRAPPER_CFLAGS $GLIB_CFLAGS $GLIBCONFIG_CFLAGS"
ATK_WRAPPER_LIBS="$ATK_WRAPPER_LIBS $GLIB_LIBS"
AC_SUBST(ATK_WRAPPER_CFLAGS)
AC_SUBST(ATK_WRAPPER_LIBS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO the proper responsibility area of each lib-...m4 is (or should be) to only provide all the required information for the buildsystem about the corresponding library. The advantages of this approach are especially clearly visible in case of GLib since it's a general purpose library. Let's imagine JBR will want to build another component (e.g. a library) using it. In that case the developer won't be able to reuse this file because it provides flags for building the atk wrapper and not flags for using GLib itself.

From this point of view, your initial approach, where you combined all the flags together in Libs.gmk, was great.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I returned it back. But what did you mean in this comment? #475 (comment)

When the custom paths flags (e.g. --with-atk-include) are used, the corresponding _CFLAGS and _LIBS flags should propagate the same dependency flags as PKG_CHECK_MODULES does for consistency. In this particular case we can just append $GLIB_CFLAGS to ATK_CFLAGS (and $GLIB_LIBS to ATK_LIBS if PKG_CHECK_MODULES does it, I haven't checked). Before doing so we may also check if GLIB_FOUND is true.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at 607f976

if test "x$AT_SPI2_ATK_FOUND" = xno; then
# Are the at-spi2-atk headers installed in the default /usr/include location?
PKG_CHECK_MODULES([AT_SPI2_ATK], [atk-bridge-2.0],
[AT_SPI2_ATK_FOUND=yes; AT_SPI2_ATK_VERSION=$(pkg-config --modversion atk-bridge-2.0)],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[AT_SPI2_ATK_FOUND=yes; AT_SPI2_ATK_VERSION=$(pkg-config --modversion atk-bridge-2.0)],
[AT_SPI2_ATK_FOUND=yes; AT_SPI2_ATK_VERSION=$("$PKG_CONFIG" --modversion atk-bridge-2.0)],

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done at 47171c4

1. loadAtkBridge() in static block

2. Changes in initNativeLibrary:
- return False instead of exit(1)
- throw an exception if initNativeLibrary returns false
1. event doesn't get freed
2. synchronization
/usr/local/include/glib-2.0/gobject/gtype.h:2732: note: this is the location of the previous definition
 2732 | #define GTYPE_TO_POINTER(t) ((gpointer) (guintptr) (t)) GOBJECT_AVAILABLE_MACRO_IN_2_80
1. return boolean to throw an exception if something is wrong
2. unref jni_main_loop and jni_main_context if the thread is null
3. undo the effect of jaw_accessibility_init performed a few lines above
4. call initNativeLibrary() and loadAtkBridge() in initAtk()
@tanya011 tanya011 requested a review from OnePatchGuy February 11, 2025 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants