Skip to content

Conversation

@misyltoad
Copy link
Contributor

Sets the desktop name for the session and initializes some variables dbus/systemd need to call the right things.

With these changes, I can successfully screencast with OBS Studio, previously it would ask the global portal interface and there would be none.

It doesn't find portals itself to talk to like some other applications do.

Ensures we use the wlr desktop portal

Signed-off-by: Joshua Ashton <joshua@froggi.es>
@ghost
Copy link

ghost commented Jul 23, 2022

merge! merge! merge! 🎉

Initialize systemd and dbus with our relevant variables
which are needed for some desktop portal use.

This fixes things such as OBS screen casting not working.
In the worst case, on non-systemd/dbus systems, this will just not do anything.

Signed-off-by: Joshua Ashton <joshua@froggi.es>
@droc12345
Copy link
Contributor

You need to make sure it works for those not running systemd.
I'm not so sure that using a system call on something that might not be there is a good idea.

If /run/systemd/system/ exists then systemd is in use, just check and bracket with an if.

@misyltoad
Copy link
Contributor Author

We don't need to check anything, it will just silently fail and continue on.

@droc12345
Copy link
Contributor

Why are you not running those commands from autostart?

@misyltoad
Copy link
Contributor Author

Because making users put stuff in their personal configs to make basic functionality that should work out of the box work is a bad thing.

@Consolatis
Copy link
Member

The change for labwc.desktop is completely fine with me. Could also be added to docs/environment (including a short comment stating the reason) so it also works for people not using a login manager.

Regarding the hardcoded (and blocking) system() calls in main.c: I am really not much a fan of this.
Even if the system() calls silently fail we still hard-code user configuration in labwc, the autostart file exists for this reason.

I'd be completely fine to add these two commands (and the corresponding comment) to docs/autostart and possibly to https://labwc.github.io/integration.html / https://labwc.github.io/getting-started.html though.

@01micko
Copy link
Contributor

01micko commented Jul 23, 2022

If we're going yo use system why not check if we have systemctl available?

  /* use type or command -v ? */
  if(system("type systemctl >/dev/null 2>&1") == 0) { 
    ...
  }

@misyltoad
Copy link
Contributor Author

@Consolatis This is not user configuration, this is completing basic setup of the session needed for things such as portals to work.

Other DEs like KDE and Gnome do essentially the same thing, and the Arch packages for Sway even inject a config to do this.

I am fine with changing it from system to something else that is non-blocking and cleaner.

@Consolatis
Copy link
Member

Other DEs like KDE and Gnome do essentially the same thing

Labwc is not a desktop environment though, its a compositor.

For the same reason labwc doesn't depend on (or tries to start if available) sway-bg, mako or some random panel. You can do screen-recording / screenshots without using dbus / xdg-desktop-portal (e.g. via wf-recorder or grim) so if you prefer using xdg-desktop-portal (or applications which require it) its your choice as a user and thus in my opinion it is a user configuration and belongs into autostart / environment.

and the Arch packages for Sway even inject a config to do this.

I assume the reason they do this is that sway itself doesn't want to hard-code it either.

@misyltoad
Copy link
Contributor Author

If you "prefer" to use applications that don't use it, you can just not use them. You are just arguing that some applications should just randomly be broken unless a user puts magic incantations to bother to finish setting up the session in their autostart.

Setting up your panels is absolutely a user configuration, but this is (and has already to me and others who use labwc) just going to be a useless footgun otherwise.

I agree the current implementation is slightly ugly but it is also simple. For the dbus-stuff, perhaps call it directly, if preferred, etc and use a non-blocking call for the systemd part.

@Consolatis
Copy link
Member

this is (and has already to me and others who use labwc) just going to be a useless footgun otherwise.

I completely agree that this is a pain point and thus both commands / XDG_CURRENT_DESKTOP=wlroots should really be included in docs/autostart / docs/environment / the getting-started page on labwc.github.io but I still don't see a point in setting random stuff up via hard-coded commands in the compositor itself when the user might not even use dbus / systemd.

