Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Workrave and other typing break monitors to work with video better on Niri #665

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jjramsey
Copy link

This partially addresses the issues described in this bug report: #660

Basically, Niri's implementation of the org.freedesktop,Screensaver D-Bus interface means that Workrave -- and other typing break monitors that use the ext-idle-notify Wayland extension -- treat time watching videos (at least via Firefox) as non-idle, even if the user provides no input, causing micro-break and rest-break windows to appear while watching the video.

This pull request provides a configuration option, prefer-no-fdo-screensaver-dbus-impl, that turns off that implementation, thus allowing Workrave and similar typing break monitors to work on Niri as well as they work on other Wayland compositors.

Note that "work on Niri as well as they work on other Wayland compositors" does not mean that Workrave will work on perfectly, since a Wayland client that activates the Wayland idle-inhibit protocol can still cause problems for typing break monitors that use ext-idle-notify to track user activity.

Still, it means that Niri wouldn't have to provide any additional obstacles to usage of Workrave and similar typing break monitors, and users who still wanted to use Niri's org.freedesktop,Screensaver can simply not add the prefer-no-fdo-screensaver-dbus-impl to their config files.

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 14, 2024

Hey; as I mentioned before, I don't think this is a good or an appropriate solution. It just happens to work with the current issues and bugs of the software you're using:

  1. If the portals update and fix the bug that makes the inhibit portal always succeed, while you disable ScreenSaver in niri like in this PR, then Firefox will receive that error and fall back to its Wayland inhibit impl, and your Workrave issue will come back.
  2. If Firefox updates to prefer the Wayland inhibit impl (which would be a welcome change for compositors that don't use portals), then your Workrave issue will come back.
  3. If, as you mention, you play videos in another player like mpv which uses the Wayland inhibit impl, then your Workrave issue will come back.

Instead, I would investigate implementing the Mutter-specific user activity monitor D-Bus interface, which Workrave can use. If Workrave can natively understand inhibiting when using the Mutter D-Bus interface, then this will be a proper solution to the problem that will not break as other components fix their bugs.

@jjramsey
Copy link
Author

I think you're letting the perfect be the enemy of the good, here.

Your points mostly come down to the same issue: Wayland's idle-inhibit protocol is a blunt instrument that can't distinguish between the user not being idle and the computer not being idle. Fixing that is probably outside of Niri's scope.

As for a detail of the first point, you seem to assume that if "the portals update and fix the bug that makes the inhibit portal always succeed", then the inhibit portal will fail on my system, rather than succeed and do its thing without interfering with the Wayland compositor or Workrave.

There are a couple problems with your suggestion, "I would investigate implementing the Mutter-specific user activity monitor D-Bus interface." First, it's specific to Workrave and doesn't generalize across compositors. Second, this solution would amount to Niri implementing two different idle notification protocols: one that aims to work across Wayland implementations, and one that was designed to be specific to a specific compositor.

I do not claim that my proposed patch is perfect. There are certainly issues upstream of Niri that make typing monitors on Wayland wonky. All the patch does is address a particular narrow issue: Niri imposes a particular obstacle to getting typing break monitors to work on Wayland that other compositors don't. This patch would simply allow Niri to not have that particular obstacle.

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 14, 2024

As for a detail of the first point, you seem to assume that if "the portals update and fix the bug that makes the inhibit portal always succeed", then the inhibit portal will fail on my system, rather than succeed and do its thing without interfering with the Wayland compositor or Workrave.

No. What I mean is the following. Firefox tries to inhibit idle in different ways in order. It first tries the portal, then if the portal fails, it tries the Wayland inhibit. Currently, the portal can never fail due to a bug, so effectively with this PR, Firefox's request to inhibit is ignored, while Firefox itself thinks it succeeded. When the portal is fixed to return an error, Firefox will instead try the next option, Wayland inhibit, so your problem with Workrave will come back.

There are a couple problems with your suggestion, "I would investigate implementing the Mutter-specific user activity monitor D-Bus interface." First, it's specific to Workrave and doesn't generalize across compositors.

...but this PR is already specific to niri and generalize across compositors? What's your point?

Second, this solution would amount to Niri implementing two different idle notification protocols

Yes, this is what I'm suggesting. Not a terribly big deal if it fixes real issues like this one.

@jjramsey
Copy link
Author

jjramsey commented Sep 14, 2024

No. What I mean is the following. Firefox tries to inhibit idle in different ways in order. It first tries the portal, then if the portal fails, it tries the Wayland inhibit. Currently, the portal can never fail due to a bug, so effectively with this PR, Firefox's request to inhibit is ignored, while Firefox itself thinks it succeeded. When the portal is fixed to return an error, Firefox will instead try the next option, Wayland inhibit, so your problem with Workrave will come back.

The statement, "When the portal is fixed to return an error, Firefox will instead try the next option, Wayland inhibit," is almost but not quite correct. Rather, when the portal is fixed to return an error, Firefox will instead try the next option if the portal is actually broken. If the portal isn't broken, then Firefox won't try the next option.

...but this PR is already specific to niri and generalize across compositors? What's your point?

I miswrote. I should have written "it's specific to Workrave and doesn't generalize across typing break monitors [not compositors]". Sorry about that.

Second, this solution would amount to Niri implementing two different idle notification protocols
Yes, this is what I'm suggesting. Not a terribly big deal if it fixes real issues like this one.

As I noted before, one of those idle notification protocols is used by only one typing break monitor and one compositor. Implementing that would amount to implementing a legacy protocol.

I think if you're going to have a more ambitious fix, it should be something that could be a proof-of-concept for future revisions of Wayland idle protocols. I already spitballed some thoughts in one of my bug reports (#660) and in an issue that I posted to freedesktop's gitlab (https://gitlab.freedesktop.org/wayland/wayland-protocols/-/issues/213).

@jjramsey
Copy link
Author

Also, upon reflection, I think Niri's org.freedesktop.Screensaver implementation is flawed because it acts indirectly via ext-idle-notify. Here's what I mean.

As far as I can tell, Niri accepts signals from apps using the org.freedesktop.Screensaver interface. What does it do with those signals? Well, it uses ext-idle-notify to indicate that the user and/or the computer is not idle, so that a separate idle-monitoring daemon won't run commands like swaylock. This has the side effect of interfering with typing break monitors.

Suppose that instead of implementing org.freedesktop.Screensaver interface in a Wayland compositor, you instead had, say, forked some idle management daemon (e.g., swayidle or https://github.com/fishman/sleepwatcher-rs/) to use that interface. An idle management daemon can simply accept signals from apps using that interface, and then directly choose whether to launch swaylock or not, without having to interfere with any typing break monitors.

Given this, I think a proper solution would involve turning off Niri's current implementation of the org.freedesktop.Screensaver interface anyway. This PR would simply be a step towards that.

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 15, 2024

Rather, when the portal is fixed to return an error, Firefox will instead try the next option if the portal is actually broken. If the portal isn't broken, then Firefox won't try the next option.

The inhibit portal impl will try to tell the compositor about the inhibit request. If there's nobody to listen (after you disable the ScreenSaver niri impl), it will report an error to Firefox, which will cause it to use the Wayland inhibit impl, which will bring you back to your issue.

I miswrote. I should have written "it's specific to Workrave and doesn't generalize across typing break monitors [not compositors]". Sorry about that.

Have you looked at what other typing break monitors do? GNOME doesn't implement Wayland idle notifier, so they might as well all just use the same Mutter D-Bus API. In that case implementing it in niri will solve it for all of them.

I think if you're going to have a more ambitious fix, it should be something that could be a proof-of-concept for future revisions of Wayland idle protocols. I already spitballed some thoughts in one of my bug reports (#660) and in an issue that I posted to freedesktop's gitlab (https://gitlab.freedesktop.org/wayland/wayland-protocols/-/issues/213).

Also, upon reflection, I think Niri's org.freedesktop.Screensaver implementation is flawed because it acts indirectly via ext-idle-notify. Here's what I mean.

Suppose that instead of implementing org.freedesktop.Screensaver interface in a Wayland compositor, you instead had, say, forked some idle management daemon (e.g., swayidle or https://github.com/fishman/sleepwatcher-rs/) to use that interface. An idle management daemon can simply accept signals from apps using that interface, and then directly choose whether to launch swaylock or not, without having to interfere with any typing break monitors.

That issue sounds good, yes. Unfortunately, until something is done about it, I heavily suspect the reality is that the idle notifier Wayland protocol is most commonly used for powering off monitors and locking the screen (and not for RSI monitoring apps). Therefore, niri hooks it up to org.freedesktop.ScreenSaver to make it work as expected with sandboxed Firefox—inhibit Wayland idle when it's playing a video. There's no other way around it, until the portal is fixed and/or Firefox starts preferring the Wayland inhibiting (at which point this PR will also stop solving your problem).

So yeah, I suggest that you keep running the niri patch that solves the problem for you (this is what software freedoms are for after all), meanwhile we can implement the Mutter D-Bus idle interface (provided that it solves this problem) because it won't break in the future, and if a new version of the Wayland idle notifier protocol appears fixing the user vs. application inhibit distinction, I will happily implement that in niri also.

@jjramsey
Copy link
Author

Have you looked at what other typing break monitors do?

Yes, and I pointed that out in https://gitlab.freedesktop.org/wayland/wayland-protocols/-/issues/213.

@jjramsey
Copy link
Author

jjramsey commented Sep 15, 2024

So yeah, I suggest that you keep running the niri patch that solves the problem for you (this is what software freedoms are for after all), meanwhile we can implement the Mutter D-Bus idle interface (provided that it solves this problem) because it won't break in the future, and if a new version of the Wayland idle notifier protocol appears fixing the user vs. application inhibit distinction, I will happily implement that in niri also.

Ok, so suppose we do it your way. Where would we start?

I can point you to the XML file for the org.gnome.Mutter.IdleMonitor, which has comments that document its interface: https://gitlab.gnome.org/GNOME/mutter/-/blob/main/data/dbus-interfaces/org.gnome.Mutter.IdleMonitor.xml

I can also point you to the code in Workrave that shows how it uses that interface: https://github.com/rcaelers/workrave/blob/cf193977815e379aa9398d7e875d7e6e620d760b/libs/input-monitor/src/unix/MutterInputMonitor.cc

Beyond this, I'm not sure what else I can do, since I don't currently have the background knowledge to implement the interface. Would you expect to do the bulk of the implementation and primarily have me test it?

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 15, 2024

Thanks.

Beyond this, I'm not sure what else I can do, since I don't currently have the background knowledge to implement the interface. Would you expect to do the bulk of the implementation and primarily have me test it?

I suppose something like this, unless someone else gets to it first.

@jjramsey
Copy link
Author

So should I create a feature request issue for this, then?

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 15, 2024

Sure

@jjramsey
Copy link
Author

Done. See here: #671

I thought I'd end up posting an issue with the "enhancement" tag, but hopefully this works as well.

@YaLTeR
Copy link
Owner

YaLTeR commented Sep 15, 2024

Thanks

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.

2 participants