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

Do we need both sync and async APIs? #45

Open
Be-ing opened this issue Oct 8, 2024 · 4 comments
Open

Do we need both sync and async APIs? #45

Be-ing opened this issue Oct 8, 2024 · 4 comments

Comments

@Be-ing
Copy link
Collaborator

Be-ing commented Oct 8, 2024

I am wondering if we really need to keep the sync API. If we get rid of it, users wanting to use this crate in sync contexts could simply use pollster::block_on in their code and we could advise that in documentation. Alternatively, we could put the sync API behind a feature flag so users that don't need it don't need to bring in the pollster dependency.

@edfloreshz
Copy link
Collaborator

It would be nice to provide a sync method that uses pollster::block_on internally, simplifying the usage of the library.

I also think we should switch to Result<Mode>, errors can occur and we should report them to our users, this is a breaking but necessary change in my opinion.

@bbb651
Copy link
Contributor

bbb651 commented Oct 8, 2024

This is a bit tricky because the api for every other platform is sync, and wrapping them in async isn't great. On the other hand xdg dekstop portals can timeout which takes 30 seconds (it's caused by broken portals setups, but it's somewhat common on DIY/tiling compositors, which is why I was hesitant to remove dconf_rs and use portals by default), and blocking means having no control over.

Also the more I think about how people utilize this crate, I'm starting to think prepending the initial value to notify is a bad idea, an application will usually have some initialization code that may or may not be async, and an event loop with some kind of task system (ignoring some exotic async-first architectures, most retained and immediate gui frameworks work like this).
You probably don't want your application to start out with the default theme, then flicker a couple of frames later, so you'll want to detect the theme in the initialization code. If you've used the notify stream, you now have to somehow move it (along with the detected theme) to your application that, then to a task, and it'll only work if initialization is async in the first place or you manually block with something like use pollster. Otherwise, you can use detect (either sync or async) to get your theme for the initial state, then just create the notify future in a task, but you'll get a duplicate update at the start. Not a huge deal, but I don't see a reason why you would want that inital value to be prepended.

With that said I think having blocking (default), async (off by default) features (with notify not being affected by either of them, maybe being it's own feature?) makes sense, because application initialization might or might not be synchronous but in most cases you'll end up awaiting it before the application starts, so it'll always work by default and you can enable async as an optimization. They should be separate methods of course so they are properly additive.

As I commented in #40, I think returning Option<Mode> would be better (maybe even replacing Mode::Default):

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?

@Be-ing
Copy link
Collaborator Author

Be-ing commented Oct 8, 2024

an application will usually have some initialization code that may or may not be async

That's a good point. Application initialization is generally synchronous.

@Be-ing
Copy link
Collaborator Author

Be-ing commented Oct 18, 2024

Considering the async API isn't working at all on macOS and it's not trivial to figure out how make it work (#47), I'm wondering if we should make the async API platform-specific for the initial release of the async API.

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

No branches or pull requests

3 participants