@Consolatis
Copy link
Member

Consolatis commented Jul 24, 2022

hm.. I still think that providing a proper example in docs/autostart / website is enough to fix this.

I am not completely against merging this when

  • it doesn't cause any startup delay when running on a non-systemd system
  • it doesn't cause any startup delay when running without dbus session
  • XDG_CURRENT_DESKTOP=wlroots is added to examples/environment as well (including a comment)
  • @johanmalm decides it to be fine

@ghost
Copy link

ghost commented Jul 24, 2022

Does anyone really cares about the one guy that will use labwc in a non-systemd system?

@Consolatis
Copy link
Member

Does anyone really cares about the one guy that will use labwc in a non-systemd system?

yes

@01micko
Copy link
Contributor

01micko commented Jul 24, 2022

Does anyone really cares about the one guy that will use labwc in a non-systemd system?

I know at least 3 not including me and there will be a whole bunch more when I release...

@ghost
Copy link

ghost commented Jul 24, 2022

Does anyone really cares about the one guy that will use labwc in a non-systemd system?

I know at least 3 not including me and there will be a whole bunch more when I release...

okay, so what would be a solution to get desktop-portal working without hurting the 4 guys using non-systemd systems?
is a huge pain in the ass to not have it working since this is the path linux distros are going for years now

@Consolatis
Copy link
Member

okay, so what would be a solution to get desktop-portal working without hurting the 4 guys using non-systemd systems?

Add to ~/.config/labwc/environment:

XDG_CURRENT_DESKTOP=wlroots

Add to ~/.config/labwc/autostart

systemctl --user import-environment DISPLAY WAYLAND_DISPLAY XDG_CURRENT_DESKTOP
dbus-update-activation-environment --systemd DISPLAY WAYLAND_DISPLAY XDG_CURRENT_DESKTOP

Restart labwc.

@ghost
Copy link

ghost commented Jul 24, 2022

okay, so what would be a solution to get desktop-portal working without hurting the 4 guys using non-systemd systems?

Add to ~/.config/labwc/environment:

XDG_CURRENT_DESKTOP=wlroots

Add to ~/.config/labwc/autostart

systemctl --user import-environment DISPLAY WAYLAND_DISPLAY XDG_CURRENT_DESKTOP
dbus-update-activation-environment --systemd DISPLAY WAYLAND_DISPLAY XDG_CURRENT_DESKTOP

Restart labwc.

that can work, what you think @Joshua-Ashton?
If Josh agrees i guess just adding documentation for it would be enough.

@droc12345
Copy link
Contributor

