Make zbus Optional, Remove anyhow, Fix notify Example

[bbb651]

I discovered --print-reply=literal for dbus-send and argN= for dbus-monitor which make the logic nicer, detect is in a very good state, subscribe is a bit more fragile in terms of forward compatibility, but I think it's good enough to merge it.

On my system notify sends each notification twice, I originally had some logic to deduplicate it, but it also happens with zbus so it's probably a Gnome bug. (It might still be useful to have it regardless?)

Fixes #34, #15 (FWIW, I agree with @parasyte, being pure-rust isn't a very compelling argument for a abstraction crate such as this, and this crate being important for accessibility and zbus being used in accesskit has almost nothing to do with how this crate is implemented. This crate feels like it should be light, so it should be light - depending on an heavy crate breaks user expectation and leads to frustration.)

Edit: I'm not sure if Mode::Preference implementing Default is a good idea, people might think it detects the theme, and I can't think of a reason to use it outside the implementation, as users will most likely convert it to their own type. I'm not sure it even Mode::Preference needs to exist, as Option<Mode> can express the same thing.

[jacksongoode]

This would be nice to merge if someone has the time to review.  There are currently a number of PRs which are providing essential fixes, which are now spinning off into forks since they don't get merged in time.

[Be-ing]

Hi,
First, please limit pull requests to one topic so each change can be discussed independently. I'll limit the rest of this comment to making zbus optional; please split off the other commits to separate pull requests.

Making zbus optional was already discussed and dismissed in #15. It doesn't solve any actual problems; shifting dependencies from Rust to C doesn't remove the dependencies. But if you really really insist and are willing to implement it yourself, replacing zbus with a C alternative could be an option off by default. I don't generally expect Rust crates to have a dependency on a C library or external executables unless it's a binding crate to a C library. I would be quite annoyed if I got a build or run time error because of that when an upstream crate could've just used a Rust crate.

As far as the implementation, I think calling external executables is a questionable approach. There's no guarantee those exectuables exist on any particular system. Even if they happen to be shipped by many distros by default now (which as far as I can tell, you haven't investigated if that's the case), there's no guarantee they will continue doing so in the future. The author of zbus is working on busd, a Rust replacement for dbus-daemon. I suspect some distros will switch to shipping that instead of dbus-daemon in the coming years. It's conceivable that project could continue to a Rust replacement for dbus-send and dbus-monitor in the future, perhaps with incompatible CLIs. Also, returning Mode::NoPreference when those exectuables aren't found is incorrect; it's not that there is no system preference, but that detecting it failed. The downstream developer/packager gets no indication that there's any problem at build time nor run time; end users would just not have dark/light mode detected correctly and have no information why.

So, I think that if you really want to not use zbus, it would be better to implement that using the dbus crate which depends on the libdbus C library. That way the C dependency is clear; downstream developers would get an error at build time if the dependency isn't found, and packagers would need to ensure that libdbus is specified as a dependency. That would obviate the need for a run time error in case an external exectuable isn't found.

[Be-ing]

I'd also be okay with simply rejecting this change and not merging any alternative to zbus if @frewsxcv and @edfloreshz agree. I don't think there's any compelling reason to maintain multiple implementations.

[bbb651]

First, please limit pull requests to one topic so each change can be discussed independently. I'll limit the rest of this comment to making zbus optional; please split off the other commits to separate pull requests.

👍

Making zbus optional was already discussed and dismissed in #15. It doesn't solve any actual problems; shifting dependencies from Rust to C doesn't remove the dependencies. But if you really really insist and are willing to implement it yourself, replacing zbus with a C alternative could be an option off by default.

Like I said, it does solve an actual problem: compile time and binary size. I tested my change on iced, and not relying on zbus reduces first (and after changes to manifest, rust version, etc.) compile time by 3.9s (9%) and binary size by 2.5MB (7%, forgot to check ashpd but it's <287.9KiB), that is pretty significant.
Not to mention zbus = "3.0" is ancient (Aug 17, 2022), so anyone actually using zbus or ashpd, say with accesskit like @mwcampbell's example (which itself is likely to be included a 3rd time, not using even the last zbus 0.3 version, 0.3.14 currently only having 55K / 642K downloads per month, I'm guessing mostly from accesskit itself), is almost guaranteed to include it again and pay that cost again.
Having it off by default mostly defeats the purpose, projects that are large (both in terms of codebase and funding, e.g. Zed) or thoroughly inspect their dependency graph and binary size likely won't be using such "simple" platform abstraction crates, but rather have them in-tree, and they are the ones who are likely to turn on such a feature. Other users might not check every feature on every crate, or might not notice since they use another platform - this ends up hurting users of this crate, transitive users of this crate (e.g. iced) and end-users.

I don't generally expect Rust crates to have a dependency on a C library or external executables unless it's a binding crate to a C library. I would be quite annoyed if I got a build or run time error because of that when an upstream crate could've just used a Rust crate.

It's on an external executable, so a build-time error is irrelevant.

