🐛 zb: Make PropertyStream yield changes if service starts later

[ids1024]

This seems like the simplest way to address #1111.

Before this, a PropertyStream created when a service wasn't running would never yield a value, even if the service started later and send property change signals.

But if a service stopped, and then was restarted, property changes would be received.

It seems PropertiesCache::init() was returning an error, from trying to call GetAll with a destination that doesn't exist.

Instead, let init() complete successfully (without populating the cache), so it returns a PropertiesChangedStream which is then monitored for changes.

It may also be good if it yielded values when a service takes ownership of the name, but that's a separate issue since it also applies to service restarts. And an application can handle OwnerChanged itself relatively easily.

[ids1024]

Seems to be working with that updating. (It would be nice if box patterns where stable...).

[zeenix]

Seems to be working with that updating.

Nice!

(It would be nice if box patterns where stable...).

Right. Could you please add a FIXEM comment pointing to the relevant issue?

[ids1024]

Added a comment. rust-lang/rust#29641 has been open since 2015, but apparently rust-lang/rust#87121 supersedes that, so maybe it will actually be stable this decade.

[zeenix]

@ids1024 the CI is failing. I think something goes wrong with the error conversion. Could you please have a look?

[ids1024]

With the error conversion commit, the basic_connection and basic_connection_async tests need to be updated to check for Error::FDO instead of Error::MethodError.

But then there's the error_from_zerror test. Instead of the expected fdo::Error::TimedOut("so long"), it's getting fdo::Error::ZBus::FDO(zbus::Error::FDO(fdo::Error::TimedOut("so long")))...

Because we already converted FDO error variants, and the DBusError macro used to define the conversion from zbus::fdo::Error doesn't handle the zbus::Error::FDO case, and just packs that into fdo::Error::ZBus.

Presumably changing that would require either a manual implementation of From<zbus::Error> that also handles the FDO case, or a special case in the macro. Though I don't know if this error conversion is likely to causes issues anywhere in general.

[zeenix]

fdo::Error::ZBus::FDO(zbus::Error::FDO(fdo::Error::TimedOut("so long")))

Hmm.. this shouldn't be happening. This is what I was hoping to avoid with this part of my suggestion:

unless the conversion ends up with the fall-through fdo::Error::ZBus variant, in which case we discard the fdo::Error and keep the original Error.

[zeenix]

@ids1024 do you intend to continue this work?

[zeenix]

@ids1024 do you intend to continue this work?

I guess not. 😢

[codspeed-hq]

CodSpeed Performance Report

Merging #1389 will not alter performance

_{Comparing ids1024:property-stream (b6d49c3) with main (b5515a9)}

Summary

✅ 22 untouched

[ids1024]

Well, if changing the definition of From<zbus::Error> for fdo::Error to have a branch handling Error::FDO is the correct way to do things, that's demonstrated by copying the expanded DBusError macro code and modifying the From impl.

I guess the only not painfully verbose way to do that would be to add some kind of special case in the DBusError macro...

[ids1024]

It doesn't seem too hard to add a special case in the macro, using an internal boolean attribute parameter.

[zeenix]

@ids1024 Thanks so much for picking this up again! Although I pinged you (was just doing a quick old PR triage), I can't seem to remember exact details here so I'll have to re-read the discussion when I'm not tired (hopefully tomorrow morning). :)

[ids1024]

Added a comment on the special case in zbus_macros, and added a little more detail to the comment in the error conversion code. Presumably 🐛 is appropriate for all three commits, since it seems like a correction to how one might expect things would already work.

[zeenix]

Sorry for the delay in reviewing. I've been drowning in work-work lately. 😭

Added a comment on the special case in zbus_macros, and added a little more detail to the comment in the error conversion code.

Thank you!

Presumably 🐛 is appropriate for all three commits, since it seems like a correction to how one might expect things would already work.

Thanks for adding that but only the last commit is a bug fix so 🐛 doesn't seem appropriate for the other 2. Better candidates: 🥅 🚸.

Otherwise we're ready to merge.

[zeenix]

Otherwise we're ready to merge.

Spoke too soon. I just realised we're missing a test case here. Would it be possible for you to add one?