A few comments.

  1. autostart is where systemd/dbus/ startup/configuration belongs.
  2. it really is up to the end user to configure their own system, there is no one size fits all
  3. things like screen-sharing/portals are not basic functionality (plenty of people don't need or use them)

Add the lines to autostart AND add a little description to either the generic doc/wiki and/or man pages mentioning this and what it's used for and why.

@misyltoad
Copy link
Contributor Author

autostart is where systemd/dbus/ startup/configuration belongs.

Can you give a genuine reason other than inconveniencing users?

it really is up to the end user to configure their own system, there is no one size fits all

This is not configuration.

things like screen-sharing/portals are not basic functionality (plenty of people don't need or use them)

Then they wouldn't have them installed! This just makes them work, if they are installed.

@misyltoad
Copy link
Contributor Author

I am willing to compromise on doing this only when XDG_CURRENT_DESKTOP is set, (ie. launched as a session) and only when systemd is available in a non-blocking way.

@johanmalm
Copy link
Member

Thanks for all the contributions here. Good discussion, although I note a slight harshness in tone and politely ask everyone stay respectful at all times.

I’m okay with the proposed patch - although think spawn_async_no_shell() or similar is preferable.

My initial reaction was similar to that of @Consolatis thinking that this is the role of a session-file or autostart. I did even contemplate the openbox approach of loading both (rather than one or the other) system wide and user autostart, and then shipping a system autostart template with the systemd/dbus calls.

However, on consideration, I think Josh’s patch makes sense.

We want labwc to be easy to run without too much tweaking and reading of man pages. I also think it’s a shame when distros/OSs have to patch software - better to implement upstream as long as it does not create problems for others or result in technical debt which I don’t think it will in this case.

If we later regret the implementation we can move to a system-wide autostart approach without regression (except that a system wide installation will be required)

Consolatis added a commit to Consolatis/labwc that referenced this pull request Jul 27, 2022
This allows xdg-desktop-portal-wlr to work out of the box for screen-recording.
If systemd or dbus is not available the environmemt update will fail gracefully.

This patch will set XDG_CURRENT_DESKTOP=wlroots but a user can change this by
either having the environment variable set before starting labwc or by having
a different value set in ~/.config/labwc/environment.

Based on PR labwc#461 by @Joshua-Ashton
@Consolatis
Copy link
Member

Consolatis commented Jul 27, 2022

Gave it a try:

Differences to this PR:

  • uses non-blocking spawn_async_no_shell() instead of system()
  • uses dbus-update-activation-environment without the --systemd flag so it also works for users that use a dbus session but no systemd user session
  • sets XDG_CURRENT_DESKTOP environment variable to wlroots even if started without a login manager
  • allows to use a different XDG_CURRENT_DESKTOP environment variable, either by having it set already when launching labwc or by setting it to a different value in ~/.config/labwc/environment. Example use-case would be to set it to sway to fix applications which depend on this value to assume a wlroots based compositor.
  • prevents accidentally launching a dbus session by checking for DBUS_SESSION_BUS_ADDRESS before updating the activation environment

Same to this PR:

  • Fails gracefully if there is no systemd / dbus session around.

Consolatis added a commit to Consolatis/labwc that referenced this pull request Jul 28, 2022
This allows xdg-desktop-portal-wlr to work out of the box for screen-recording.
If systemd or dbus is not available the environmemt update will fail gracefully.

This patch will set XDG_CURRENT_DESKTOP=wlroots but a user may change this by
either having the environment variable set before starting labwc or by having
a different value set in ~/.config/labwc/environment.

Based on PR labwc#461 by @Joshua-Ashton
Consolatis added a commit to Consolatis/labwc that referenced this pull request Jul 28, 2022
This allows xdg-desktop-portal-wlr to work out of the box for screen-recording.
If systemd or dbus is not available the environment update will fail gracefully.

This patch will set XDG_CURRENT_DESKTOP=wlroots but a user may change this by
either having the environment variable set before starting labwc or by having
a different value set in ~/.config/labwc/environment.

Based on PR labwc#461 by @Joshua-Ashton
Consolatis added a commit to Consolatis/labwc that referenced this pull request Jul 28, 2022
This allows xdg-desktop-portal-wlr to work out of the box for screen-recording.
If systemd or dbus is not available the environment update will fail gracefully.

This patch will set XDG_CURRENT_DESKTOP=wlroots but a user may change this by
either having the environment variable set before starting labwc or by having
a different value set in ~/.config/labwc/environment.

Based on PR labwc#461 by @Joshua-Ashton
johanmalm pushed a commit that referenced this pull request Jul 29, 2022
This allows xdg-desktop-portal-wlr to work out of the box for screen-recording.
If systemd or dbus is not available the environment update will fail gracefully.

This patch will set XDG_CURRENT_DESKTOP=wlroots but a user may change this by
either having the environment variable set before starting labwc or by having
a different value set in ~/.config/labwc/environment.

Based on PR #461 by @Joshua-Ashton
@Consolatis
Copy link
Member

@Consolatis Consolatis closed this Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants