Add grim-based wayland universal screenshot adapter #3018
Conversation
|
@mmahmoudian This PR is ready for review, please do a merge review. |
|
This looks good to me. I just request another dev to check it out as well. In the meantime I would be thankful if you can handle the clang format: |
|
@mmahmoudian I just did code formatting. |
|
I tested the patch with the additional cmake option in both Sway and Hyprland. Almost everything works flawlessly. There are two things:
|
|
Wouldn't it a better idea just to use the Wayland protocol? Also grim doesn't work under GNOME, as Mutter doesn't implement the required protocols that grim uses, so I think that should be added to the messages. |
yes, what do you mean? should i delete it?
I was wondering about this issue, it shouldn't be my fault, there was a noticeable delay before I added this patch, I think you might want to open a new issue to fix it - it might not be related to this patch |
The use of grim is mainly because it has implemented the implementation for wlroots. It should be used in a large number of wayland environments. Due to the wayland specification, its implementation is completely determined by the synthesizer. Therefore, for gnome, it is possible to use the dbus protocol to take screenshots is currently the way to go - until there are alternatives available for gnome-compositor |
Yeah I mean use the protocol that grim uses, if flameshot detects the user to be on a compositor that implements it. Also wasn't GNOME dbus api made private? Also I meant add it to the cmake messages |
Do you mean, should I add a message telling users that grim is available in wlroots compositor based implementations? |
|
@yavko Now, when users are running on GNOME or KDE, the grim-based generic screenshot adapter will not be enabled |
I might be wrong here, but I think kwin (might) implement it, as it implements other wlroots protocols. |
|
@yavko Implementation excludes KDE |
|
If KDE does not support grim, it can be modified in another PR, or through the cmake flag to disable GRIM |
After doing some research I think that KDE doesn't support it, but apparently Mir does lol. EDIT: This website says it doesn't, and its pretty accurate most of the time https://wayland.app/protocols/wlr-screencopy-unstable-v1#compositor-support |
|
@yavko Ok, do you think I need to remove it? Or keep it for now? |
Well from what I've seen KDE implements stuff like layer shell, but I cant find anything on it implementing screencopy so maybe remove it, and someone can re-add it down the line if they so please. For KDE its probably be a better idea to implement the in house solution used by spectacle, in a different PR. |
|
@yavko OK, It can be re-added if someone needs it in KDE |
|
Replenish: Regarding the gnome adaptation method of wayland, the currently feasible method is to serve the desktop app of upwork, which is at this address: https://github.com/MarSoft/upwork-wayland The author is a freelancer who prepared this for sway, and I implemented a fork that undoes more parts of sway than implements, at (since I'm not sway): https://github.com/jack9603301/upwork-wayland This adapter is implemented to run freelance work software such as upwork that relies on the gnome interface under wayland, but it may provide an implementation for gnome software. Although grim may not be available on gnome, we can know that gnome The status of the interface used With the help of this simple dbus adapter, some software with the screenshot function of the gnome screenshot protocol can be used freely under wlroots of wayland - because it simulates the interface of gnome |
Yeah apps can still use that dbus api, but GNOME removed it on GNOME, in GNOME 41. So if its implemented apps that were made to use it work, but it doesn't work on versions of GNOME newer than 41, 40 and below is fine tho. EDIT: This is why zoom screensharing stopped working after GNOME 41, so they implemented the portal method |
|
Can this be merged as an opt-in with some make flags to enable it? Those of us not using gnome are waiting on this patch as well, have several open issues which would be resolved by it. We should not be punished because of gnome's design decisions when it comes to implementing wayland. |
That's what this PR adds.
this ONLY adds support for wlroots, and is not a universal solution. That's why the portal exists and mostly works fine.
GNOME itself doesn't have working screenshotting it has nothing to do with GNOME's design decisions. GNOME had a internal dbus api that apps leveraged that was made private, which doesn't affect other compositors in anyways. People need to stop blaming everything on GNOME |
|
@mgil2 GRIM is currently known to work with wlrools, it is not really a general solution for GNOME, it is very likely that grim will not work properly under GNOME, when you find an alternative available for gnome, you or other community members can do it via PR Realize it When you use most compositors based on wlroots other than gnome and kde, you can enable the grim screenshot adapter |
|
@jack9603301 Thank you for keeping this branch up to date. I appreciate the efforts. I'm still waiting for other dev to approve the PR. |
There was a problem hiding this comment.
@jack9603301 @mmahmoudian Sorry for the long wait. I have reviewed the code - looks good. I have compiled flameshot with the -DUSE_WAYLAND_GRIM option and it works good on sway. Although I need to mention that without this option flameshot doesn't work for me on wayland. Not that it's in any way this PR's fault, but this means I can't verify if these changes break anything on wayland without grim. So I'll defer to other people's experience with this. I can verify that nothing breaks when running flameshot under X.
Just fix the formatting please.
In the past, flameshot used a special and superfluous method to call the wayland screenshot component - dbus protocol communication. Although this method is supported by a large number of distributions, it does not take into account the actual situation of wayland (including WM custom desktop environment users). Now, we can enable the wayland universal screenshot adapter with the help of grim, just add the following compilation flags in cmake: ``` -DUSE_WAYLAND_GRIM=true ``` In addition, the patch also adds HYPRLAND type and OTHER type support
Due to the dependency problem of USE_WAYLAND_CLIPBOARD, cancel USE_WAYLAND_GRIM to activate USE_WAYLAND_CLIPBOARD by default, Add a warning prompt to activate USE_WAYLAND_GRIM when USE_WAYLAND_CLIPBOARD is activated
The grim adapter cannot be used in gnome and similar environments, modify the cmake message to express it
Generic screenshot adapter is only supported on compositors that support wlroots
* Add grim-based wayland universal screenshot adapter In the past, flameshot used a special and superfluous method to call the wayland screenshot component - dbus protocol communication. Although this method is supported by a large number of distributions, it does not take into account the actual situation of wayland (including WM custom desktop environment users). Now, we can enable the wayland universal screenshot adapter with the help of grim, just add the following compilation flags in cmake: ``` -DUSE_WAYLAND_GRIM=true ``` In addition, the patch also adds HYPRLAND type and OTHER type support * grim outputs to standard streams instead of files * Automatically enable wayland clipboard support when USE_WAYLAND_GRIM is enabled * Cancel USE_WAYLAND_GRIM Activate USE_WAYLAND_CLIPBOARD by default Due to the dependency problem of USE_WAYLAND_CLIPBOARD, cancel USE_WAYLAND_GRIM to activate USE_WAYLAND_CLIPBOARD by default, Add a warning prompt to activate USE_WAYLAND_GRIM when USE_WAYLAND_CLIPBOARD is activated * perform formatting * modify cmake message The grim adapter cannot be used in gnome and similar environments, modify the cmake message to express it * remove generic screenshot adapter for gnome Generic screenshot adapter is only supported on compositors that support wlroots * Update format (cherry picked from commit 3ededae)
Is this something that we should also add to our AUR build instructions and also to our docs/Sway and wlroots support.md ? |
|
@mmahmoudian To docs/Sway and wlroots support.md the answer is probably yes. For AUR build instructions, do you mean we should add it as a comment, or enable the option while building based on some condition? |
Would it be possible for you to add it? This was we can ask others to test it for us. And also I get a better understanding of where you have used that flag
Good question. Initially I thought that if it doesn't break X11, we can have it for eveyone and always build with that flag, but then I realized this might be wlroot specific as in KDE Plasma Wayland and Gnome Wayland, Flameshot is already functional. I think this needs some investigation. I have Hyprland on my machine but few others can be tested either by the community or through VM. What do you think? |
|
I think it's now necessary to sync to other distro's main repos, this includes aur and ebuilds Meanwhile I think this should be added to the docs |
Can be used as an option to enable the flag when the user turns on |
I confirm that I observe this behavior as well. Moreover, even when compiling with both |
In the past, flameshot used a special and superfluous method to call the wayland screenshot component - dbus protocol communication. Although this method is supported by a large number of distributions, it does not take into account the actual situation of wayland (including WM custom desktop environment users). Now, we can enable the wayland universal screenshot adapter with the help of grim, just add the following compilation flags in cmake:
In addition, the patch also adds HYPRLAND type and OTHER type support