On Sat, 27 May 2023 01:25:40 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 one > additional commit since the last revision: > > fix macos build src/java.desktop/share/classes/java/awt/peer/RobotPeer.java line 130: > 128: return false; > 129: } > 130: The only change to this file is adding a blank line. Please revert. src/java.desktop/unix/classes/sun/awt/screencast/ScreencastHelper.java line 139: > 137: > 138: for (TokenItem tokenItem : tokensForRectangle) { > 139: retVal = getRGBPixelsImpl( This can't be right. You surely want to check all cases of retVal not just the last one. src/java.desktop/unix/classes/sun/awt/screencast/TokenItem.java line 88: > 86: > 87: public boolean hasValidBounds() { > 88: //This check is very formal, in order to filter out abnormal > values very "rough" ? src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 62: > 60: private static final String REL_NAME = > 61: ".java/.robot/screencast-tokens.properties"; > 62: .java is already hidden. Why make .robot hidden ? src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 66: > 64: private static final Path PROPS_PATH; > 65: private static final Path PROP_FILENAME; > 66: private static final Object PROPS_LOCK = new Object(); Why do you need PROPS_LOCK to synchronize access to PROPS ? I don't see why you can't synchronize on PROPS directly ? src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 150: > 148: } > 149: > 150: private static class WatcherThread extends Thread { What is all this for ? Reading and writing a file doesn't need all this. src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 387: > 385: } > 386: > 387: try (OutputStream output = Files.newOutputStream(PROPS_PATH)) { Note that this stream is un-buffered (slow) https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/file/Files.html#newOutputStream(java.nio.file.Path,java.nio.file.OpenOption...) Maybe doesn't matter given how small the file is. src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 43: > 41: > 42: #define EXCEPTION_CHECK_DESCRIBE_CLEAR() if ((*env)->ExceptionCheck(env)) > { \ > 43: > (*env)->ExceptionDescribe(env); \ Describe already clears the exception 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 375: > 373: DEBUG_SCREENCAST("@@@ using screen %i\n", index); > 374: if (index >= screenSpace.screenCount) { > 375: DEBUG_SCREENCAST("⚠ Wrong index for screen\n", NULL); I've seen this "! in a triangle" symbol in a quite a few places in the DEBUG messages. Looks like you got some non-ascii char in there. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210736642 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210738114 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210735716 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210744585 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210754519 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210749175 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210767223 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210788290 PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1210799141
