Skip to content

Improve error handling#59

Merged
edfloreshz merged 11 commits intomainfrom
revert/split-api
Jan 9, 2025
Merged

Improve error handling#59
edfloreshz merged 11 commits intomainfrom
revert/split-api

Conversation

@edfloreshz
Copy link
Collaborator

@edfloreshz edfloreshz commented Jan 7, 2025

This pull request improves error handling, removes synchronous support and updates the Rust edition.

Note

Given that this crate is aimed at desktop applications, the need to keep a sync API is unnecessary, every GUI toolkit should provide a way to execute async code, and thus, dark-light will be switching to async methods.

Changes

  • Updated the Rust edition to 2021 and added thiserror as a dependency in Cargo.toml.
  • Introduced a new Error enum in src/error.rs to handle various error cases, including detection failures and XDG Desktop Portal communication issues.
  • Updated the detect and subscribe functions across different platforms to return Result types instead of fallbacks.
  • Removed the synchronous API and related examples from the codebase, including sync feature references in Cargo.toml, README.md, and the src/lib.rs file.
  • Updated the README.md to reflect changes in the supported platforms and the new asynchronous API usage.
  • Revised the examples to use the new asynchronous API and error handling.
  • Added a justfile for building and testing the crate across different targets.
  • Refactored the Mode enum to replace Default with Unspecified and added utility methods for checking the mode.

@edfloreshz
Copy link
Collaborator Author

This should be enough for us to publish v2.0.0, we still need to figure out how to fix #47 for platforms that busy loop.

@Be-ing
Copy link
Collaborator

Be-ing commented Jan 7, 2025

I think this is going in the opposite direction of where we should be going. We only have a real async implementation for one platform (Freedesktop). The others are just sync or busy polling, neither of which are useful to wrap in an async API. It's the not-really-async APIs that should be removed. Async should only be available for platforms we actually have async implementations.

every GUI toolkit should provide a way to execute async code

Most Rust GUI libraries are not async executors.

@edfloreshz
Copy link
Collaborator Author

So you are suggesting that we provide an async API for platforms that support it and leave the others sync? That could complicate things for the user as they would have to do conditional compilation.

This commit reverts the split API implementation.

The crate now simply provides a sync `detect`
method and `subscribe` remains async, also
removed `notify` files.
- Added `Error` struct.
- Added `Result` return type to methods.
- Added justfile to facilitate building multiple platforms.
- Updated README.md
@edfloreshz
Copy link
Collaborator Author

edfloreshz commented Jan 7, 2025

Alright, I believe you'll find this acceptable @Be-ing

  • The detect function is back to being sync using block_on for Linux.
  • The subscribe function is still available to every platform, however, using it on unsupported platforms will result in an empty stream, this warning has been added to the docs.

As per your suggestion, the plan is to add support to other platforms later on and avoid busy looping.

@Be-ing
Copy link
Collaborator

Be-ing commented Jan 7, 2025

The subscribe function is still available to every platform, however, using it on unsupported platforms will result in an empty stream, this warning has been added to the docs.

Oh, that's a nice idea to avoid conditional compilation. Then if/when people contribute subscribe implementations for other OSes, it'll just work for downstreams with cargo update.

I'll look at this in detail tomorrow.

@edfloreshz
Copy link
Collaborator Author

@Be-ing Can we merge this? Depending on how #47 resolves I plan to send a future PR to remove subscribe.

Copy link
Collaborator

@Be-ing Be-ing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks pretty good, just a few small comments

@edfloreshz
Copy link
Collaborator Author

I'll try to make the requested changes today and merge it after, thanks!

@edfloreshz edfloreshz changed the title Simplify the API Improve error handling Jan 9, 2025
@edfloreshz edfloreshz merged commit a70ef09 into main Jan 9, 2025
4 checks passed
@edfloreshz edfloreshz deleted the revert/split-api branch January 9, 2025 12:07
@edfloreshz
Copy link
Collaborator Author

Fixes #21

@edfloreshz edfloreshz mentioned this pull request Jan 9, 2025
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