As far as the implementation, I think calling external executables is a questionable approach. There's no guarantee those exectuables exist on any particular system. Even if they happen to be shipped by many distros by default now (which as far as I can tell, you haven't investigated if that's the case), there's no guarantee they will continue doing so in the future. The author of zbus is working on busd, a Rust replacement for dbus-daemon. I suspect some distros will switch to shipping that instead of dbus-daemon in the coming years. It's conceivable that project could continue to a Rust replacement for dbus-send and dbus-monitor in the future, perhaps with incompatible CLIs.

This is delightfully hypocritical, this is literally what is already being done with dconf_rs.
Relying on dbus-send and dbus-monitor is done very commonly: ImHex (44K stars), QOwnNotes (4.6K stars), sctk-adwaita (used by winit) - it's kind of hard to search for code by popularity github, but there are 17.8K results for "dbus-send", and some portion of that is for checking light/dark.

No sane distro will stop shipping dbus-send/dbus-notify.

I've looked a bit at the dbus FAQ and README and it's not entirely clear what stability the dbus binaries have:

API/ABI Policy

Now that D-Bus has reached version 1.0, the objective is that all
applications dynamically linked to libdbus will continue working
indefinitely with the most recent system and session bus daemons.

The main D-Bus package
contains the low-level libdbus, the bus daemon, and a few command-line
tools such as dbus-launch.

The main reason I opted for the binaries over linking with libdbus is that it makes dbus a hard requirement for binaries, if you are for some reason missing dbus, applications won't break (I don't mind looking into dlopen, but that's a lot of additional complexity and unsafe for the library, and afaik it doesn't exist on non-glibc systems, and if dbus is missing somewhere it's probably going to be there).

Also, returning Mode::NoPreference when those exectuables aren't found is incorrect; it's not that there is no system preference, but that detecting it failed. The downstream developer/packager gets no indication that there's any problem at build time nor run time; end users would just not have dark/light mode detected correctly and have no information why.

That is simply not true. For detect, I introduced it as a fallback for the other detection methods, as conservatively as possible, so it's guaranteed to work for more cases. (There's another good reason for it to be a fallback, there's a certain subset of users with broken xdg-desktop-portal implementations that will take 30 seconds to time out, both with zbus and the binaries (haven't tested it with the binaries but there's no reason it wouldn't effect them as well. And I assume subscriptions won't time out but never produce anything but needs testing too.). But it's irrelevant for this discussion)
As for not turning errors to Mode::NoPreference, I was just following what was already being done. The api of this crate returns Mode so it's not possible to give an error, and the code silently gives Mode::NoPreference on every freedesktop backend (look within dconf_detect).
I don't really mind either way, but I'm not convinced returning an error from detect will be useful to anyone, it's a platform abstraction crate so the only thing you can do is show it to the user, but at that point it's simply easier to ask the user directly to choose light/dark or default to one of them. dirs is a good example of this, it could've returned an std::io::Error but instead returns Option<PathBuf>, but if you really care about having meaningful error handling, you would implement the logic directly and not relay on a crate to abstract it.
Even if we switch to Option<Mode> and properly turn errors to options, I'm not even sure Mode::NoPreference is meaningful to users and has a reason to exist, does a user of the library care if detecting failed or if the system explicitly chooses to not them if it's light/dark?
For subscribe it's quite a similar story, I currently kept it the signature the same (ignoring the anyhow change), but I think Option<impl Stream<Item = Mode>> is fine (if we do have an error it should be pretty high-level, maybe just Failed, Unsupported). Here it's not a fallback and can fail where zbus doesn't, but like I said in my last point, it's very unlikely.

[frewsxcv]

Please open separate pull requests for these separate changes

[bbb651]

Can we resolve the zbus issue first? The changes are already pretty much already broken to commits as they would be into PRs (I think just 192daa0 and 28ab70d should be one PR).

[frewsxcv]

Please open separate pull requests for the separate commits

[Be-ing]

Having it off by default mostly defeats the purpose

If you think that, then let's just drop this. Bringing in a C library dependency or a dependency on an external executable by default is IMO a nonstarter. (Just because others call external executables for this too doesn't make it a great idea.)

You know what takes longer than 4 seconds? Making this pull request and commenting on it. Or having your build fail because you don't have libdbus' development package installed, figuring out why your build failed, and fixing the build. Or debugging why an application isn't detecting your dark/light mode preference, finding out you don't have dbus-send, then installing it.

This crate is only for GUI applications. 2.5 MiB additional binary size is not ideal, but hardly a pressing problem. If this was a crate for running on microcontrollers with constrained memory, that would be more concerning, but that's not what this crate is for. If you're really concerned about zbus' binary size, improve that upstream instead of introducing novel problems to downstream dependencies.

[Be-ing]

This is delightfully hypocritical, this is literally what is already being done with dconf_rs.

Yeah and it's not great. That was the only crate available when the implementation was written. Turns out someone wrote Rust bindings for the dconf C library recently, so I'll switch to using those, which should be done anyway considering dconf_rs is unmaintained. If someone writes a pure Rust replacement for the dconf C library in the future, that'd be even better.