RFR: 8280982: [Wayland] [XWayland] java.awt.Robot taking screenshots [v11]
Prasanta Sadhukhan
psadhukhan at openjdk.org
Wed Jun 7 05:28:16 UTC 2023
On Wed, 7 Jun 2023 01:27:50 GMT, Alexander Zvegintsev <azvegint at openjdk.org> wrote:
>> Modern Linux systems often come with [Wayland](https://wayland.freedesktop.org/) by default.
>> This comes with some difficulties, and one of them is the inability to get screenshots from the system.
>> This is because we now use the [X Window System API](https://en.wikipedia.org/wiki/X_Window_System) to capture screenshots and it cannot access data outside the [XWayland server](https://wayland.freedesktop.org/xserver.html)
>>
>> But this functionality is a very important part of automated testing.
>>
>>
>> At the moment there are two obvious solutions to this problem, and both use [xdg-desktop-portal](https://github.com/flatpak/xdg-desktop-portal):
>>
>> 1. [org.freedesktop.portal.Screenshot DBUS API](https://flatpak.github.io/xdg-desktop-portal/#gdbus-org.freedesktop.portal.Screenshot)
>> It has several drawbacks though:
>> + It saves a screenshot to disk, which must be read and deleted(may add some delays depending on the type of a disk drive).
>> + There is no way to disable the visual "screen flash" after screenshot
>> + It asks a user confirmation to save a screenshot. This confirmation can be saved on Gnome 43+.
>> Since we would like Ubuntu 22.04 LTS which comes with Gnome 42 this option is not acceptable for us because it would require user confirmation for each screenshot.
>> But we still can consider this option as a fallback.
>>
>>
>>
>> 2. [org.freedesktop.portal.ScreenCast](https://flatpak.github.io/xdg-desktop-portal/#gdbus-org.freedesktop.portal.ScreenCast)
>> It typically used by applications that need to capture the contents of the user's screen or a specific window for the purpose of sharing, recording, or streaming.
>> This might be a bit of overkill, but it avoids several of the problems mentioned in the Screenshot API.
>>
>> + implementation is more complicated comparing to Screenshot API
>> + no intermediate file, screenshot data can be obtained from memory
>> + Permission to make screenshots can be stored with [`restore_token`](https://flatpak.github.io/xdg-desktop-portal/#gdbus-method-org-freedesktop-portal-ScreenCast.SelectSources)
>>
>>
>> So this PR adds the ability to take screenshots using the ScreenCast API. This functionality is currently disabled by default.
>>
>> This change also introduces some new behavior for the robot:
>> A system window now appears asking for confirmation from the user to capture the screen.
>> + The user can refuse the screen capture completely. In this case a security exception will be thrown.
>> + The user can allow...
>
> Alexander Zvegintsev has updated the pull request incrementally with two additional commits since the last revision:
>
> - move screencast-tokens.properties to a new location
> - fix failure of javax/swing/reliability/HangDuringStaticInitialization.java in headless environment
Mostly alignment issue with couple of code comments..
src/java.desktop/unix/classes/sun/awt/X11/XRobotPeer.java line 53:
> 51:
> 52: tryGtk = Boolean.parseBoolean(
> 53: AccessController.doPrivileged(
Code formatting..better to align `AccessController `below `Boolean`
src/java.desktop/unix/classes/sun/awt/X11/XRobotPeer.java line 69:
> 67: isOnWayland
> 68: ? METHOD_SCREENCAST
> 69: : METHOD_X11
code alignment issue...will look better if all starts in same column
src/java.desktop/unix/classes/sun/awt/X11/XRobotPeer.java line 126:
> 124: int[] pixelArray = new int[1];
> 125: if (screenshotMethod.equals(METHOD_SCREENCAST)
> 126: && ScreencastHelper.isAvailable()) {
align `&&` below `screenshotMethod`
src/java.desktop/unix/classes/sun/awt/X11/XRobotPeer.java line 138:
> 136: int[] pixelArray = new int[bounds.width * bounds.height];
> 137: if (screenshotMethod.equals(METHOD_SCREENCAST)
> 138: && ScreencastHelper.isAvailable()) {
same here
src/java.desktop/unix/classes/sun/awt/X11/XRobotPeer.java line 142:
> 140: bounds.x, bounds.y,
> 141: bounds.width, bounds.height,
> 142: pixelArray
you can align below the brace ( as it will not cross 80 chars, I believe
src/java.desktop/unix/classes/sun/awt/X11/XRobotPeer.java line 150:
> 148: bounds.width, bounds.height,
> 149: pixelArray, useGtk
> 150: );
same here
src/java.desktop/unix/classes/sun/awt/screencast/ScreencastHelper.java line 63:
> 61: SCREENCAST_DEBUG = Boolean.parseBoolean(
> 62: AccessController.doPrivileged(
> 63: new GetPropertyAction(
align AccessController below Boolean
src/java.desktop/unix/classes/sun/awt/screencast/ScreencastHelper.java line 72:
> 70:
> 71: if (!(Toolkit.getDefaultToolkit()
> 72: instanceof UNIXToolkit tk && tk.loadGTK())
I guess better to move `Toolkit.getDefaultToolkit() instance UNIXToolkit tk` in same line and align `&& `below Toolkit
src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 145:
> 143: path,
> 144: Set.of(PosixFilePermission.OWNER_READ,
> 145: PosixFilePermission.OWNER_WRITE)
alignment issue..Posix..OWNER_WRITE should be placed below Posix..OENER_READ
Also, why we aren't adding OWNER_EXECUTE here too like done above?
src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 316:
> 314: allTokenItems = PROPS.entrySet()
> 315: .stream()
> 316: .map(o -> {
align below PROPS
src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 335:
> 333: if (tokenItem != null
> 334: && tokenItem
> 335: .hasAllScreensWithExactMatch(affectedScreenBounds)) {
align below `tokenItem`
src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 350:
> 348: .map(rectangle -> new Dimension(
> 349: rectangle.width,
> 350: rectangle.height
align below `affectedScreenBounds`
src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 356:
> 354: for (TokenItem tokenItem : allTokenItems) {
> 355: if (tokenItem != null
> 356: && tokenItem.hasAllScreensOfSameSize(dimensions)) {
align below `tokenItem`
src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 371:
> 369: if (!isWritable()
> 370: || malformedRecords == null
> 371: || malformedRecords.isEmpty()) {
align below `isWritable`
src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 406:
> 404: private static boolean isWritable() {
> 405: if (PROPS_PATH == null
> 406: || (Files.exists(PROPS_PATH) && !Files.isWritable(PROPS_PATH))) {
align below PROPS_PATH
src/java.desktop/unix/native/libawt_xawt/awt/gtk2_interface.h line 94:
> 92: gint width;
> 93: gint height;
> 94: } GdkRectangle;
Any reason why it is removed? I think it's needed in gtk2_interface.c, right?
Probably we can delete from gtk3_interface.h as it is not used in gtk3_interface.c
src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 674:
> 672: EXCEPTION_CHECK_DESCRIBE();
> 673: if (!allowedBounds) {
> 674: (*env)->ExceptionClear(env);
I guess as mentioned above, DESCRIBE clears exception also so no need of calling explicit Clear
https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#exceptiondescribe
src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 694:
> 692: if ((*env)->ExceptionCheck(env)) {
> 693: (*env)->ExceptionDescribe(env);
> 694: (*env)->ExceptionClear(env);
same here
-------------
Marked as reviewed by psadhukhan (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13803#pullrequestreview-1466590673
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220836005
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220836075
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220836350
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220836410
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220837445
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220837512
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220838239
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220839222
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220853099
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220853881
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220854061
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220854228
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220854400
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220854551
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220854760
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220857001
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220864748
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1220865107
More information about the client-libs-dev
mailing list