On Wed, 7 Jun 2023 01:27:50 GMT, Alexander Zvegintsev <azveg...@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
