[rcore][GLFW] Fix GetClipboardImage creating new connection under X11.#5871
[rcore][GLFW] Fix GetClipboardImage creating new connection under X11.#5871steampuker wants to merge 1 commit into
Conversation
|
|
||
| // Request the data: "Convert whatever is in CLIPBOARD to image/png and put it in RAYLIB_CLIPBOARD_MANAGER" | ||
| XConvertSelection(dpy, clipboard, targetType, property, win, CurrentTime); | ||
| XConvertSelection(platform.cbDisplay, platform.cbAtom, platform.cbTargetAtom, platform.cbPropertyAtom, platform.cbWindow, CurrentTime); |
There was a problem hiding this comment.
ColleagueRiley's reference also calls XSync after this, do we need to add it here?
There was a problem hiding this comment.
It's not really required because you later wait for the SelectNotify event, but it may still be good to have.
The XSync() function flushes the output buffer and then waits until all requests have been received and processed by the X server.
|
I just looked into commit history, it seems RGFW is also affected? Do I port these to RGFW platform as well? |
|
There should generally be possible to use the original GLFW X Display and X Window rather than creating your own internal display. But, in my opinion, hacky backend fixes like these don't really belong in Raylib, as they make Raylib harder to maintain. This is exactly why the RGFW backend exists in Raylib, because the RGFW is more actively worked on and the implementation is easier to understand and modify. Features can be added directly into RGFW rather than being forced into each backend. Of course, that means that GLFW users will have to wait for GLFW to get updated, but I think that's for the best since for Raylib shouldn't have to maintain these messy hacks throughout each backend. The I have also seen plenty of these sorts of hacks being the result of AI generated code. Ray isn't as familiar with each low-level backend as someone like me, or a GLFW maintainer and I don't believe Ray directly tests for Linux or OSX. So dubious low level backend hacks will create hard to find bugs directly in Raylib, rather than being caught filtered out by RGFW/GLFW. |
Hello, Riley, thanks for your insight. At some point, I also thought of using raylib's main connection for this but then one would have to deal with event loop management. What if a Key or a Mouse event fires in the middle of this process? I suppose that's why it was made like that in the first place (even though I don't like it personally). I agree that this is a hacky solution, having platform-specific code in a platform-agnostic backend is not a good idea. I suppose for Windows it worked because it's a few lines of code but for Linux, the situation is different (considering, there's also Wayland support wanted). If RGFW supports this out of the box, will be awesome but I'm also concerned about the GLFW backend. My main issue with this whole thing is that because of this, projects using static raylib have to be explicitly linked to X11. The way it was before, GLFW would dynamically load necessary X11 functions and you would simply link raylib without additional flags (maybe aside from -lm), so the very first claim:
Would be true. Not sure about AI generated but this does feel like a PoC code pushed into the upstream. So now that raylib does require X11 linkage, I want the implementation to be somewhat usable in real projects. |
|
I think your first point is somewhat true. Though, I'm pretty sure you are able to resolve this by peeking the event, seeing if it's the one you want and if not, put it back in the queue. Yeah, I don't think it's even a good idea for winapi code due to maintainability issues. But you bring up some other points that I didn't mention. Generally for RGFW I have two methods for handling wayland/x11, either the user has to choose one at compile time, making it incompatible with the other, or you can actually compile for both backends, and then choose the preferred one on runtime. It will also fallback to X11 if wayland fails. While that does lead to a larger binary, I think that's generally worth it as it will ensure both platforms are supported by default. But Raylib can handle that however it sees fit. I don't really consider X11 or Wayland to be an external dependency as they're core parts of the Linux platform. Dynamically loading doesn't necessarily fix the problem, as it requires the headers, but RGFW has a way of doing that too via XDL. But I would still consider dynamic loading to be a dependency. I have seen AI code in other PRs too, but specially with the original changes for this feature @CrackedPixel and I saw some tell tale signs of AI. For example, how there were a ton of comments all over the code that were later removed. |
| @@ -1068,30 +1070,32 @@ Image GetClipboardImage(void) | |||
| #elif defined(__linux__) && defined(_GLFW_X11) | |||
There was a problem hiding this comment.
defined(__linux__) is incorrect, the user may be using X11 on a different Unix system that is not Linux
As of now, GetClipboardImage creates a new connection and a ghost window to retrieve image data and then unloads it all. This adds unnecessary overhead for this function caused by establishing and terminating connection.
This PR stores this clipboard connection and other related data in the global platform struct but is also lazy loaded (so a first call would have the overhead and subsequent ones would not). The clipboard handling data is unloaded upon ClosePlatform.
Tested with ubench:
Without the fix, the result is:
With the fix: