Added river support

[midirhee12]

This attempts to generalize wlroots compositors by checking against the proper desktop name instead of only sway. This fixes the problem of only supporting sway. Any additional desktop will just have to be added to the source to check for the correct string.

[Sunderland93]

This rule is not support since River 0.3.x. You should use riverctl rule-add -app-id "flameshot" float instead

[midirhee12]

Okay, I'll change this

[midirhee12]

Changed 👍

[TAforever]

Maybe I don’t understand something, but river is already supported in the master branch and you no longer need to indicate that the current desktop is sway

[midirhee12]

Maybe I don’t understand something, but river is already supported in the master branch and you no longer need to indicate that the current desktop is sway

Changing your XDG vars to sway is not a solution. It is a very hacky work around. It breaks all other software using XDG vars. So explicit support is required instead of the hack which the docs recommend.

[TAforever]

You don’t understand, you no longer need to indicate that the current desktop is sway. I have XDG_CURRENT_DESKTOP=river set on river and XDG_CURRENT_DESKTOP=Hyprland on hyprland

[midirhee12]

You don’t understand, you no longer need to indicate that the current desktop is sway. I have XDG_CURRENT_DESKTOP=river set on river and XDG_CURRENT_DESKTOP=Hyprland on hyprland

Can you link the commit that solved this?

[TAforever]

Maybe 3ededae not sure but you can build the master branch and make sure everything works

[midirhee12]

@TAforever

1. This only adds Hyprland support which no longer uses the wlroots portals. This issue is for river support and wlroots compositors in general through the future thus completely off topic.
2. I'm starting to seriously doubt that you actually tested this on both Hyprland AND river. It sounds like you only tried Hyprland and assumed it works for all. Don't tell others to build against master on the exact target when they have and you obviously haven't.
3. The code in its current state (both v12.1.0 release and master) matches for gnome, qtile, sway, hyprland, and KDE here and here. So unless your compiler is magic, everyone else's compiler is somehow defunk, the docs are wrong, and/or our complete understanding of how a switch-case statement works is defunk, you're wrong or lying.

Please find some other person to troll when you're too lazy and lack the time to actually perform a proper system test.

[TAforever]

It was very strange to read this. If you don’t believe me (although why should I deceive you), then trust yourself simply by checking. Good luck

[midirhee12]

You may have XDG_CURRENT_DESKTOP=Hyprland still set for river which because of the xdg desktop hyprland portal being a fork of the wlroots portal may somehow still be backwards compatible. But this again is another workaround and will probably not work in the foreseeable future. Also, you may not be exposing your XDG vars to dbus.

then trust yourself simply by checking

I have. I don't make PRs or comment on issues or PRs without checking like you.

[TAforever]

river.mp4

[midirhee12]

What are you running for dbus-update-activation-environment?

[TAforever]

In init I have dbus-update-activation-environment --systemd WAYLAND_DISPLAY

[midirhee12]

Then that's probably why. You haven't properly told dbus your XDG_CURRENT_DESKTOP which some but not all programs require.

[midirhee12]

Also, how are you launching flameshot? I see no flameshot command executed on the terminal. So this is really deceiving.

[TAforever]

I don't know how to comment on this. Goodbye

[midirhee12]

Well, you could show how you're running flameshot or even send another video of actually running flameshot from the terminal. But the fact that you're only telling dbus that you're on Wayland and not which compositor already throws a red flag.

[mmahmoudian]

Apart from the discussion, and despite of not being a river user myself, the PR's code change looks good to me. let's wait for @veracioux, @panpuchkov, @jack9603301, or other devs review it too before merging.

As a side note, imho this if(...) || (...) || ... situation is not scalable for wlroots. There must be a better way and more universal approach to this.

[midirhee12]

situation is not scalable for wlroots

I 100% agree. But the only other thing I can think of to find if something uses wlroots is to ask if the xdg-desktop-portal-wlr is installed which is not always the case. So to be consistent with the other code and to be conforming with every wlroots system, this is probably the best choice for now.

[midirhee12]

I mean, I guess the xdg-desktop-portal is kind of required for a screenshot tool to be fair. So it may be possible to test based on this?

[mmahmoudian]

So it may be possible to test based on this?

Perhaps. I think for now we go with this and keep an open mind on alternative approaches

[midirhee12]

I'll play around with the xdg-desktop-portal in the meantime to see if I can figure out something. I think it'll require a lot of research though. So I don't anticipate having a PR ready any time soon. So yeah, I agree. I think merging this in the meantime is the best idea.

[midirhee12]

Any further thoughts? Or is there anything keeping from merging this?

[0xmayro]

Hello, not sure if it's helpful,
this PR worked for me on Arch with the river compositor.

[midirhee12]

Yeah, I'm not sure what the hold up is. It compiles. It works. I'm willing to test for river in the future. And no one is merging it. Absolutely nuts. It isn't like there's much activity on the repo either.